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

Support .note.gnu.property #196

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

marxin
Copy link
Contributor

@marxin marxin commented Nov 24, 2024

The PR implements the feature where I would like to discuss the following things:

  1. The merging of the sections happens for all object files where the output section is present just once and should be synthesized. That's why I chose to implement it in EpilogueLayout, where the corresponding sections are identified based on the newly added SectionSlot value. Is it a good design choice?
  2. The implementation supports (similarly to Mold) only properties with 4B property data. Do you have a hint on how to do a smarter serialization in elf_writer.rs? Theoretically, GnuProperty 1 from object create can be converted to not only read mode.
  3. I face an issue in testing:
Error: Unexpected memory offsets:
Part #17 (section `.note.gnu.property` alignment: 8) expected: 0x400408 actual: 0x4003d8 bumped by: 0x0 requested size: 0x30

Can you help me with that?

  1. The output (.gnu.note.property) should be binary equal to what other linkers do. That said, will we need any special handling in linker-diff`, or is the binary diff the default one?

Thanks for review in advance!

Implements: #38

Footnotes

  1. https://docs.rs/object/latest/object/read/elf/struct.GnuProperty.html

@marxin
Copy link
Contributor Author

marxin commented Nov 24, 2024

2. The implementation supports (similarly to Mold) only properties with 4B property data. Do you have a hint on how to do a smarter serialization in elf_writer.rs? Theoretically, GnuProperty 1 from object create can be converted to not only read mode.

FYI: @philipc

@davidlattimore
Copy link
Owner

Error: Unexpected memory offsets:
Part #17 (section .note.gnu.property alignment: 8) expected: 0x400408 actual: 0x4003d8 bumped by: 0x0 requested size: 0x30

Allocation of addresses within the output file goes through two phases. In the first phase, objects make requests for how much space they want in a particular section. In the second phase (finalise_layout), objects take addresses in the sections in which they requested space. They do this by getting the next address for that section from the argument memory_offsets. Once they take their address, they need to increment the next address for that section so that any subsequent files that also put stuff into that section don't get overlapping addresses.

So one possible fix is to make sure that finalise_layout increments memory_offsets for the .note.gnu.property section by whatever size it is using.

Alternatively, assuming the only "file" that ever does stuff with .note.gnu.property is the epilogue, then you can just ignore that PartId, by adding it in verification::clear_ignored.

@davidlattimore
Copy link
Owner

The merging of the sections happens for all object files where the output section is present just once and should be synthesized. That's why I chose to implement it in EpilogueLayout, where the corresponding sections are identified based on the newly added SectionSlot value. Is it a good design choice?

I think that's reasonable. Wherever there are things that aren't obviously "owned" by one particular input object, I usually write them from either the prologue or the epilogue. In some other cases, I pick a "winning" input object to be responsible for writing a particular thing. For example if multiple input objects define something, just one of them will be selected to write that thing. Either approach is fine, so whichever you think would be cleanest.

The implementation supports (similarly to Mold) only properties with 4B property data. Do you have a hint on how to do a smarter serialization in elf_writer.rs? Theoretically, GnuProperty 1 from object create can be converted to not only read mode.

If Object doesn't (currently) provide a struct that we can use, then perhaps just define a struct in elf.rs then derive Pod. e.g. see EhFrameHdr.

@davidlattimore
Copy link
Owner

The output (.gnu.note.property) should be binary equal to what other linkers do. That said, will we need any special handling in linker-diff`, or is the binary diff the default one?

Linker-diff currently won't compare section content by default. I'm not aware of any other sections where we expect all linkers to produce exactly equal content, so this would be the first.

Copy link
Owner

@davidlattimore davidlattimore left a comment

Choose a reason for hiding this comment

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

Nice! Thanks for doing this - it'll be good to have this gap filled

// If all bits in the the output pr_data field are zero, this property should be removed from output.
Or,
// A bit in the output pr_data field is set only if it is set in all relocatable input pr_data fields.
// If all bits in the the output pr_data field are zero, this property should be removed from output.
Copy link
Owner

Choose a reason for hiding this comment

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

