This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Support (TYPE=<value>) to customize the output section type
ClosedPublic

Authored by MaskRay on Feb 2 2022, 12:47 PM.

Details

Summary

The current output section type allows to set the ELF section type to
SHT_PROGBITS or SHT_NOLOAD. This patch allows an arbitrary section value
to be specified. Some common SHT_* literal names are supported as well.

SECTIONS {
  note (TYPE=SHT_NOTE) : { BYTE(8) *(note) }
  init_array ( TYPE=14 ) : { QUAD(14) }
  fini_array (TYPE = SHT_FINI_ARRAY) : { QUAD(15) }
}

When sh_type is specified, it is an error if an input section has a different type.

Our syntax is compatible with GNU ld 2.39 (https://sourceware.org/bugzilla/show_bug.cgi?id=28841).

Diff Detail

Event Timeline

MaskRay created this revision.Feb 2 2022, 12:47 PM
MaskRay requested review of this revision.Feb 2 2022, 12:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 2 2022, 12:47 PM

Not had a chance to look through the code, this is based on the description and the test so my apologies if the answers are in the code.

Initial thoughts are that this could be quite useful for creating more specific types from more general ones like raw data, although in the majority of cases input section type != output section type is likely a mistake. I'm guessing the interesting part will be dealing with the error and edge cases. For example:

  • Do the input sections have to be compatible with the output section type?
  • If they are not compatible, which takes precedent the input or output section type?
  • Are there, for want of a better word, coercions from one type to another, for example can SHT_NOBITS input sections be coerced into SHT_PROGBITS so that zeroes are written into the file.

I note in the example that all of your examples are of the form of linker script data directives BYTE, QUAD etc. Would it make sense to restrict this to OutputSections with no InputSections or at least non executable input sections?

MaskRay updated this revision to Diff 406657.Feb 7 2022, 5:18 PM
MaskRay edited the summary of this revision. (Show Details)

I do intend to report a type mismatch error.
The previous diff had an issue that this is not detected.
The uploaded one has fixed it.

So I pick this choice:

Do the input sections have to be compatible with the output section type?

I note in the example that all of your examples are of the form of linker script data directives BYTE, QUAD etc. Would it make sense to restrict this to OutputSections with no InputSections or at least non executable input sections?

I think .init_array (TYPE=SHT_INIT_ARRAY) : { *(.init_array) } should be supported.
In addition, our orphan placement rule may place input sections of the same name into the output section.
It seems odd to reject them.

Thanks for the update. The intent is clearer now. I've left some small suggestions.

lld/ELF/ScriptParser.cpp
792

I'm interested in why SHT_STRTAB that is usually only accessed from linker generated symbol tables. While in theory it could be constructed by a user created section, what symbol table would reference it with the sh_link field.

796

Worth undefining ECase after its last use?

797–798

Comment will need updating to include (TYPE)

823

It will be worth checking that there is a SHT_ prefix here so we can give a better error message if someone has used an unsupported type like SHT_DYNAMIC.

lld/docs/ELF/linker_script.rst
109

Suggest `if an input section in *S* has a different type.

lld/test/ELF/linkerscript/custom-section-type.s
63

For SHT_INIT_ARRAY et-al to be practically useful I guess we're looking at something like

.init_array { QUAD(function_name) }

As the address of the function to execute won't usually be known when writing the script. Is it worth making the test reflect that?

MaskRay updated this revision to Diff 407398.Feb 9 2022, 10:57 PM
MaskRay marked 6 inline comments as done.

Thanks for the comments. Updated.

MaskRay updated this revision to Diff 407399.Feb 9 2022, 11:00 PM

update test

lld/ELF/ScriptParser.cpp
792

Nick suggested SHT_STRTAB in https://sourceware.org/pipermail/binutils/2022-February/119594.html

I agree that SHT_STRTAB is not easily usable, so I have dropped it in the diff. We can add it when the need comes.

One more section type SHT_GROUP that I think we should disallow, although if GNU ld would prefer to include that we should prefer compatibility.

I'm not sure what the best policy is on setting approval as we've got a dependency on binutils. I'm happy with the implementation. Perhaps it would be useful to commit with an [ALPHA] tag, with explanation, to the feature? That way we reserve the right to make small modifications to the interface to maintain consistency with GNU ld.

lld/ELF/ScriptParser.cpp
794

I think SHT_GROUP may also be too difficult to use without further extensions to linker scripts. It would only be useful in ld -r links as groups don't mean anything in ELF executables and shared libraries. To effectively use them from a linker script we'd need something like SH_IDX("output section name") which the linker would fill in the sh_idx in the output ELF file. I don't think a user could be expected to hard code a number in the linker script.

Apologies I didn't spot this one on the first go.

MaskRay updated this revision to Diff 408468.Feb 14 2022, 9:48 AM

Remove SHT_GROUP.

Thanks for the comments.
The urgence of this syntax is dependent on how much systemd/Fedora abuses the .note* ==> SHT_NOTE behavior
https://bugzilla.redhat.com/show_bug.cgi?id=2052801
Right now now too urgent to me.
We can give binutils more time and I'll keep pinging the binutils discussion.
If for some reason we need to push this earlier, I'll consider adding "experimental".

MaskRay added a comment.EditedFeb 16 2022, 6:18 PM

GNU ld added the (TYPE=...) syntax today: https://sourceware.org/pipermail/binutils/2022-February/119789.html
... but it additionally added the awkward .ro.note (READONLY (TYPE=SHT_NOTE)) : { LONG(5678); }.

I think READONLY is an ill advice and we should not follow.

This patch is compatible with GNU ld supported syntax and I think we can move forward.

peter.smith accepted this revision.Feb 17 2022, 2:16 AM

LGTM. At least if we start off without READONLY then we can put it in later if it is needed.

This revision is now accepted and ready to land.Feb 17 2022, 2:16 AM
MaskRay edited the summary of this revision. (Show Details)Feb 17 2022, 11:57 AM
MaskRay edited the summary of this revision. (Show Details)
MaskRay updated this revision to Diff 409743.Feb 17 2022, 12:04 PM
MaskRay marked an inline comment as done.

Update doc.
Actually remove SHT_GROUP support from code but keep the test

This revision was landed with ongoing or failed builds.Feb 17 2022, 12:11 PM
This revision was automatically updated to reflect the committed changes.

I have not done a bisect but it seems likely that this change breaks linking modules for the arm64 Linux kernel, is this expected?

$ make -skj"$(nproc)" ARCH=arm64 LLVM=1 mrproper defconfig all
...
ld.lld: error: section type mismatch for .plt
>>> <internal>:(.plt): SHT_PROGBITS
>>> output section .plt: SHT_NOBITS
...

I believe the .plt handling in the linker script comes from https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/include/asm/module.lds.h?h=v5.17-rc4.

MaskRay added a comment.EditedFeb 17 2022, 5:26 PM

I have not done a bisect but it seems likely that this change breaks linking modules for the arm64 Linux kernel, is this expected?

$ make -skj"$(nproc)" ARCH=arm64 LLVM=1 mrproper defconfig all
...
ld.lld: error: section type mismatch for .plt
>>> <internal>:(.plt): SHT_PROGBITS
>>> output section .plt: SHT_NOBITS
...

I believe the .plt handling in the linker script comes from https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/include/asm/module.lds.h?h=v5.17-rc4.

I do not know whether we want to remove the error for NOLOAD.

.plt 0 (NOLOAD) : { BYTE(0) } looks questionable to me.

Seems that it needs /DISCARD/ : { *(.plt) } if it just wants to leave a one-byte .plt but ignoring the regular .plt.

Alternatively/preferably, drop (NOLOAD). @ardb for kernel fd045f6cd98ec4953147b318418bd45e441e52a3

edit: sent https://lore.kernel.org/linux-arm-kernel/20220218081209.354383-1-maskray@google.com/T/#u
sent one for riscv as well.

MaskRay added a subscriber: ardb.Feb 17 2022, 5:30 PM
ardb added a comment.Feb 18 2022, 1:14 AM

I have not done a bisect but it seems likely that this change breaks linking modules for the arm64 Linux kernel, is this expected?

$ make -skj"$(nproc)" ARCH=arm64 LLVM=1 mrproper defconfig all
...
ld.lld: error: section type mismatch for .plt
>>> <internal>:(.plt): SHT_PROGBITS
>>> output section .plt: SHT_NOBITS
...

I believe the .plt handling in the linker script comes from https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/include/asm/module.lds.h?h=v5.17-rc4.

I do not know whether we want to remove the error for NOLOAD.

.plt 0 (NOLOAD) : { BYTE(0) } looks questionable to me.

Seems that it needs /DISCARD/ : { *(.plt) } if it just wants to leave a one-byte .plt but ignoring the regular .plt.

Alternatively/preferably, drop (NOLOAD). @ardb for kernel fd045f6cd98ec4953147b318418bd45e441e52a3

edit: sent https://lore.kernel.org/linux-arm-kernel/20220218081209.354383-1-maskray@google.com/T/#u
sent one for riscv as well.

These .plt sections have nothing to do with linker emitted PLTs, so perhaps the name should be improved.

These sections are instantiated by the module loader at runtime, but in order for the module loader to be able to create a section of arbitrary size, it needs to be present in the object file to begin with. This is why the dummy byte is inserted, as otherwise, the section is dropped.

The purpose of these sections is to carry veneers that may or may not be needed to fix up out-of-range direct branches from the module code into the kernel itself or other modules. Whether such veneers are necessary depends on the actual load address of the module. (Veneers are also emitted to work around the Cortex-A53 ADRP erratum #843419, if needed)

If the section name '.plt' is special in some way, I'd be happy to prepare a kernel patch that renames these into something more suitable.

nathanchance added a comment.EditedFeb 18 2022, 7:38 AM

If the section name '.plt' is special in some way, I'd be happy to prepare a kernel patch that renames these into something more suitable.

Would this suggested change replace Fangrui's existing patch?

Regardless, @MaskRay, would you consider dropping this error for the time being until the kernel can be patched? At minimum, it is going to take a few weeks for the patch to land in Linus's tree and get backported to stable and I would like to avoid carrying any change in our continuous integration, as it adds additional maintenance overhead. Not to mention we are not the only continuous integration setup that tests with tip of tree LLVM so this breakage is going to be noticed by multiple parties, who won't have the ability to apply patches.

If the section name '.plt' is special in some way, I'd be happy to prepare a kernel patch that renames these into something more suitable.

Would this suggested change replace Fangrui's existing patch?

Regardless, @MaskRay, would you consider dropping this error for the time being until the kernel can be patched? At minimum, it is going to take a few weeks for the patch to land in Linus's tree and get backported to stable and I would like to avoid carrying any change in our continuous integration, as it adds additional maintenance overhead. Not to mention we are not the only continuous integration setup that tests with tip of tree LLVM so this breakage is going to be noticed by multiple parties, who won't have the ability to apply patches.

Downgraded (NOLOAD) error to warning with https://reviews.llvm.org/rGcb0a4bb5be10636aaec3ecb56ed586dee3eb0b9e

https://github.com/ClangBuiltLinux/linux/issues/1597 has more discussions.

This change is breaking rust embedded use cases where NOLOAD in a linker script is no longer being honored. So we had to revert it (locally) in ChromeOS.

example:

#[link_section = ".arena"]
static mut IMAGE_DATA: [i8; NUM_PIXELS] = [0i8; NUM_PIXELS];

Linker script :

PROVIDE( end = . );
      SECTIONS {
          .arena (NOLOAD) :
          {
                  _farena = .;
                  *(.arena)
                  _earena = .;
          } > arena
      }

With this commit where NOBITS is changed to PROGBITS.

Before:
  [ 8] .arena            NOBITS          60000000 006000 012c00 00  WA  0   0  1
After:
  [ 8] .arena            PROGBITS        60000000 006000 012c00 00  WA  0   0  1

This seems to break a common use case in rust embedded
e.g. https://github.com/rust-embedded/cortex-m/commit/94fbbe01189f97aefa8d6a97cc7cd73e5c6f6ec9

Rust currently do not have a specify to section type and I am not sure llvm even provides an API for that.

@peter.smith wdyt? Should this be reverted for now since it breaks rust embedded without any short term alternatives.
Fangrui currently is out so we prefer to not revert outright before consulting you.

Herald added a project: Restricted Project. · View Herald TranscriptMay 5 2022, 2:30 PM
Herald added a subscriber: StephenFan. · View Herald Transcript
MaskRay added a comment.EditedMay 5 2022, 7:23 PM

This change is breaking rust embedded use cases where NOLOAD in a linker script is no longer being honored. So we had to revert it (locally) in ChromeOS.

example:

#[link_section = ".arena"]
static mut IMAGE_DATA: [i8; NUM_PIXELS] = [0i8; NUM_PIXELS];

Linker script :

PROVIDE( end = . );
      SECTIONS {
          .arena (NOLOAD) :
          {
                  _farena = .;
                  *(.arena)
                  _earena = .;
          } > arena
      }

With this commit where NOBITS is changed to PROGBITS.

Before:
  [ 8] .arena            NOBITS          60000000 006000 012c00 00  WA  0   0  1
After:
  [ 8] .arena            PROGBITS        60000000 006000 012c00 00  WA  0   0  1

This seems to break a common use case in rust embedded
e.g. https://github.com/rust-embedded/cortex-m/commit/94fbbe01189f97aefa8d6a97cc7cd73e5c6f6ec9

Rust currently do not have a specify to section type and I am not sure llvm even provides an API for that.

@peter.smith wdyt? Should this be reverted for now since it breaks rust embedded without any short term alternatives.
Fangrui currently is out so we prefer to not revert outright before consulting you.

This is an issue with the Rust linker script and I don't think there is sufficient justification to revert the patch in the upstream. (Reverting Chrome OS local one is certainly fine).
Think of another case that an input .arena has type SHT_PROGBITS but with non-all-zero content, the output type being SHT_NOBITS will be broken without any diagnostic.

In the current state, ld.lld actually emits a warning:

% ld.lld @response.txt 
ld.lld: warning: section type mismatch for .arena
>>> path/to/fpga_rom-2c6c7738d58296f2.fpga_rom.52cb7a2a-cgu.0.rcgu.o:(.arena): SHT_PROGBITS
>>> output section .arena: SHT_NOBITS

Whether it is necessary to have support SHT_NOBITS output with SHT_PROGBITS input without any diagnostic? I think there is no sufficient evidence this is a common request.

To be fair, there are more problems with the linker script, including the superfluous parens surrounding input section descriptions { ... (*(.trap)) (*(.trap.rust)) }.
This does not work with GNU ld but currently is not detected by lld.

We can close the discussion as Fangrui has uploaded https://reviews.llvm.org/D125074 to keep older behavior.