Page MenuHomePhabricator

[MC][ELF] Make linked-to symbol name part of ELFSectionKey
ClosedPublic

Authored by MaskRay on Feb 4 2020, 4:01 PM.

Details

Summary

https://bugs.llvm.org/show_bug.cgi?id=44775

This rule has been implemented by GNU as https://sourceware.org/ml/binutils/2020-02/msg00028.html (binutils >= 2.35)

It allows us to simplify

.section .foo,"o",foo,unique,0
.section .foo,"o",bar,unique,1  # different section

as:

.section .foo,"o",foo
.section .foo,"o",bar  # different section

We consider the two .foo different even if the linked-to symbols foo and bar
are defined in the same section. This is a deliberate choice so that we don't
need to know the section where foo and bar are defined beforehand.

Diff Detail

Unit TestsFailed

TimeTest
40 msLLVM.CodeGen/AArch64::patchable-function-entry-bti.ll
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/llc -mtriple=aarch64 /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/llvm/test/CodeGen/AArch64/patchable-function-entry-bti.ll -o - | /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/FileCheck --check-prefixes=CHECK /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/llvm/test/CodeGen/AArch64/patchable-function-entry-bti.ll
50 msLLVM.CodeGen/AArch64::patchable-function-entry.ll
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/llc -mtriple=aarch64 /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/llvm/test/CodeGen/AArch64/patchable-function-entry.ll -o - | /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/FileCheck --check-prefixes=CHECK,NOFSECT /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/llvm/test/CodeGen/AArch64/patchable-function-entry.ll
40 msLLVM.CodeGen/X86::patchable-function-entry-ibt.ll
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/llc -mtriple=i686 /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/llvm/test/CodeGen/X86/patchable-function-entry-ibt.ll -o - | /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/FileCheck --check-prefixes=CHECK,32 /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/llvm/test/CodeGen/X86/patchable-function-entry-ibt.ll
30 msLLVM.CodeGen/X86::patchable-function-entry.ll
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/llc -mtriple=i386 /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/llvm/test/CodeGen/X86/patchable-function-entry.ll -o - | /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/FileCheck --check-prefixes=CHECK,NOFSECT,32 /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/llvm/test/CodeGen/X86/patchable-function-entry.ll

Event Timeline

MaskRay created this revision.Feb 4 2020, 4:01 PM
Herald added a project: Restricted Project. · View Herald Transcript

Unit tests: unknown.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

MaskRay marked an inline comment as done.Feb 4 2020, 4:12 PM
MaskRay added inline comments.
llvm/lib/MC/MCContext.cpp
322–323

As a note: this is only used by renaming .debug_* to .zdebug_*, which don't do SHF_LINK_ORDER magic. So using an empty linked-to symbol name will be fine.

As I understand it, .zdebug* are deprecated. ELF toolchain moves to SHF_COMPRESSED, so we don't need to add more support to .zdebug*.

It looks reasonable to me. Two minor nits are below, the rest looks probably fine.
Lets see what other people think.

llvm/include/llvm/MC/MCContext.h
217

Perhaps just:

if (UniqueID  != Other.UniqueID)
  return UniqueID < Other.UniqueID;
return LinkedToName < Other.LinkedToName;
llvm/lib/MC/MCContext.cpp
322–323

It deserves a comment in the code I think.

MaskRay updated this revision to Diff 242522.Feb 5 2020, 12:42 AM
MaskRay marked 2 inline comments as done.

Add a comment

llvm/include/llvm/MC/MCContext.h
217

The order is so that it matches the field order. Name fields placing together looks better, so I want to stick with the current order.

I can do a follow-up change to use StringRef::compare for SectionName and GroupName.

grimar added inline comments.Feb 5 2020, 12:52 AM
llvm/include/llvm/MC/MCContext.h
217

Up to you. Not a big problem anyways.

Unit tests: unknown.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

I think that this patch removes the need for uniquing sections for symbols with associated symbols which are explicitly assigned to a section name e.g. via attribute((section name)).

I have updated the comment in https://reviews.llvm.org/D72194 to say that. It would be a nice follow up change to remove the UniqueID added to sections for symbols with COMDATs/associated symbols that are explicitly assigned a section name.

llvm/include/llvm/MC/MCContext.h
199

Here start using "linked_to" terminology instead of "associated". Should we stick to just one?

This is a deliberate choice so that we don't need to know the section where foo and bar are defined beforehand.

Just want to make sure this is well defined with respect to the description in ELF.

SHF_LINK_ORDER
     This flag adds special ordering requirements for link editors. The requirements apply if the sh_link field of this section's header references another section (the linked-to section). If this section is combined with other sections in the output file, it must appear in the same relative order with respect to those sections, as the linked-to section appears with respect to sections the linked-to section is combined with.

A typical use of this flag is to build a table that references text or data sections in address order.

If we have two sections S1, S2, with a SHF_LINK_ORDER on the same section D, what is their ordering relationship between S1 and S2? If foo and bar are in the same section and we don't create two sections then we don't get this problem as we'd only have one S with a sh_link to D. I think that if the author is trying to use SHF_LINK_ORDER to build a table that references text or data sections in address order then they may get surprised if S1 and S2 are not in the order of foo and bar. True that if one section where generated they'd have to ensure that the contents of S where in the same relative order to foo and bar in D but at least they would know the linker couldn't alter it.

MaskRay marked an inline comment as done.Feb 5 2020, 9:50 AM
MaskRay added inline comments.
llvm/include/llvm/MC/MCContext.h
199

"linked-to section" is used by the ELF spec. By analogy, I use "linked-to symbol" here (omitted "Sym" in "LinkedToName"). "linked-to" implies a directed edge and makes it clear its relation with "sh_link", while one can argue that "associated" means an undirected edge. I think we should rename MCSymbol related "Associated" to "LinkedToSym". The IR level "!associated" can be left unchanged. I can do that in a separate change.

This is a deliberate choice so that we don't need to know the section where foo and bar are defined beforehand.

Just want to make sure this is well defined with respect to the description in ELF.

SHF_LINK_ORDER
     This flag adds special ordering requirements for link editors. The requirements apply if the sh_link field of this section's header references another section (the linked-to section). If this section is combined with other sections in the output file, it must appear in the same relative order with respect to those sections, as the linked-to section appears with respect to sections the linked-to section is combined with.

A typical use of this flag is to build a table that references text or data sections in address order.

If we have two sections S1, S2, with a SHF_LINK_ORDER on the same section D, what is their ordering relationship between S1 and S2? If foo and bar are in the same section and we don't create two sections then we don't get this problem as we'd only have one S with a sh_link to D. I think that if the author is trying to use SHF_LINK_ORDER to build a table that references text or data sections in address order then they may get surprised if S1 and S2 are not in the order of foo and bar. True that if one section where generated they'd have to ensure that the contents of S where in the same relative order to foo and bar in D but at least they would know the linker couldn't alter it.

"Two sections with the same sh_link" this scenario is probably not clearly specified. I will expect a stable sort by sh_link. For the following part of the added test, after linking,

.section .foo,"o",@progbits,foo  # first in the output section .foo due to stable sort
.byte 0

.section .foo,"o",@progbits,bar  # second
.byte 1

This is what lld currently does. It will be nice to get a sign-up from GNU ld. An alternative choice is that users should not rely on the order. I'm inclined to define an order here.

MaskRay added a comment.EditedFeb 5 2020, 10:07 AM

Another thing is whether we should use sh_flags/sh_type to differentiate sections. In addition, whether we should use sh_entsize.
If yes, the implementation can be a bit clumsy. We may have to add many Elf*_Shdr fields to ELFSectionKey.
If no (GNU as behavior), we should add warnings. D73999 will do that.

The section group name is part of the key in both MC and GNU as. By analogy, SHF_LINK_ORDER+sh_link should probably be part of the key.

Field not specified by the .section directive (sh_addrline,sh_addr,sh_offset,sh_info) should definitely not be part of the key.

I have a slight preference for "no", though I don't currently have a particular good reason why "sh_entsize" should not be part of a key. Maybe we are fine with current .rodata.cst16 naming.

.section .foo,"o",@progbits,foo  # first in the output section .foo due to stable sort
.byte 0

.section .foo,"o",@progbits,bar  # second
.byte 1

This is what lld currently does. It will be nice to get a sign-up from GNU ld. An alternative choice is that users should not rely on the order. I'm inclined to define an order here.

I don't believe that assembler nor linker should guarantee any orders here.

Another thing is whether we should use sh_flags/sh_type to differentiate sections. In addition, whether we should use sh_entsize.
If yes, the implementation can be a bit clumsy. We may have to add many Elf*_Shdr fields to ELFSectionKey.
If no (GNU as behavior), we should add warnings. D73999 will do that.

The section group name is part of the key in both MC and GNU as. By analogy, SHF_LINK_ORDER+sh_link should probably be part of the key.

Field not specified by the .section directive (sh_addrline,sh_addr,sh_offset,sh_info) should definitely not be part of the key.

I have a slight preference for "no", though I don't currently have a particular good reason why "sh_entsize" should not be part of a key. Maybe we are fine with current .rodata.cst16 naming.

An alternative design to this patch is for the assembler to *never* create more than one section with a given name (apart from for COMDATs because that is established behaviour we can't change) unless the ,unique, N, extension is used. However, the assembler can give warnings as in D73999. This is really simple for users to understand - the rule is you have to choose a unique name for your sections.

llvm/include/llvm/MC/MCContext.h
199

SGTM.

MaskRay marked 2 inline comments as done.Feb 5 2020, 11:54 AM
MaskRay added inline comments.
llvm/include/llvm/MC/MCContext.h
199

Created D74082 to rename Associated to LinkedToSym

MaskRay updated this revision to Diff 243453.Feb 9 2020, 10:08 AM
MaskRay marked an inline comment as done.

Rebase after D74082 was committed

My current thoughts regarding (.section directives with the same name but different fields)
are similar to binutils' (eventual) state:

  • sh_flags or sh_type: warn
  • sh_link/group name: no warning. produce separate sections
  • sh_entsize due to SHF_MERGE: still controversial. I am slightly inclined to require ,unique
MaskRay added a subscriber: psmith.Feb 13 2020, 9:13 PM

Address review comments.

I tend to agree with Alan Modra's arguement in
https://sourceware.org/ml/binutils/2020-02/msg00093.html

I don't think so. User assembly often gets section attributes wrong
or leaves them off entirely for special sections known to the
assembler. ie. the first .section .foo above is a built-in rather
than user input.

Add more folks for feedback.

Please see my binutils post. For a .section directive with the same name but a different field. If the field is:

  • sh_flags or sh_type: warn
  • sh_link due to SHF_LINK_ORDER: no warning. produce separate sections
  • sh_entsize due to SHF_MERGE: still controversial

I agree with warning for sh_flags, I think there is too much legacy code out there that would behave differently.
I agree with sh_link producing separate sections. In an ideal world no-one writes this by hand in assembly.
For sh_entsize SHF_MERGE, this is informing the linker that it can produce an optimisation, which it is permitted to ignore. I tend towards an error message if it is implementable as it is a strong message to fix the assembler. The next best thing is a warning then clearing SHF_MERGE and setting sh_entsize to 0. I don't think a warning on its own is safe.

Does this patch look good? The consensus is that we should make sh_link producing separate sections.

MaskRay updated this revision to Diff 244569.Feb 13 2020, 9:38 PM

Update 4 __patchable_function_entries tests.

The case no-function-sections is very similar to function-sections case now.
A future clean-up can delete ,unique from the tests but that will require some interface changes.

MaskRay edited the summary of this revision. (Show Details)Feb 13 2020, 9:42 PM

I'm happy with this patch, I think it is a fair reflection of the consensus, I'll let others approve when they have had a chance to read through in the US timezone.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 14 2020, 8:07 PM
This revision was automatically updated to reflect the committed changes.

It seems like there's some bad interaction between this, ARM exception handling, and asm parsing. I'm seeing link errors like error: EXIDX sections .ARM.exidx(10) and .ARM.exidx(4) both link to text section.text(2) if I build with "-via-file-asm", and link with binutils ld. Reverting this patch seems to fix the issue.

You can produce an object file with multiple .ARM.exidx sections using something like echo "void h();void f(){}void g(){}" | clang -x c - -o armeh.o -c -via-file-asm --target=arm-eabi

I suspect that ARMELFStreamer::SwitchToEHSection is somehow triggering this code, but I'm not sure I follow how this is supposed to work. And I'm not sure why -via-file-asm is relevant.

MaskRay added a comment.EditedFeb 24 2020, 5:28 PM

It seems like there's some bad interaction between this, ARM exception handling, and asm parsing. I'm seeing link errors like error: EXIDX sections .ARM.exidx(10) and .ARM.exidx(4) both link to text section.text(2) if I build with "-via-file-asm", and link with binutils ld. Reverting this patch seems to fix the issue.

You can produce an object file with multiple .ARM.exidx sections using something like echo "void h();void f(){}void g(){}" | clang -x c - -o armeh.o -c -via-file-asm --target=arm-eabi

I suspect that ARMELFStreamer::SwitchToEHSection is somehow triggering this code, but I'm not sure I follow how this is supposed to work. And I'm not sure why -via-file-asm is relevant.

It seems that .ARM.exidx should be special cased.

edit: clang test/MC/ARM/eh-directive-section-multiple-func.s -o a.o -c --target=arm-eabi; readelf -S a.o a.o has multiple .ARM.exidx sections.

llvm-mc runs Ctx.setUseNamesOnTempLabels(false);, so FnStart will have an empty name. Multiple .ARM.exidx are considered the same.

In clang cc1as, UseNamesOnTempLabels is true. FnStart will get different names for different functions. Multiple .ARM.exidx are considered different.

Will send a fix soon.

@efriedma Either D75095 or D75097 can fix the .ARM.exidx issue. I think both make sense. Unfortunately neither can be easily tested :/

@MaskRay , hi. This patch miscompiles 471.omnetpp and 453.povray on arm-linux-gnueabihf. Would you please investigate?

Let me know if you need help reproducing the problem.

@MaskRay , hi. This patch miscompiles 471.omnetpp and 453.povray on arm-linux-gnueabihf. Would you please investigate?

Let me know if you need help reproducing the problem.

I just cloned llvm-test-suite. Do you mean the following files? I have no experience configuring SPEC. Can you give more detailed instructions?

% fd -u omnetpp
External/SPEC/CINT2006/471.omnetpp
External/SPEC/CINT2006/471.omnetpp/471.omnetpp.reference_output
External/SPEC/CINT2017rate/520.omnetpp_r
External/SPEC/CINT2017speed/620.omnetpp_s
LNTBased/speccpu2006/int/471.omnetpp

lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp had one bug which was fixed now: D75095.

Are you using the latest llvm-project?

Cross-compiling the LLVM testsuite is a bit painful; I wouldn't recommend trying to set it up unless you really need it. But see http://llvm.org/docs/TestSuiteGuide.html describes SPEC and cross-compiling, if you really want to,

My team's internal buildbot indicates SPEC2006 is passing for arm-linux-gnueabi. So I think more details are necessary. What compile flags are you using? Are you using "test" or "ref" input? Is there a compiler error, or is it failing at runtime?

It's runtime failure and occurs on "ref" input. Compilation flags are
-O3 -marm --target=armv7a-linux-gnueabihf --sysroot=/path/to/build/sysroots/arm-linux-gnueabihf
and linking flags are (note that I'm linking with LLD).
-Wl,--build-id -Wl,-dynamic-linker=/path/to/run/sysroot/lib/ld-2.31.9000.so -Wl,-rpath=/path/to/run/sysroot/lib -Wl,-rpath=/path/to/run/sysroot/usr/lib -O3 -marm --target=armv7a-linux-gnueabihf --sysroot=/path/to/build/sysroots/arm-linux-gnueabihf -fuse-ld=lld

Same failure occurs with -Os/-Oz with -mthumb.

I'll check whether linking with BFD linker avoids the problem.

Omnetpp fails with:
1583131428.18: 471.omnetpp: copy 0 non-zero return code (exit code=6, signal=0)
1583131428.18: ********
1583131428.18: Contents of omnetpp.err
1583131428.18: ********
1583131428.18: terminate called after throwing an instance of 'cTerminationException*'
1583131428.18: Aborted
1583131428.18: ********

and povray fails with
1583130534.05: 453.povray: copy 0 non-zero return code (exit code=6, signal=0)

I briefly made a couple attempts to reproduce SPEC2006 failures, but failed; all the tests pass for me, with both binutils ld and lld. My setup isn't hardfloat, but that seems unlikely to be relevant.

Are you using latest master? (In particular, more recent than D75095?)

It would be helpful if you could try to figure out what's actually different about the executables before/after this patch, using objdump. I mean, the failures are consistent with something going wrong generating the unwind info section, but it's not clear what specifically is going wrong.