Page MenuHomePhabricator

[AsmPrinter] Split up .gcc_except_table
ClosedPublic

Authored by MaskRay on Jul 12 2020, 10:02 PM.

Details

Summary

MC currently produces monolithic .gcc_except_table section. GCC can split up .gcc_except_table:

  • if comdat: .section .gcc_except_table._Z6comdatv,"aG",@progbits,_Z6comdatv,comdat
  • otherwise, if -ffunction-sections: .section .gcc_except_table._Z3fooi,"a",@progbits

This ensures that (a) non-prevailing copies are discarded and (b)
.gcc_except_table associated to discarded text sections can be discarded by a
.gcc_except_table-aware linker (GNU ld, but not gold or LLD)

This patches matches the GCC behavior. If -fno-unique-section-names is
specified, we don't append the suffix. If -ffunction-sections is additionally specified,
use .section ...,unique.

Note, if clang driver communicates that the linker is LLD and we know it
is new (11.0.0 or later) we can use SHF_LINK_ORDER to avoid string table
costs, at least in the -fno-unique-section-names case. We cannot use it on GNU
ld because as of binutils 2.35 it does not support mixed SHF_LINK_ORDER &
non-SHF_LINK_ORDER components in an output section
https://sourceware.org/bugzilla/show_bug.cgi?id=26256

For RISC-V -mrelax, this patch additionally fixes an assembler-linker
interaction problem: because a section is shrinkable, the length of a call-site
code range is not a constant. Relocations referencing the associated text
section (STT_SECTION) are needed. However, a STB_LOCAL relocation referencing a
discarded section group member from outside the group is disallowed by the ELF
specification (PR46675):

// a.cc
inline int comdat() { try { throw 1; } catch (int) { return 1; } return 0; }
int main() { return comdat(); }

// b.cc
inline int comdat() { try { throw 1; } catch (int) { return 1; } return 0; }
int foo() { return comdat(); }

clang++ -target riscv64-linux -c a.cc b.cc -fPIC -mno-relax
ld.lld -shared a.o b.o => ld.lld: error: relocation refers to a symbol in a discarded section:

-fbasic-block-sections= is similar to RISC-V -mrelax: there are outstanding relocations.

Diff Detail

Event Timeline

MaskRay created this revision.Jul 12 2020, 10:02 PM
MaskRay marked an inline comment as done.Jul 12 2020, 10:12 PM
MaskRay added inline comments.
llvm/lib/CodeGen/AsmPrinter/EHStreamer.cpp
471

Forgot to add comments. I'd write

// GNU as<2.35 does not support SHF_LINK_ORDER. If we are using the integrated assembler, use SHF_LINK_ORDER so that the linker can discard .gcc_except_table associated to a discarded text section.

Sounds reasonable in principle, but what is the compile/link-time performance cost of adding lots of extra section headers, especially when using -ffunction-sections on a link with many functions?

Sounds reasonable in principle, but what is the compile/link-time performance cost of adding lots of extra section headers, especially when using -ffunction-sections on a link with many functions?

Functions using try catch needs personality and .gcc_except_table. Such functions are not common and thus the increased number of section headers is not a concern.

What would happen if there was a single .text section containing multiple functions, would that produce multiple .gcc_except_table sections each with a SHF_LINK_ORDER dependency on the single .text section? I think that this is OK as there are no other sections that refer to .gcc_except_table and there doesn't need to be a total order. Ideally we could have one .gcc_except_table section per associated .text section.

What would happen if there was a single .text section containing multiple functions, would that produce multiple .gcc_except_table sections each with a SHF_LINK_ORDER dependency on the single .text section? I think that this is OK as there are no other sections that refer to .gcc_except_table and there doesn't need to be a total order. Ideally we could have one .gcc_except_table section per associated .text section.

For

.section .text.foo,"ax"
.globl foo, bar
foo: ret
bar: ret

.section .gcc_except_table,"ao",@progbits,foo; .quad 0
.section .gcc_except_table,"ao",@progbits,bar; .quad 0

There will be two .gcc_except_table with the same sh_link (llvm-mc & GNU as>=2.35). Linkers can handle the output correctly. .gcc_except_table doesn't have an order requirement.

MaskRay added a comment.EditedJul 15 2020, 10:30 PM

I failed to think about one problem. We can't use SHF_LINK_ORDER because mixing SHF_LINK_ORDER sections and non-SHF_LINK_ORDER sections are disallowed by linkers.

GNU ld: .gcc_except_table has both ordered [.gcc_except_table' in a.o] and unordered [.gcc_except_table' in b.o] sections
LLD: a.o:(.gcc_except_table): SHF_LINK_ORDER sections in .gcc_except_table are not contiguous I relaxed the restriction a bit (D77007) but non-contignuous SHF_LINK_ORDER sections are still not allowed.

It is a bit unfortunate that the original semantics (order requirement) of SHF_LINK_ORDER imposes an unnecessary restriction.

Created a generic-abi & binutils thread: https://groups.google.com/forum/#!topic/generic-abi/hgx_m1aXqUo


