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

Fri Jan 28 15:21:45 PST 2022

MaskRay added a comment.

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118490