Skip to content

[MC] Set SHF_EXCLUDE for AArch64 (and other) build attributes sections #125824

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

zmodem
Copy link
Collaborator

@zmodem zmodem commented Feb 5, 2025

The sections are not supposed to end up in linked executables.

Follow-up to #123990

The sections are not supposed to end up in linked executables.

Follow-up to llvm#123990
@zmodem zmodem added backend:AArch64 mc Machine (object) code labels Feb 5, 2025
@llvmbot
Copy link
Member

llvmbot commented Feb 5, 2025

@llvm/pr-subscribers-mc

@llvm/pr-subscribers-backend-aarch64

Author: Hans Wennborg (zmodem)

Changes

The sections are not supposed to end up in linked executables.

Follow-up to #123990


Full diff: https://github.com/llvm/llvm-project/pull/125824.diff

2 Files Affected:

  • (modified) llvm/lib/MC/MCELFStreamer.cpp (+2-1)
  • (modified) llvm/test/CodeGen/AArch64/aarch64-build-attributes-all.ll (+6-1)
diff --git a/llvm/lib/MC/MCELFStreamer.cpp b/llvm/lib/MC/MCELFStreamer.cpp
index 282c82198507d74..bae6becf719c99e 100644
--- a/llvm/lib/MC/MCELFStreamer.cpp
+++ b/llvm/lib/MC/MCELFStreamer.cpp
@@ -800,7 +800,8 @@ void MCELFStreamer::createAttributesWithSubsection(
   if (AttributeSection) {
     switchSection(AttributeSection);
   } else {
-    AttributeSection = getContext().getELFSection(Section, Type, 0);
+    AttributeSection =
+        getContext().getELFSection(Section, Type, ELF::SHF_EXCLUDE);
     switchSection(AttributeSection);
 
     // Format version
diff --git a/llvm/test/CodeGen/AArch64/aarch64-build-attributes-all.ll b/llvm/test/CodeGen/AArch64/aarch64-build-attributes-all.ll
index aecc74b2ce46ddd..0943a3e5cc9a360 100644
--- a/llvm/test/CodeGen/AArch64/aarch64-build-attributes-all.ll
+++ b/llvm/test/CodeGen/AArch64/aarch64-build-attributes-all.ll
@@ -1,11 +1,16 @@
 ; RUN: llc %s -o - | FileCheck %s --check-prefix=ASM
-; RUN: llc %s -filetype=obj -o - | llvm-readelf --hex-dump=.ARM.attributes - | FileCheck %s --check-prefix=ELF
+; RUN: llc %s -filetype=obj -o - | llvm-readelf --section-details --hex-dump=.ARM.attributes - | FileCheck %s --check-prefix=ELF
 
 ; ASM:      .aeabi_subsection	aeabi_feature_and_bits, optional, uleb128
 ; ASM-NEXT: .aeabi_attribute	Tag_Feature_BTI, 1
 ; ASM-NEXT: .aeabi_attribute	Tag_Feature_PAC, 1
 ; ASM-NEXT: .aeabi_attribute	Tag_Feature_GCS, 1
 
+; ELF: Section Headers:
+; ELF: .ARM.attributes
+; ELF-NEXT: 0000000000000000
+; ELF-NEXT: EXCLUDE
+
 ; ELF: Hex dump of section '.ARM.attributes':
 ; ELF-NEXT: 0x00000000 41230000 00616561 62695f66 65617475 A#...aeabi_featu
 ; ELF-NEXT: 0x00000010 72655f61 6e645f62 69747300 01000001 re_and_bits.....

@zmodem
Copy link
Collaborator Author

zmodem commented Feb 5, 2025

Please take a careful look since I'm not really familiar with these things. See comments on #123990 for background.

Note that this also affects other consumers of emitAttributesSection(), such as .ARM.attributes, .hexagon.attributes, and .riscv.attributes, but I think they also don't want the sections in the linked output?

Copy link
Collaborator

@smithp35 smithp35 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think SHF_EXCLUDE is too strong. I think the underlying problem is that LLD support hasn't been added yet. It is being worked on but there's no public patch as yet.

The original (Arm) specification for BuildAttributes https://github.com/ARM-software/abi-aa/blob/main/addenda32/addenda32.rst#313the-scope-of-build-attributes

This standard places no requirements on the presence or absence of build attributes in executable files.

This was added on behalf of the GNU project, which wanted to retain a "merged" build attributes section in the ELF output file.

In the original Arm LLD port I followed Arm's proprietary linker and did not output a Build Attributes section, only to find that eglibc (Ubuntu 14.04 LTS) dlopen was using it and wouldn't work unless it was present.

ld -r relocatable links also need to output a merged build attributes section as the relocatable object can be used as an input for further links.

Setting SHF_EXCLUDE could have some unintended consequences with interoperation with GNU ld.

I think the best way forward for now is to make a placeholder LLD patch that just discards the AArch64 Build Attributes section. When the LLD support is implemented it can remove that.

I'll submit a PR for that and will post a link here (@sivan-shani )

@smithp35
Copy link
Collaborator

smithp35 commented Feb 5, 2025

I've posted #125838 to discard .ARM.attributes sections in LLD. This will be the right thing to do for all but relocatable links so it can be built upon by @sivan-shani , who is working on full support in LLD right now.

@zmodem
Copy link
Collaborator Author

zmodem commented Feb 5, 2025

Thanks for the quick reply!

I'm a bit concerned that we're making this dependent on the LLD version. For example, someone using Clang 20 with LLD 19 will get these extra sections (and as we've seen in our case, the size can be significant if there are many .o files.)

If the intention is that LLD (and other linkers) without explicit support for these attribute sections should drop them, isn't SHF_EXCLUDE the right answer? A linker which does support these sections can still process them any way they want, right?

Setting SHF_EXCLUDE could have some unintended consequences with interoperation with GNU ld.

I suppose this would be an issue if they filter out SHF_EXCLUDE before checking for SHT_AARCH64_ATTRIBUTES. I don't have any aarch64 ld around to check with.

I think the best way forward for now is to make a placeholder LLD patch that just discards the AArch64 Build Attributes section.

That will solve our problem :) I'm not sure it's best solution, but I'm not an expert in this area.

@smithp35
Copy link
Collaborator

smithp35 commented Feb 5, 2025

There may be some scope to add SHF_EXCLUDE to the AArch64 Build Attributes as it is new. I'd like to run it past the GNU team in Arm to make sure they're OK with it in GNU ld. I'd also like to check when it was introduced and if any older linkers error if they see it and don't understand it.

I wouldn't want to add it to ARM as it is included in the output for GNU ld.

@zmodem
Copy link
Collaborator Author

zmodem commented Feb 5, 2025

Closing this in favor of #125838

@zmodem zmodem closed this Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:AArch64 mc Machine (object) code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants