fix: OR GNU property notes within file before ANDing across files - #2309
Conversation
merge_gnu_property_notes was incorrectly ANDing all property entries together including multiple entries within the same file. The correct behavior is to OR entries within a file (accumulate all features the file supports) and then AND across files (keep only features all files support). For example, a single file with separate BTI (0x1) and PAC (0x2) entries for GNU_PROPERTY_AARCH64_FEATURE_1_AND was being merged as 0x1 & 0x2 = 0x0 instead of 0x1 | 0x2 = 0x3. This also fixes .note.gnu.property not being output in relocatable links (-r), since the zero value was being filtered out.
There was a problem hiding this comment.
It looks like this test passes without the fix. Can you see what needs changing with the test so that it fully exercises the expected behaviour?
There was a problem hiding this comment.
You're right! Fixed. I updated the test to exercise the expected behavior and also updated the commit message.
The previous test had two separate notes each with one property, which did not exercise the OR-within-file behavior. Updated to use a single note with two property entries (BTI=1 and PAC=2) so the test correctly fails without the fix (0x1 & 0x2 = 0) and passes with it (0x1 | 0x2 = 0x3).
| .text | ||
| ret | ||
| // Single .note.gnu.property section with TWO property entries in one note. | ||
| // This triggers the bug where Wild ANDs BTI(1) & PAC(2) = 0 instead of OR-ing. |
There was a problem hiding this comment.
We always want comments to be written so that make sense to someone reading them after the PR is merged. Once the PR is merged, wild shouldn't have that bug, so the comment could be misleading. I'd suggest wording like "This verifies that the linker does ___ not ___"
There was a problem hiding this comment.
Updated the comment as suggested. I also updated the test and verified that it fails on upstream/main and passes with this change. Please take another look when you get a chance?
davidlattimore
left a comment
There was a problem hiding this comment.
Did you try the new version of your test without the fix? I just tried it and it still passes for me.
Use a single note with two property entries (BTI=1 and PAC=2) inline rather than a separate relocatable input file. Add ExpectSection check to verify .note.gnu.property is present in output. Use lld as reference linker since GNU ld cross linker does not output .note.gnu.property in static mode. The test now correctly fails without the fix (BTI&PAC=0 is filtered out) and passes with the fix (BTI|PAC=3 is preserved).
|
Thank you for the fix ;) |
merge_gnu_property_noteswas ANDing all property entries together, including multiple entries within the same file.A file with separate BTI (0x1) and PAC (0x2) entries was merged as:
0x1 & 0x2 = 0x0→ filtered out → no.note.gnu.propertyin output!Also fixes
.note.gnu.propertynot being output in-rmode, sinc the zero value was being filtered out.Issue #2247