[PATCH] D118490: [lld][ELF] add .note sections from linker scripts as SHT_NOTE

Fri Jan 28 15:59:48 PST 2022

bluca added a comment.

In D118490#3281013 <https://reviews.llvm.org/D118490#3281013>, @MaskRay wrote:

>> 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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118490/new/

https://reviews.llvm.org/D118490