Seems odd that this isn't the opposite of Or. i.e. I would have expected that the property would be removed if any bit in the output pr_data field was zero. But what you've got here matches what's in the spec. I probably don't have a deep enough understanding of the intent behind these rules - although now that I look at your implementation, it looks like that doesn't match what's written here, but does match the behaviour that I would expect. I guess the question is, is the spec wrong or do the other linkers implement it as written here.

Copy link
Contributor Author

@marxin marxin Nov 25, 2024

Choose a reason for hiding this comment

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

Well, the wording is a bit convoluted, but it's correct:

// A bit in the output pr_data field is set only if it is set in all relocatable input pr_data field

This correctly maps to the fact it's AND property - so a bit is 1 if it's 1 in all inputs (we are merging bits in pr_data).

// If all bits in the the output pr_data field are zero, this property should be removed from output.

This speaks about a property (can have property type e.g. GNU_PROPERTY_X86_FEATURE_1_AND), where if you merge all bits and there is set none, you can omit the entire property. Values of this particular property can be GNU_PROPERTY_X86_FEATURE_1_IBT, or GNU_PROPERTY_X86_FEATURE_1_SHSTK.

Is it clearer now?

Copy link
Owner

Choose a reason for hiding this comment

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

I think I understand. So I had (mis) interpreted this as saying if all inputs are 0 then the output should be omitted, but it sounds like what it's actually saying is if the output is 0, then the output should be omitted. Is that right?

wild_lib/src/layout.rs Outdated Show resolved Hide resolved
@@ -769,7 +772,10 @@ fn resolve_sections_for_object<'data>(
SectionSlot::Unloaded(UnloadedSection::new(id))
}
TemporaryPartId::Custom(custom_section_id, _alignment) => {
if custom_section_id.name.bytes().starts_with(b".debug_") {
let section_name = custom_section_id.name.bytes();
if section_name == b".note.gnu.property" {
Copy link
Owner

Choose a reason for hiding this comment

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

Making ".note.gnu.property" a custom section seems a bit odd, since you define a PartId for the section. It means that a custom section PartId for the section will be allocated, but never used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All right, let me use TemporaryPartId::NoteGnuProperty, hope it's the right approach?!

Copy link
Owner

Choose a reason for hiding this comment

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

Rather than introducing a new variant of TemporaryPartId, would it work to use TemporaryPartId::BuiltIn(part_id::NOTE_GNU_PROPERTY:)?

@philipc
Copy link
Contributor

philipc commented Nov 25, 2024

The implementation supports (similarly to Mold) only properties with 4B property data. Do you have a hint on how to do a smarter serialization in elf_writer.rs? Theoretically, GnuProperty from object create can be converted to not only read mode.

GnuProperty is not an appropriate struct for this. It contains a reference to a slice of data which must already be in the correct byte order. Really what you want is something that contains a u32 property and u32 value. It's pretty easy to write that yourself though in three lines of code, as already done elsewhere in object.

@marxin
Copy link
Contributor Author

marxin commented Nov 25, 2024

So one possible fix is to make sure that finalise_layout increments memory_offsets for the .note.gnu.property section by whatever size it is using.

Thanks for reminding me how the linker works :) So yes, I used the appropriate way and bumped memory_offsets.

@marxin
Copy link
Contributor Author

marxin commented Nov 25, 2024

Linker-diff currently won't compare section content by default.

Heh. I was too optimistic as we'll need something more complicated. I noticed LLD omits the section for a trivial test case where BFD produces:

Displaying notes found in: .note.gnu.property
  Owner                Data size 	Description
  GNU                  0x00000030	NT_GNU_PROPERTY_TYPE_0
      Properties: x86 ISA needed: x86-64-baseline
	x86 feature used: x86
	x86 ISA used: 

And I also noticed the order of the properties depends on the order of the files, where stable output can be ensured by sorting the properties by their type. So a proper comparison (using 1) would be a better approach!

Footnotes

  1. https://docs.rs/object/latest/object/read/elf/struct.Note.html#method.gnu_properties

@marxin
Copy link
Contributor Author

marxin commented Nov 25, 2024

If Object doesn't (currently) provide a struct that we can use, then perhaps just define a struct in elf.rs then derive Pod. e.g. see EhFrameHdr.

Yeah, I've just done that and the code seems nicer (at least I hope).

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.

3 participants