For .gcc_except_table, .gcc_except_table.suffix is cumbersome and wastes .strtab space. I want to avoid it. Unfortunately LLD cannot garbage collect .gcc_except_table even if .gcc_except_table is split up so .section .gcc_except_table,"a",@progbits,unique,1 (unique linkage) isn't ideal.

In LLD, --gc-sections happens before .eh_frame combination. During --gc-sections, LLD simply treats every .eh_frame live and thus the referenced .gcc_except_table will be kept live as well (the sole references to .gcc_except_table are from .eh_frame). If we could use SHF_LINK_ORDER, SHF_LINK_ORDER would make .gcc_except_table discardable.

(
Gold and GNU ld relax the "STB_LOCAL relocation referencing a discarded section group member from
outside the group is disallowed " rule by special casing .gcc_except_table (gold: Default_comdat_behavior; GNU ld: _bfd_elf_default_action_discarded))

I failed to think about one problem. We can't use SHF_LINK_ORDER because mixing SHF_LINK_ORDER sections and non-SHF_LINK_ORDER sections are disallowed by linkers.

GNU ld: .gcc_except_table has both ordered [.gcc_except_table' in a.o] and unordered [.gcc_except_table' in b.o] sections
LLD: a.o:(.gcc_except_table): SHF_LINK_ORDER sections in .gcc_except_table are not contiguous I relaxed the restriction a bit (D77007) but non-contignuous SHF_LINK_ORDER sections are still not allowed.

It is a bit unfortunate that the original semantics (order requirement) of SHF_LINK_ORDER imposes an unnecessary restriction.

Created a generic-abi & binutils thread: https://groups.google.com/forum/#!topic/generic-abi/hgx_m1aXqUo

I've just posted on there too, as I have a similar problem with fragmenting DWARF sections (which would require a mix of interleaved common and SHF_LINK_ORDER (but not actually ordered) sections. I think SHF_LINK_ORDER is maybe not the right tool for the job in this context, because of the ordering requirement as you mention.

MaskRay updated this revision to Diff 301736.Oct 29 2020, 1:49 PM
MaskRay edited the summary of this revision. (Show Details)

Play safe and just use the GCC approach.

@psmith @jhenderson @jrtc27 This is ready. I simply switched to the safe GCC approach.

+1,
Maybe worth mentioning in the description that the issue with -fbasic-block-sections is the revocations associated with @LPStart (Relocations are needed both in PIC and non-PIC mode). The no-basic-block-sections case is not affected since LPStart is omitted.

I wonder if it'd be nicer to push this logic into getLSDASection by passing an (optional?) Function given that's where the normal LSDASection gets created? Having the check for ELF here seems to indicate that the logic isn't quite in the right place and belongs in the MCOFI subclass.

llvm/lib/CodeGen/AsmPrinter/EHStreamer.cpp
466

Should this instead inherit the (base) flags from LSDASection? For CHERI we have to add SHF_WRITE (we make it a relro section) so it'd be nice to inherit that automatically here rather than needing to have the logic duplicated between here and initELFMCObjectFileInfo.

MaskRay updated this revision to Diff 302056.Oct 30 2020, 6:48 PM

Reuse flags
Check function sections

MaskRay marked an inline comment as done.Oct 30 2020, 6:49 PM
MaskRay added inline comments.
llvm/lib/CodeGen/AsmPrinter/EHStreamer.cpp
466

Changed to the base flags to make CHERI convenient.

I wonder if it'd be nicer to push this logic into getLSDASection by passing an (optional?) Function given that's where the normal LSDASection gets created? Having the check for ELF here seems to indicate that the logic isn't quite in the right place and belongs in the MCOFI subclass.

I agree with @jrtc27 here. How about we implement this like MCObjectFileInfo::getStackSizeSection(const MCSection&) or MCObjectFileInfo::getBBAddrMapSection(const MCSection&). The intended logic seems quite similar.

MaskRay marked an inline comment as done.Oct 30 2020, 10:42 PM

I wonder if it'd be nicer to push this logic into getLSDASection by passing an (optional?) Function given that's where the normal LSDASection gets created? Having the check for ELF here seems to indicate that the logic isn't quite in the right place and belongs in the MCOFI subclass.

I agree with @jrtc27 here. How about we implement this like MCObjectFileInfo::getStackSizeSection(const MCSection&) or MCObjectFileInfo::getBBAddrMapSection(const MCSection&). The intended logic seems quite similar.

It would be inconvenient.

  1. getStackSizeSection and getBBAddrMapSection do not use different section names. They will need to know the suffix
  2. MCObjectFileInfo does not know whether function sections is enabled

MCObjectFileInfo is a good place for getStackSizeSection and getBBAddrMapSection because they need just information from MCObjectFileInfo. However, it is different for .gcc_except_table

I wonder if it'd be nicer to push this logic into getLSDASection by passing an (optional?) Function given that's where the normal LSDASection gets created? Having the check for ELF here seems to indicate that the logic isn't quite in the right place and belongs in the MCOFI subclass.

I agree with @jrtc27 here. How about we implement this like MCObjectFileInfo::getStackSizeSection(const MCSection&) or MCObjectFileInfo::getBBAddrMapSection(const MCSection&). The intended logic seems quite similar.

It would be inconvenient.

  1. getStackSizeSection and getBBAddrMapSection do not use different section names. They will need to know the suffix
  2. MCObjectFileInfo does not know whether function sections is enabled

MCObjectFileInfo is a good place for getStackSizeSection and getBBAddrMapSection because they need just information from MCObjectFileInfo. However, it is different for .gcc_except_table

Sorry for the misunderstanding. I am fine with your logic, but prefer if the code is better organized: I had a stab at it D90523.

MaskRay updated this revision to Diff 302106.Oct 31 2020, 11:12 AM
MaskRay edited the summary of this revision. (Show Details)

Move section creation logic to TargetLoweringObjectFileImpl.cpp and add a virtual function getSectionForLSDA there

I wonder if it'd be nicer to push this logic into getLSDASection by passing an (optional?) Function given that's where the normal LSDASection gets created? Having the check for ELF here seems to indicate that the logic isn't quite in the right place and belongs in the MCOFI subclass.

I agree with @jrtc27 here. How about we implement this like MCObjectFileInfo::getStackSizeSection(const MCSection&) or MCObjectFileInfo::getBBAddrMapSection(const MCSection&). The intended logic seems quite similar.

It would be inconvenient.

  1. getStackSizeSection and getBBAddrMapSection do not use different section names. They will need to know the suffix
  2. MCObjectFileInfo does not know whether function sections is enabled

MCObjectFileInfo is a good place for getStackSizeSection and getBBAddrMapSection because they need just information from MCObjectFileInfo. However, it is different for .gcc_except_table

Sorry for the misunderstanding. I am fine with your logic, but prefer if the code is better organized: I had a stab at it D90523.

Thanks! This does look cleaner, moreoever, EHStreamer.cpp does not have to include 3 additional headers. Tweaked a bit.

This revision is now accepted and ready to land.Oct 31 2020, 11:14 AM
rahmanl added inline comments.Nov 1 2020, 11:18 AM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
858

I have a question here: Why can't we use a unique ID and use the same section name ('gcc_except_table')?

MaskRay updated this revision to Diff 302173.Nov 1 2020, 1:04 PM
MaskRay edited the summary of this revision. (Show Details)

Add fullblown support

MaskRay edited the summary of this revision. (Show Details)Nov 1 2020, 1:07 PM
MaskRay added inline comments.Nov 2 2020, 9:14 AM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
858

We can... the logic will become more complicated. I'm going to do this.

MaskRay marked an inline comment as done.Nov 2 2020, 9:14 AM

@rahmanl Does this look good to you now? :)

