[AArch64][ELF] Section alignment of 4 for AArch64 instruction #114031

popaflorin · 2024-10-29T10:47:23Z

The integrated assembler sets a minimum alignment for the .text section of 4. However user defined sections get an alignment of 1. Unlike the GNU assembler which raises the section alignment to 4 if an AArch64 instruction is used, the integrated assembler leaves the alignment at 1

github-actions · 2024-10-29T10:47:45Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2024-10-29T10:48:20Z

@llvm/pr-subscribers-lld-elf
@llvm/pr-subscribers-lld
@llvm/pr-subscribers-mc

@llvm/pr-subscribers-backend-aarch64

Author: Florin Popa (popaflorin)

Changes

The integrated assembler sets a minimum alignment for the .text section of 4. However user defined sections get an alignment of 1. Unlike the GNU assembler which raises the section alignment to 4 if an AArch64 instruction is used, the integrated assembler leaves the alignment at 1

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

2 Files Affected:

(modified) llvm/lib/MC/MCParser/ELFAsmParser.cpp (+9)
(added) llvm/test/MC/AArch64/directive-arch-section-alignment.s (+22)

diff --git a/llvm/lib/MC/MCParser/ELFAsmParser.cpp b/llvm/lib/MC/MCParser/ELFAsmParser.cpp
index c4536441665fa0..8e7256d6fae9cd 100644
--- a/llvm/lib/MC/MCParser/ELFAsmParser.cpp
+++ b/llvm/lib/MC/MCParser/ELFAsmParser.cpp
@@ -697,6 +697,15 @@ bool ELFAsmParser::ParseSectionArguments(bool IsPush, SMLoc loc) {
       getContext().getELFSection(SectionName, Type, Flags, Size, GroupName,
                                  IsComdat, UniqueID, LinkedToSym);
   getStreamer().switchSection(Section, Subsection);
+
+  // Section alignment of 4 if an AArch64 instruction is used when $x mapping
+  // symbol is added Match GNU Assembler
+  const Triple &TT = getContext().getTargetTriple();
+  if ((Section->getFlags() & ELF::SHF_EXECINSTR) && (TT.isAArch64())) {
+    if (Section->getAlign() < 4)
+      getStreamer().emitValueToAlignment(Align(4));
+  }
+
   // Check that flags are used consistently. However, the GNU assembler permits
   // to leave out in subsequent uses of the same sections; for compatibility,
   // do likewise.
diff --git a/llvm/test/MC/AArch64/directive-arch-section-alignment.s b/llvm/test/MC/AArch64/directive-arch-section-alignment.s
new file mode 100644
index 00000000000000..bf3881b9c288a7
--- /dev/null
+++ b/llvm/test/MC/AArch64/directive-arch-section-alignment.s
@@ -0,0 +1,22 @@
+// RUN: llvm-mc -triple aarch64-- -o - %s | FileCheck %s
+
+// CHECK: .section sec00
+// CHECK-NEXT: .p2align 2
+// CHECK-NEXT: nop
+.section sec00, "ax"
+nop
+nop
+// CHECK: .section sec01
+// CHECK-NEXT: .p2align 2
+// CHECK-NEXT: .p2align 2
+// CHECK-NEXT: nop
+.section sec01, "ax"
+.balign 4
+nop
+// CHECK: .section sec02
+// CHECK-NEXT: .p2align 2
+// CHECK-NEXT: .byte 1
+.section sec02, "ax"
+// CHECK-NEXT: nop
+.byte 1
+nop

llvm/lib/MC/MCParser/ELFAsmParser.cpp

llvm/test/MC/AArch64/directive-arch-section-alignment.s

llvm/lib/MC/MCParser/ELFAsmParser.cpp

lenary · 2024-10-31T12:34:26Z

llvm/lib/Target/AArch64/MCTargetDesc/AArch64ELFStreamer.cpp

You only need this for sections with instructions, right?

Yes - this should be updated to be like before @popaflorin where you were checking for if AArch64 instructions were used.

Thank you both for your feedback! I haven't figured out how to do that yet in this AArch64ELFStreamer. I thought this will suffice: Section->hasInstructions() for a check but it always comes back as 0. I'm looking into it...

it might be better/easier to check if the section is executable? that's probably the only info you have when the section is brand new (I think hasInstructions only becomes true when the first instruction is emitted into the section)

I am checking now Section->isText() in the latest patchset which seems to get the correct executable section. In the test I have also included a aw section to prove that the alignment will remain unchanged at 1

llvm/test/MC/AArch64/directive-arch-section-alignment.s

lenary · 2024-11-22T21:06:58Z

llvm/lib/Target/AArch64/MCTargetDesc/AArch64ELFStreamer.cpp

Can you clarify why you need both the setAlignment on the section, and the emitValueToAlignment (which will insert bytes until the offset in the section is so aligned)

Am I missing when the latter is needed?

This was a leftover from initial testing and I forgot to remove it. I have now corrected it in the latest patch

lenary

Since the most recent update, llvm/test/MC/AArch64/directive-arch-section-alignment.s seems to be failing on Linux at least - can you check it again please?

I also have a suggestion, both for clarity (using ensureMinAlignment) and code style (no braces on a single-statement if).

llvm/lib/Target/AArch64/MCTargetDesc/AArch64ELFStreamer.cpp

lenary

LGTM! Thanks!

lenary · 2024-12-03T22:06:54Z

Rebasing the patch, there seems to be a problem with lld/test/ELF/aarch64-relocs.s (this would have also happened after a merge). Can you check that testcase? I think the problem is likely the testcase itself - but I'm not entirely sure. You'll need to build LLD, which shouldn't add much overhead to your existing LLVM build.

Stylie777

Changes LGTM! Thanks @popaflorin

lld/test/ELF/aarch64-relocs.s

ostannard · 2025-02-19T15:42:09Z

lld/test/ELF/aarch64-relocs.s

Can this be converted into a CHECK line, to make sure that the address matches? If not, then please at least update it to match the actual address of foo128 (which I think is 0x210190 now).

ostannard

Thanks, LGTM

MaskRay · 2025-02-19T17:28:22Z

lld/test/ELF/aarch64-relocs.s

the section addresses are insignificant. Just remove them.

If lld has changed the section layout, the test won't need to be updated.

Thank you! I have merged now your changes

MaskRay · 2025-02-19T17:39:14Z

lld/test/ELF/aarch64-relocs.s

These addresses are insignificant. I removed them in e1d1bb9

Thank you! I have merged now your changes

llvm/test/MC/AArch64/directive-arch-section-alignment.s

The integrated assembler sets a minimum alignment for the .text section of 4. However user defined sections get an alignment of 1. Unlike the GNU assembler which raises the section alignment to 4 if an AArch64 instruction is used, the integrated assembler leaves the alignment at 1

As a result of review feedback, updated the synthax for the change and first attempt to fix lld test that was failing due to missalignment

Renamed the test to align-code.s and changed generic aarch64 ELF

Stylie777 · 2025-02-24T09:17:19Z

I have merged this on behalf of @popaflorin, thanks for this!

github-actions · 2025-02-24T09:17:27Z

@popaflorin Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

llvmbot added backend:AArch64 llvm:mc Machine (object) code labels Oct 29, 2024

lenary reviewed Oct 29, 2024

View reviewed changes

llvm/lib/MC/MCParser/ELFAsmParser.cpp Outdated Show resolved Hide resolved

Stylie777 reviewed Oct 31, 2024

View reviewed changes

llvm/test/MC/AArch64/directive-arch-section-alignment.s Outdated Show resolved Hide resolved

lenary reviewed Oct 31, 2024

View reviewed changes

lenary mentioned this pull request Nov 21, 2024

Sections containing instructions should be aligned to the architecture's instruction alignment #117103

Open

lenary reviewed Nov 22, 2024

View reviewed changes

lenary reviewed Dec 3, 2024

View reviewed changes

llvm/lib/Target/AArch64/MCTargetDesc/AArch64ELFStreamer.cpp Outdated Show resolved Hide resolved

lenary approved these changes Dec 3, 2024

View reviewed changes

Stylie777 approved these changes Dec 4, 2024

View reviewed changes

llvmbot added lld lld:ELF labels Feb 19, 2025

ostannard reviewed Feb 19, 2025

View reviewed changes

ostannard approved these changes Feb 19, 2025

View reviewed changes

MaskRay reviewed Feb 19, 2025

View reviewed changes

llvm/test/MC/AArch64/directive-arch-section-alignment.s Outdated Show resolved Hide resolved

flopop01 added 7 commits February 20, 2025 07:11

[AArch64][ELF] Updated the synthax and fixed the lld test

2aa199f

As a result of review feedback, updated the synthax for the change and first attempt to fix lld test that was failing due to missalignment

[AArch64][ELF] Updated the synthax and fixed the lld test

1483be7

As a result of review feedback, updated the synthax for the change and first attempt to fix lld test that was failing due to missalignment

popaflorin force-pushed the aarch64-section-alignment branch from 21c9b19 to 1483be7 Compare February 20, 2025 07:13

Merge branch 'main' into aarch64-section-alignment

28cf272

MaskRay approved these changes Feb 20, 2025

View reviewed changes

[AArch64][ELF] Renamed and updated the target for the test

83c8656

Renamed the test to align-code.s and changed generic aarch64 ELF

Stylie777 merged commit 301fe47 into llvm:main Feb 24, 2025
8 checks passed

[AArch64][ELF] Section alignment of 4 for AArch64 instruction #114031

[AArch64][ELF] Section alignment of 4 for AArch64 instruction #114031

Uh oh!

Conversation

popaflorin commented Oct 29, 2024

Uh oh!

github-actions bot commented Oct 29, 2024

Uh oh!

llvmbot commented Oct 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lenary left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lenary left a comment

Choose a reason for hiding this comment

Uh oh!

lenary commented Dec 3, 2024

Uh oh!

Stylie777 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flopop01 Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ostannard left a comment

Choose a reason for hiding this comment

Uh oh!

MaskRay Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Stylie777 commented Feb 24, 2025

Uh oh!

github-actions bot commented Feb 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

llvmbot commented Oct 29, 2024 •

edited

Loading

flopop01 Feb 19, 2025 •

edited

Loading

MaskRay Feb 19, 2025 •

edited

Loading