Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix!: explicitly mark _KGDTENTRY64 and _KIDTENTRY64 as opaque types in bindgen #277

Draft
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

wmmc88
Copy link
Collaborator

@wmmc88 wmmc88 commented Jan 24, 2025

The new types are generated as:

#[repr(C)]
#[derive(Copy, Clone)]
pub union _KGDTENTRY64 {
    pub _address: u8,
}
pub union _KIDTENTRY64 {
    pub _address: u8,
}

and replace the historic structs that were manually added to the bindgen input header. These historic structs are no longer present in modern WDK headers, and these types should be treated as opaque.
 
BREAKING CHANGE: definitions for _KGDTENTRY64 and _KIDTENTRY64 have been removed

Note: only review the top commit as this pr is blocked on #260, #263

@wmmc88 wmmc88 requested a review from leon-xd January 24, 2025 01:59
@wmmc88 wmmc88 self-assigned this Jan 24, 2025
@wmmc88 wmmc88 requested review from a team and Copilot January 24, 2025 01:59
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 24 out of 39 changed files in this pull request and generated 2 comments.

Files not reviewed (15)
  • .vscode/settings.json: Language not supported
  • crates/wdk-sys/src/input.h: Language not supported
  • crates/wdk-sys/src/wdf.c: Language not supported
  • .github/workflows/code-formatting-check.yaml: Evaluated as low risk
  • .github/workflows/codeql.yml: Evaluated as low risk
  • .github/workflows/build.yaml: Evaluated as low risk
  • Cargo.toml: Evaluated as low risk
  • crates/wdk-sys/src/ntddk.rs: Evaluated as low risk
  • crates/wdk-build/src/bindgen.rs: Evaluated as low risk
  • CONTRIBUTING.md: Evaluated as low risk
  • .github/workflows/docs.yaml: Evaluated as low risk
  • crates/wdk-sys/Cargo.toml: Evaluated as low risk
  • crates/wdk-sys/src/lib.rs: Evaluated as low risk
  • .github/workflows/test.yaml: Evaluated as low risk
  • .github/workflows/lint.yaml: Evaluated as low risk
Comments suppressed due to low confidence (3)

crates/wdk-sys/src/hid.rs:13

  • The reason parameter is not valid for the #[allow] attribute. It should be removed.
#[allow(missing_docs, reason = "most items in the WDK headers have no inline documentation, so bindgen is unable to generate documentation for their bindings")]

crates/wdk-sys/src/hid.rs:19

  • The reason parameter is not valid for the #[allow] attribute. It should be removed.
#[allow(clippy::wildcard_imports, reason = "the underlying c code relies on all type definitions being in scope, which results in the bindgen generated code relying on the generated types being in scope as well")]

crates/wdk-sys/build.rs:47

  • The usage of unreachable! macro might not be the best way to handle unexpected configurations. Consider using a more descriptive error message or handle the case more gracefully.
unreachable!("generate_wdf_function_table is only called with WDF driver configurations")

crates/wdk-build/src/lib.rs Outdated Show resolved Hide resolved
crates/wdk-build/src/lib.rs Outdated Show resolved Hide resolved
@wmmc88 wmmc88 force-pushed the opaque-missing-km-structs branch from b8ca1b1 to 7e5a77a Compare January 24, 2025 03:27
@wmmc88 wmmc88 force-pushed the opaque-missing-km-structs branch from 7e5a77a to 85dc60f Compare January 24, 2025 18:40
…s in bindgen

BREAKING CHANGE: defintions for `_KGDTENTRY64` and `_KIDTENTRY64` have been removed
@wmmc88 wmmc88 force-pushed the opaque-missing-km-structs branch from 85dc60f to d8c1e96 Compare January 25, 2025 04:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant