This is an archive of the discontinued LLVM Phabricator instance.

[lld][ELF] add .note sections from linker scripts as SHT_NOTE
AbandonedPublic

Authored by bluca on Jan 28 2022, 9:52 AM.

Details

Reviewers
MaskRay
ruiu
Summary

For compatibility with ld.bfd:

https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=bfd/elf.c;h=14c2c7ba734d4d0e5ccec7a637597aae08a97777;hb=HEAD#l2700

bfd marks any section named .note* as SHT_NOTE.

This is required by the packaging notes spec, which is rolled out
starting from Fedora 36.

https://fedoraproject.org/wiki/Changes/Package_information_on_ELF_objects

Diff Detail

Event Timeline

bluca created this revision.Jan 28 2022, 9:52 AM
bluca requested review of this revision.Jan 28 2022, 9:52 AM
bluca added a reviewer: ruiu.Jan 28 2022, 9:55 AM
bluca set the repository for this revision to rG LLVM Github Monorepo.
Herald added a project: Restricted Project. · View Herald TranscriptJan 28 2022, 9:55 AM
MaskRay requested changes to this revision.Jan 28 2022, 9:57 AM

Not very comfortable with this. I think GNU ld has other rules in this area and I am not sure we want to follow them all.
Can you file a feature request on https://sourceware.org/bugzilla/ to add a way to specify the output section type? https://sourceware.org/binutils/docs/ld/Output-Section-Type.html#Output-Section-Type

This revision now requires changes to proceed.Jan 28 2022, 9:57 AM
bluca added a comment.EditedJan 28 2022, 10:07 AM

Not very comfortable with this. I think GNU ld has other rules in this area and I am not sure we want to follow them all.
Can you file a feature request on https://sourceware.org/bugzilla/ to add a way to specify the output section type? https://sourceware.org/binutils/docs/ld/Output-Section-Type.html#Output-Section-Type

Hi,

