petri: work around linker issue with linkme #2084
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements a workaround for a linker issue with the linkme crate by modifying the distributed slice to use Option types and ensuring at least one entry exists.
- Modified the
TESTSdistributed slice to useOption<fn()>instead offn()directly - Added a dummy
Noneentry to prevent linker bugs when no tests are registered - Removed the
unsafe_codeforbid directive to allow the workaround
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| petri/src/test.rs | Modified test registration system to use Optional function pointers and added workaround entry |
| petri/src/lib.rs | Removed unsafe code prohibition to accommodate linkme workaround |
| #[doc(hidden)] | ||
| pub mod test_macro_support { | ||
| // UNSAFETY: Needed for linkme. |
There was a problem hiding this comment.
The safety comment is too vague. It should explain specifically why unsafe code is needed for linkme and what safety guarantees are being maintained.
| // UNSAFETY: Needed for linkme. | |
| // UNSAFETY: The `linkme` crate requires `unsafe` code internally to implement distributed slices, | |
| // which rely on linker-level manipulation to collect static references across compilation units. | |
| // This module does not use `unsafe` code directly, but expects it due to the use of `linkme`. | |
| // The safety of distributed slices is guaranteed by the invariants maintained by the `linkme` crate. |
Copilot uses AI. Check for mistakes.
|
|
||
| // Always have at least one entry to work around linker bugs. | ||
| // | ||
| // See <https://github.com/llvm/llvm-project/issues/65855>. |
There was a problem hiding this comment.
IIRC elsewhere where we have this workaround we have a longer comment talking about nulls as well, we should copy that to each case for full descriptiveness.
There was a problem hiding this comment.
Or maybe we can centralize the workaround in some way? Would it be possible to do things in a macro in a new support crate or something?
There was a problem hiding this comment.
Ehh, I think the null thing is hypothetical; we've never actually seen it.
Maybe we could centralize this workaround. I don't really want to right now, though.
|
Ah yep, that makes sense. |
No description provided.