rahmanl added inline comments.Nov 2 2020, 9:50 AM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
852

You have a point here. Currently the help line for -funique-section-names says:
-funique-section-names Use unique names for text and data sections (ELF Only).

And I couldn't find a case where getUniqueSectionNames() is used for non-globals.

One thing I can suggest is not to gate on this flag and instead use the suffix whenever we can't use a unique ID. WDYT?

MaskRay updated this revision to Diff 302365.Nov 2 2020, 12:19 PM
MaskRay edited the summary of this revision. (Show Details)

Don't condition on -funique-section-names

MaskRay marked an inline comment as done.Nov 2 2020, 12:22 PM

(FWIW, I'd personally be in favor of -funique-section-names applying here - the specific textual description of the flag in documentation I don't think should necessarily be held as definitive, but possibly descriptive (perhaps it describes correctly the behavior as it exists/was implemented, and could be updated to include this new behavior) - seems like if someone's passing that flag it's probably because they don't want or may not have toolchain support for non-unique section names. It'd be pretty quirky/odd situation where they didn't want unique section names only for some sections and not for others.

But I don't feel super strongly about it if folks have other ideas)

MaskRay updated this revision to Diff 302399.Nov 2 2020, 1:58 PM
MaskRay edited the summary of this revision. (Show Details)

Switch back to GCC compatible behavior (if -funique-section-names, use a suffix)

@rahmanl: @dblaikie and @jrtc27 prefer having a suffix to be consistent with .text.foo and .rodata.foo . On a second thought I think this indeed makes more sense. People who want to save .strtab space can use -fno-unique-section-names. Are you fine with the resolution?

rahmanl accepted this revision.Nov 2 2020, 2:31 PM

@rahmanl: @dblaikie and @jrtc27 prefer having a suffix to be consistent with .text.foo and .rodata.foo . On a second thought I think this indeed makes more sense. People who want to save .strtab space can use -fno-unique-section-names. Are you fine with the resolution?

Agreed.
In this case`-funique-section-names` is mandatory to ensure LSDA sections are separated when unique IDs cannot be used, and that is fine because unique-section-names=true is the default.

Thanks for making this patch a priority.

This revision was landed with ongoing or failed builds.Nov 2 2020, 2:36 PM
This revision was automatically updated to reflect the committed changes.
jrtc27 added a comment.Nov 2 2020, 8:42 PM

Thanks, we all managed to miss that annoying case. Build is now green.

Thanks, we all managed to miss that annoying case. Build is now green.

Thanks for fixing it! We need an ARM EH test case to prevent regression.