Skip to content

fix: avoid Windows crash while preserving entry metadata#38

Open
qartik wants to merge 8 commits intomainfrom
ks/fix-windows-entry-attrs-crash
Open

fix: avoid Windows crash while preserving entry metadata#38
qartik wants to merge 8 commits intomainfrom
ks/fix-windows-entry-attrs-crash

Conversation

@qartik
Copy link
Member

@qartik qartik commented Mar 6, 2026

Summary

This PR fixes the Windows CI crash in the isolated test_get_entry_attributes path, scoped separately from the broader QIR 2.0 work in #34.

What Changed

  • Avoid generic inkwell function-attribute iteration when finding the entry function.
  • Parse QIR bitcode with LLVM bitcode APIs instead of routing bitcode through create_module_from_ir.
  • Restore full entry metadata extraction by enumerating all entry-point string attributes via raw LLVM APIs.
  • Unify entry attribute extraction so get_string_attrs safely returns all string function attributes again.
  • Strip all translated entry-function string attributes after translation, including custom metadata, while still preserving them for get_entry_attributes on input QIR.
  • Add a regression test covering custom entry attributes so extraction still works on input and cleanup still happens in translated QIS output.

Notes

#34 overlaps with adjacent code, but it does not fully replace this fix on its own. This branch also updates the bitcode parsing path used by get_entry_attributes, validate_qir, and qir_to_qis, and preserves spec-compatible metadata extraction for output schemas without leaking source entry metadata into translated output.

Copilot AI review requested due to automatic review settings March 6, 2026 21:13
Copy link

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.

Pull request overview

This PR updates the entry-metadata handling path to avoid generic LLVM function attribute iteration, addressing a Windows CI crash (STATUS_ACCESS_VIOLATION) likely originating from LLVM/inkwell FFI behavior.

Changes:

  • Switch entry-function detection to a direct lookup for the entry_point string attribute (no attribute iteration).
  • Update get_entry_attributes to fetch a fixed set of known QIR entry attributes via direct lookups.
  • Remove entry-related attributes during translation by explicitly removing known keys rather than iterating attributes.

Reviewed changes

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

File Description
src/lib.rs Replaces attribute iteration with fixed-key lookups for entry metadata extraction and fixed-key removal during translation.
src/convert.rs Updates find_entry_function to use direct attribute lookup and removes the generic string-attribute iterator helper.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@qartik qartik changed the title fix: avoid LLVM attribute iteration for entry metadata fix: avoid Windows crash while preserving entry metadata Mar 9, 2026
Copy link

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.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link

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.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +194 to +206
let count = usize::try_from(unsafe {
LLVMGetAttributeCountAtIndex(function.as_value_ref(), LLVMAttributeFunctionIndex)
})
.unwrap_or(0);
let mut attrs: Vec<LLVMAttributeRef> = vec![std::ptr::null_mut(); count];

unsafe {
LLVMGetAttributesAtIndex(
function.as_value_ref(),
LLVMAttributeFunctionIndex,
attrs.as_mut_ptr(),
);
}
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

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

get_string_attrs always calls LLVMGetAttributesAtIndex even when the attribute count is 0. When count == 0, attrs.as_mut_ptr() comes from an empty Vec and is a dangling non-null pointer; passing it across FFI is risky if LLVM ever dereferences the pointer even when the count is zero. Guard the call (only invoke LLVMGetAttributesAtIndex when count > 0) or pass a null pointer in the zero-count case.

Copilot uses AI. Check for mistakes.
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.

2 participants