The problem is that breaks backward compatibility with bfd, because there's no such specifier there, so we can't change how it behaves with existing scripts. For the linked feature ( https://fedoraproject.org/wiki/Changes/Package_information_on_ELF_objects ), we have a distro-wide linker script that we need to use for all builds. It's not really doable to do per-build-dependency generation, it's hard enough as it is. We need full compatibility.

Also, isn't compatibility with input supported by bfd a project goal? If you take the linker script I added as a test and run it through bfd and lld, they'll generate different and incompatible outputs that breaks tools like systemd-coredump.

Finally it's not just the section type, it also needs to be allocatable.

Not very comfortable with this. I think GNU ld has other rules in this area and I am not sure we want to follow them all.
Can you file a feature request on https://sourceware.org/bugzilla/ to add a way to specify the output section type? https://sourceware.org/binutils/docs/ld/Output-Section-Type.html#Output-Section-Type

Or maybe I misunderstood - are you asking for a no-op to be added to bfd before this can be merged?

Not very comfortable with this. I think GNU ld has other rules in this area and I am not sure we want to follow them all.
Can you file a feature request on https://sourceware.org/bugzilla/ to add a way to specify the output section type? https://sourceware.org/binutils/docs/ld/Output-Section-Type.html#Output-Section-Type

Hi,

The problem is that breaks backward compatibility with bfd, because there's no such specifier there, so we can't change how it behaves with existing scripts. For the linked feature ( https://fedoraproject.org/wiki/Changes/Package_information_on_ELF_objects ), we have a distro-wide linker script that we need to use for all builds. It's not really doable to do per-build-dependency generation, it's hard enough as it is. We need full compatibility.

Also, isn't compatibility with input supported by bfd a project goal? If you take the linker script I added as a test and run it through bfd and lld, they'll generate different and incompatible outputs that breaks tools like systemd-coredump.

Finally it's not just the section type, it also needs to be allocatable.

Let me add some more context on this feature work, given that wiki page is quite long. Fedora 36 is being rebuilt with every ELF binary including a small note following this specification:

https://systemd.io/COREDUMP_PACKAGE_METADATA/

the purpose of this in short is to give reliable and accurate metadata when things crash, regardless of the state of the system, configuration, network, etc, for the benefit of support engineers, maintainers and developers.

So we need this note to appear in SHT_NOTE format, and allocatable (SHF_ALLOC) so that it's loaded in memory and automatically included in the core file generated by the kernel.
systemd-coredump can then find it in the core file and add structured metadata to the journal and various log messages, and tooling handling cores like coredumpctl shows it by default, and so on.

Given lld is not compatible with the linker script we generated, which is accepted by bfd, we have to disable this feature for packages using lld. This is of course not ideal, and it's lld that gets blamed due to the lack of compatibility with bfd, so I'm attempting to fix it.

Which these two patches, the existing linker scripts we are using work out of the box and the binaries behave in the expected way, just like if they were built with gcc and bfd.

Hope this helps. Thanks!

Hi,

The problem is that breaks backward compatibility with bfd, because there's no such specifier there, so we can't change how it behaves with existing scripts. For the linked feature ( https://fedoraproject.org/wiki/Changes/Package_information_on_ELF_objects ), we have a distro-wide linker script that we need to use for all builds. It's not really doable to do per-build-dependency generation, it's hard enough as it is. We need full compatibility.

Also, isn't compatibility with input supported by bfd a project goal? If you take the linker script I added as a test and run it through bfd and lld, they'll generate different and incompatible outputs that breaks tools like systemd-coredump.

It's complex. https://maskray.me/blog/2020-12-19-lld-and-gnu-linker-incompatibilities
It is not uncommon for ld.lld to simplify or adapt GNU ld's features/behaviors instead of porting it bug for bug.
Every one needs to be assessed differently and I have personally contributed many portability changes.

Finally it's not just the section type, it also needs to be allocatable.

See below.

Let me add some more context on this feature work, given that wiki page is quite long. Fedora 36 is being rebuilt with every ELF binary including a small note following this specification:

https://systemd.io/COREDUMP_PACKAGE_METADATA/

the purpose of this in short is to give reliable and accurate metadata when things crash, regardless of the state of the system, configuration, network, etc, for the benefit of support engineers, maintainers and developers.

So we need this note to appear in SHT_NOTE format, and allocatable (SHF_ALLOC) so that it's loaded in memory and automatically included in the core file generated by the kernel.
systemd-coredump can then find it in the core file and add structured metadata to the journal and various log messages, and tooling handling cores like coredumpctl shows it by default, and so on.

Thanks for providing the link.

Given lld is not compatible with the linker script we generated, which is accepted by bfd, we have to disable this feature for packages using lld. This is of course not ideal, and it's lld that gets blamed due to the lack of compatibility with bfd, so I'm attempting to fix it.

Thanks. I think it'd be nice for lld to support this as well, but I think we should implement this feature properly. So please bear me with some pushback.
Have you considered using an ELF relocatable object file instead of a linker script?
This will be more readable than using the BYTE data commands.

.section .note.gnu.property,"a",%notes
.long 4
...
.ascii "...."

In addition, INSERT AFTER .note.gnu.build-id; has a problem that if the user specifies --build-id=none, there will be a linker error.
I don't think it is right to break a link if the user doesn't want build ID.
build ID computation is slow (worse, GNU ld does not have parallelism), so it is not justified for any arbitrary user program which is not intended to be distributed like a distro package to incur the overhead.

Moreover, gold does not support INSERT AFTER (https://sourceware.org/bugzilla/show_bug.cgi?id=15373), so using INSERT for every distro package will lock out gold.

In the other patch, you added READONLY, which implies that you do not compatibility with older linkers.
Then I don't understand why a new output section type cannot be added to make the .note* => SHT_NOTE less magic.
You don't need to worry about SHF_ALLOC because output section descriptions are SHF_ALLOC by default.

(I think communication can usually be improved by informing the involved parties early.
An un-involved party may feel frustration, especially if you only started conversation with one camp and then essentially force the other camp to accept.)

bluca added a comment.Jan 28 2022, 3:59 PM

Hi,

The problem is that breaks backward compatibility with bfd, because there's no such specifier there, so we can't change how it behaves with existing scripts. For the linked feature ( https://fedoraproject.org/wiki/Changes/Package_information_on_ELF_objects ), we have a distro-wide linker script that we need to use for all builds. It's not really doable to do per-build-dependency generation, it's hard enough as it is. We need full compatibility.

Also, isn't compatibility with input supported by bfd a project goal? If you take the linker script I added as a test and run it through bfd and lld, they'll generate different and incompatible outputs that breaks tools like systemd-coredump.

It's complex. https://maskray.me/blog/2020-12-19-lld-and-gnu-linker-incompatibilities
It is not uncommon for ld.lld to simplify or adapt GNU ld's features/behaviors instead of porting it bug for bug.
Every one needs to be assessed differently and I have personally contributed many portability changes.

Finally it's not just the section type, it also needs to be allocatable.

See below.

Let me add some more context on this feature work, given that wiki page is quite long. Fedora 36 is being rebuilt with every ELF binary including a small note following this specification:

https://systemd.io/COREDUMP_PACKAGE_METADATA/

the purpose of this in short is to give reliable and accurate metadata when things crash, regardless of the state of the system, configuration, network, etc, for the benefit of support engineers, maintainers and developers.

So we need this note to appear in SHT_NOTE format, and allocatable (SHF_ALLOC) so that it's loaded in memory and automatically included in the core file generated by the kernel.
systemd-coredump can then find it in the core file and add structured metadata to the journal and various log messages, and tooling handling cores like coredumpctl shows it by default, and so on.

Thanks for providing the link.

Given lld is not compatible with the linker script we generated, which is accepted by bfd, we have to disable this feature for packages using lld. This is of course not ideal, and it's lld that gets blamed due to the lack of compatibility with bfd, so I'm attempting to fix it.

Thanks. I think it'd be nice for lld to support this as well, but I think we should implement this feature properly. So please bear me with some pushback.
Have you considered using an ELF relocatable object file instead of a linker script?
This will be more readable than using the BYTE data commands.

.section .note.gnu.property,"a",%notes
.long 4
...
.ascii "...."

We have considered compiled objects and discarded that option as unfeasible. It's an absolute nightmare to even get a simple linker script to work reliably across 30.000 packages with a huge variation of different build systems and setups, across half a dozen different architectures. Having to also take care of an additional compilation steps means the whole endeavor will simply fail due to complications, so it is not an option.

In addition, INSERT AFTER .note.gnu.build-id; has a problem that if the user specifies --build-id=none, there will be a linker error.
I don't think it is right to break a link if the user doesn't want build ID.
build ID computation is slow (worse, GNU ld does not have parallelism), so it is not justified for any arbitrary user program which is not intended to be distributed like a distro package to incur the overhead.

Those are not issues for this use case - this is for distribution builds. The build-id is _always_ included and always will be. Of course users doing their own builds are free to follow the same spec in different way if they wishes, that's not an issue, they don't have to use the same linker script, but here we are talking about a distribution-wide specification where the build-id is mandatory.

Moreover, gold does not support INSERT AFTER (https://sourceware.org/bugzilla/show_bug.cgi?id=15373), so using INSERT for every distro package will lock out gold.

In the other patch, you added READONLY, which implies that you do not compatibility with older linkers.
Then I don't understand why a new output section type cannot be added to make the .note* => SHT_NOTE less magic.

This linker script is used distro-wide, which means we have control over which versions of the various linkers are used. Hence, there are not older linkers in play, and it's fine if this particular script doesn't work with older versions. The issue is with differences between current versions of different linkers: due to how rpm macros and build systems work we can't really have different scripts, so the script needs to work with both linkers. So I cannot add a new output section type that is specific to lld, because it would break when building with bfd.

You don't need to worry about SHF_ALLOC because output section descriptions are SHF_ALLOC by default.

With the current git head of lld, without the second part of this patch, the note section is always generated without SHF_ALLOC, because the flags are reset during the loop in LinkerScript::adjustSectionsBeforeSorting here:

https://github.com/llvm/llvm-project/blob/main/lld/ELF/LinkerScript.cpp#L1154

It's easy to see this behaviour with the linker script I included in the test section of the patch, without the change in that function you'll see that the note section in the linked elf doesn't have the SHF_ALLOC flag set. Run the integration test provided, which expects SHF_ALLOC to be set, and you'll see it fail.

(I think communication can usually be improved by informing the involved parties early.
An un-involved party may feel frustration, especially if you only started conversation with one camp and then essentially force the other camp to accept.)

I'm sorry if you feel frustrated, but I cannot change the past. Also, I assure you there was no conversation with the bfd 'camp' on this topic, so there's no 'favoritism' - I don't think I even mentioned this specification when I sent the READONLY patch, as it was completely orthogonal. Please understand that this is a complex effort with a huge number of moving parts (that has been discussed many times in many public places where anybody could chime in, including tech press articles and conferences), and the critical and really complicated pieces are elsewhere in the stack.

MaskRay added a comment.EditedJan 29 2022, 12:46 AM

Non-SHF_ALLOC SHT_NOTE sections exists in the wild.

I have created two patches to improve the propagating SHF_ALLOC situation.


For the section type, I have filed a feature request ld: Customize output section type.
If an arbitrary output section type is too difficult, supporting just SHT_NOTE seems good enough.

We have considered compiled objects and discarded that option as unfeasible. It's an absolute nightmare to even get a simple linker script to work reliably across 30.000 packages with a huge variation of different build systems and setups, across half a dozen different architectures. Having to also take care of an additional compilation steps means the whole endeavor will simply fail due to complications, so it is not an option.

https://fedoraproject.org/wiki/Changes/Package_information_on_ELF_objects references https://github.com/systemd/package-notes/blob/main/hello.spec .
Can't we replace -Wl,-dT,$PWD/notes.ld} with an compiled .o?
Then it will work all linkers: GNU ld, gold, ld.lld, and mold, perhaps also the abandoned MCLinker.

(I think communication can usually be improved by informing the involved parties early.

An un-involved party may feel frustration, especially if you only started conversation with one camp and then essentially force the other camp to accept.)

I'm sorry if you feel frustrated, but I cannot change the past. Also, I assure you there was no conversation with the bfd 'camp' on this topic, so there's no 'favoritism' - I don't think I even mentioned this specification when I sent the READONLY patch, as it was completely orthogonal. Please understand that this is a complex effort with a huge number of moving parts (that has been discussed many times in many public places where anybody could chime in, including tech press articles and conferences), and the critical and really complicated pieces are elsewhere in the stack.

(I did not want to make this review a blame place, but I have mentioned READONLY in two messages https://sourceware.org/pipermail/binutils/2021-May/116579.html https://sourceware.org/pipermail/binutils/2021-July/117492.html and I carefully ensured the patch author was on the To:/Cc: lines.
Perhaps I really should have kept more vigilance if I knew this going to be used in systemd, or, all Fedora packages?
I just raised https://sourceware.org/pipermail/binutils/2022-January/119537.html to question the error-prone READONLY
)

nikic added a subscriber: nikic.Jan 29 2022, 12:46 AM
bluca added a comment.Jan 29 2022, 9:58 AM

Non-SHF_ALLOC SHT_NOTE sections exists in the wild.

I have created two patches to improve the propagating SHF_ALLOC situation.

Thank you, I have tested them and can confirm this fixes the SHF_ALLOC problem. Updated this patch to remove the second part.


For the section type, I have filed a feature request ld: Customize output section type.
If an arbitrary output section type is too difficult, supporting just SHT_NOTE seems good enough.

This would break backward compatibility in bfd, as suddenly linker scripts that produced SHT_NOTE sections would no longer do so without the option, so I don't think it is a viable option. And having it only for lld means there's source-level incompatibility with bfd, so that doesn't look like a viable option either.

We have considered compiled objects and discarded that option as unfeasible. It's an absolute nightmare to even get a simple linker script to work reliably across 30.000 packages with a huge variation of different build systems and setups, across half a dozen different architectures. Having to also take care of an additional compilation steps means the whole endeavor will simply fail due to complications, so it is not an option.

https://fedoraproject.org/wiki/Changes/Package_information_on_ELF_objects references https://github.com/systemd/package-notes/blob/main/hello.spec .
Can't we replace -Wl,-dT,$PWD/notes.ld} with an compiled .o?
Then it will work all linkers: GNU ld, gold, ld.lld, and mold, perhaps also the abandoned MCLinker.

No, I already explained in the sentence you quoted, compiled objecs are not a realistic option.

(I think communication can usually be improved by informing the involved parties early.

An un-involved party may feel frustration, especially if you only started conversation with one camp and then essentially force the other camp to accept.)

I'm sorry if you feel frustrated, but I cannot change the past. Also, I assure you there was no conversation with the bfd 'camp' on this topic, so there's no 'favoritism' - I don't think I even mentioned this specification when I sent the READONLY patch, as it was completely orthogonal. Please understand that this is a complex effort with a huge number of moving parts (that has been discussed many times in many public places where anybody could chime in, including tech press articles and conferences), and the critical and really complicated pieces are elsewhere in the stack.

(I did not want to make this review a blame place, but I have mentioned READONLY in two messages https://sourceware.org/pipermail/binutils/2021-May/116579.html https://sourceware.org/pipermail/binutils/2021-July/117492.html and I carefully ensured the patch author was on the To:/Cc: lines.
Perhaps I really should have kept more vigilance if I knew this going to be used in systemd, or, all Fedora packages?
I just raised https://sourceware.org/pipermail/binutils/2022-January/119537.html to question the error-prone READONLY
)

As I have already shown, it is necessary. The note is read/write otherwise:

[ 3] .note.package NOTE 00000000000002e8 000002e8

0000000000000030  0000000000000000  WA       0     0     4

and that breaks processing the note from core files. It needs to be read/only, like the build-id, and hence the required change in bfd to make it possible to do so.

bluca updated this revision to Diff 404286.Jan 29 2022, 10:01 AM

v2: removed SHF_ALLOC fixup, as it is fixed by https://reviews.llvm.org/D118530

MaskRay added a comment.EditedJan 29 2022, 11:10 AM

Non-SHF_ALLOC SHT_NOTE sections exists in the wild.

I have created two patches to improve the propagating SHF_ALLOC situation.

Thank you, I have tested them and can confirm this fixes the SHF_ALLOC problem. Updated this patch to remove the second part.


For the section type, I have filed a feature request ld: Customize output section type.
If an arbitrary output section type is too difficult, supporting just SHT_NOTE seems good enough.

This would break backward compatibility in bfd, as suddenly linker scripts that produced SHT_NOTE sections would no longer do so without the option, so I don't think it is a viable option. And having it only for lld means there's source-level incompatibility with bfd, so that doesn't look like a viable option either.

I did not suggest that GNU ld dropped the special section notation.
I just suggest that new sections should explicitly specify the type and avoid magic names.
This can be used by more types other than SHT_NOTE.

We have considered compiled objects and discarded that option as unfeasible. It's an absolute nightmare to even get a simple linker script to work reliably across 30.000 packages with a huge variation of different build systems and setups, across half a dozen different architectures. Having to also take care of an additional compilation steps means the whole endeavor will simply fail due to complications, so it is not an option.

https://fedoraproject.org/wiki/Changes/Package_information_on_ELF_objects references https://github.com/systemd/package-notes/blob/main/hello.spec .
Can't we replace -Wl,-dT,$PWD/notes.ld} with an compiled .o?
Then it will work all linkers: GNU ld, gold, ld.lld, and mold, perhaps also the abandoned MCLinker.

No, I already explained in the sentence you quoted, compiled objecs are not a realistic option.

I do not see why it is a complication.
The linker will add the section contributed by the relocatable object file to an appropriate place.
What problems did you find?
Does .note.package need to follow .note.gnu.build-id?

(I think communication can usually be improved by informing the involved parties early.

An un-involved party may feel frustration, especially if you only started conversation with one camp and then essentially force the other camp to accept.)

I'm sorry if you feel frustrated, but I cannot change the past. Also, I assure you there was no conversation with the bfd 'camp' on this topic, so there's no 'favoritism' - I don't think I even mentioned this specification when I sent the READONLY patch, as it was completely orthogonal. Please understand that this is a complex effort with a huge number of moving parts (that has been discussed many times in many public places where anybody could chime in, including tech press articles and conferences), and the critical and really complicated pieces are elsewhere in the stack.

(I did not want to make this review a blame place, but I have mentioned READONLY in two messages https://sourceware.org/pipermail/binutils/2021-May/116579.html https://sourceware.org/pipermail/binutils/2021-July/117492.html and I carefully ensured the patch author was on the To:/Cc: lines.
Perhaps I really should have kept more vigilance if I knew this going to be used in systemd, or, all Fedora packages?
I just raised https://sourceware.org/pipermail/binutils/2022-January/119537.html to question the error-prone READONLY
)

As I have already shown, it is necessary. The note is read/write otherwise:

[ 3] .note.package NOTE 00000000000002e8 000002e8

0000000000000030  0000000000000000  WA       0     0     4

and that breaks processing the note from core files. It needs to be read/only, like the build-id, and hence the required change in bfd to make it possible to do so.

As I explained in https://sourceware.org/pipermail/binutils/2022-January/119543.html ,
SHF_WRITE SHT_NOTE more likely suggests an unknown GNU ld bug (also ld.lld's when INSERT BEFORE is used).
I cannot reproduce what you may have seen.
The unknown bug (if exists) should be fixed instead of introducing READONLY.

No, I already explained in the sentence you quoted, compiled objecs are not a realistic option.

I do not see why it is a complication.

If we couldn't use a linker script, we could probably try to make a compiled object work.
But a linker script is certainly a lot easier. In particular, we don't know if we have any particular
compiler installed (a package might pull in one of the C or C++ compilers, or ocaml/fortran/ada/…),
and we don't have a good place to do compilation. It's one thing to write a hundred bytes of text for
the linker note, and quite another to generate some sources and then then call a compiler on that.

I guess that if we can't make the linker script work, we could fall back to a compiled object for lld
and/or gold. But it's require a lot more scaffolding.

Does .note.package need to follow .note.gnu.build-id?

There is is a benefit in having things in the *some* predictable order.
But no, in general the order shouldn't matter.

MaskRay added a comment.EditedFeb 7 2022, 11:51 AM

No, I already explained in the sentence you quoted, compiled objecs are not a realistic option.

I do not see why it is a complication.

If we couldn't use a linker script, we could probably try to make a compiled object work.
But a linker script is certainly a lot easier. In particular, we don't know if we have any particular
compiler installed (a package might pull in one of the C or C++ compilers, or ocaml/fortran/ada/…),
and we don't have a good place to do compilation. It's one thing to write a hundred bytes of text for
the linker note, and quite another to generate some sources and then then call a compiler on that.

I guess that if we can't make the linker script work, we could fall back to a compiled object for lld
and/or gold. But it's require a lot more scaffolding.

Thanks (assuming you are another Fedora developer) for weighing in.
A compiled object will help gold, ld.lld, and mold.
It only needs an assembler (GNU assembler, shipped as part of binutils). A compiler is not needed.

If a linker script is still needed in some circumstances, perhaps you may contact Nick Clifton to add this TYPE= syntax to binutils 2.38? https://sourceware.org/pipermail/binutils/2022-February/119600.html
Once the syntax is finalized, I can continue on D118840.
If this is necessary for Fedora, I'll port TYPE= to release/14.x branch.

Does .note.package need to follow .note.gnu.build-id?

There is is a benefit in having things in the *some* predictable order.
But no, in general the order shouldn't matter.

bluca abandoned this revision.Feb 17 2022, 5:01 AM
thakis added a subscriber: thakis.May 24 2022, 6:37 AM

(there was similar feedback on the corresponding mold bug: https://github.com/rui314/mold/issues/505)

Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2022, 6:37 AM
Herald added a subscriber: StephenFan. · View Herald Transcript

(there was similar feedback on the corresponding mold bug: https://github.com/rui314/mold/issues/505)

(there was similar feedback on the corresponding binutils patch: https://sourceware.org/pipermail/binutils/2022-May/120846.html https://sourceware.org/pipermail/binutils/2022-May/120985.html)