Page MenuHomePhabricator

[THUMB2] default .text alignment to 2B
AbandonedPublic

Authored by nickdesaulniers on Sep 27 2021, 1:21 PM.

Details

Summary

Similar to GAS, default the alignment of .text to 2B. This difference
between clang vs GAS was frustrating efforts to debug pr/51929.

Similar to D102052.

Does not regress Linux kernel boots in QEMU. Tested with Linux kernel
tag v5.15-rc3 with
https://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=9122/1
applied, and QEMU tag v5.2.0. In pr/51929 we're working to dig our way
out of boot failures with ToT QEMU (v6.1.0+).

Link: https://github.com/ClangBuiltLinux/linux/issues/1447

Diff Detail

Unit TestsFailed

TimeTest
670 msx64 debian > ORC-x86_64-linux.TestCases/Linux/x86-64::trivial-cxa-atexit.S
Script: -- : 'RUN: at line 3'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -m64 -c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/orc/X86_64LinuxConfig/TestCases/Linux/x86-64/Output/trivial-cxa-atexit.S.tmp /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/orc/TestCases/Linux/x86-64/trivial-cxa-atexit.S
670 msx64 debian > ORC-x86_64-linux.TestCases/Linux/x86-64::trivial-static-initializer.S
Script: -- : 'RUN: at line 7'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -m64 -c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/orc/X86_64LinuxConfig/TestCases/Linux/x86-64/Output/trivial-static-initializer.S.tmp /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/orc/TestCases/Linux/x86-64/trivial-static-initializer.S
670 msx64 debian > ORC-x86_64-linux.TestCases/Linux/x86-64::trivial-tls.S
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -m64 -c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/orc/X86_64LinuxConfig/TestCases/Linux/x86-64/Output/trivial-tls.S.tmp /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/orc/TestCases/Linux/x86-64/trivial-tls.S

Event Timeline

nickdesaulniers requested review of this revision.Sep 27 2021, 1:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 27 2021, 1:21 PM
  • add llvm-mc based test comparing arm vs thumb
MaskRay added inline comments.Sep 27 2021, 1:45 PM
llvm/lib/Target/ARM/MCTargetDesc/ARMMCObjectFileInfo.h
2

80 column

The approach looks good to me.

  • expand header comment to 80 lines
nickdesaulniers marked an inline comment as done.Sep 27 2021, 1:57 PM

Like the RISCV patch, the test should be in llvm/test/MC/ARM. test/CodeGen/ARM does not test pure lib/MC behavior.

llvm/test/CodeGen/ARM/thumb-text-align.s
1 ↗(On Diff #375399)

< and -o - can be removed

7 ↗(On Diff #375399)

Align the keys (Name, Type, etc)

  • mv test, use @ style comments, remove < and -o -, realign CHECKs
nickdesaulniers marked 2 inline comments as done.Sep 27 2021, 2:45 PM
MaskRay accepted this revision.Sep 27 2021, 2:59 PM

LGTM.

llvm/test/MC/ARM/thumb-text-align.s
2

@@ Test the default alignment for text sections

This revision is now accepted and ready to land.Sep 27 2021, 2:59 PM
  • add comment explaining point of test
nickdesaulniers marked an inline comment as done.Sep 27 2021, 3:09 PM

No objections as I think a defaut alignment of 2 for Thumb is the right thing to do.

One thing that may cause problems is an immediate inline change of state to Arm something like the following with a thumb target.

.text
.arm
nop

Whereas the default alignment of 4 would save us beforehand. There is a small risk that some existing projects could be affected. I think that an eventual fix for pr/51929 could address this case though. Such projects could also add an align directive as well.

nickdesaulniers planned changes to this revision.Sep 28 2021, 10:27 AM
nickdesaulniers added inline comments.
llvm/test/MC/ARM/thumb-text-align.s
5–7

I can drop --sd; that's not needed.

  • drop --sd arg to llvm-readobj
This revision is now accepted and ready to land.Sep 28 2021, 10:32 AM

One thing that may cause problems is an immediate inline change of state to Arm something like the following with a thumb target.

.text
.arm
nop

I think I can extend this patch (preferably a child patch on top); it seems that GAS will:

  1. use 4B alignment for .text if .arm and .thumb are encountered, regardless of -mthumb.
  2. use 4B alignment if only .arm is encountered regardless of -mthumb, or 2B alignment if only .thumb is encountered regardless of -marm.
  3. use 2B alignment for -mthumb, 4B otherwise (if neither .arm or .thumb are encountered).

I'll bet this happens with other TEXT sections, too, not just .text. (unverified)

though for pr/51929, there are no .arm or .thumb directives, so pr/51929 is the third case above.

@ $ arm-linux-gnueabi-as -march=armv7-a x.s -o x.o
@ $ llvm-readelf -S x.o
@ [ 1] .text             PROGBITS        00000000 000034 000004 00  AX  0   0  4
.text
nop
@ $ arm-linux-gnueabi-as -mthumb -march=armv7-a x.s -o x.o
@ $ llvm-readelf -S x.o
@ [ 1] .text             PROGBITS        00000000 000034 000002 00  AX  0   0  2
.text
nop
@ $ arm-linux-gnueabi-as -mthumb -march=armv7-a x.s -o x.o
@ $ llvm-readelf -S x.o
@ [ 1] .text             PROGBITS        00000000 000034 000004 00  AX  0   0  4
.text
.arm
nop
@ $ arm-linux-gnueabi-as -march=armv7-a x.s -o x.o
@ $ llvm-readelf -S x.o
@ [ 1] .text             PROGBITS        00000000 000034 000002 00  AX  0   0  2
.text
.thumb
nop
@ $ arm-linux-gnueabi-as -march=armv7-a x.s -o x.o
@ $ llvm-readelf -S x.o
@ [ 1] .text             PROGBITS        00000000 000034 000008 00  AX  0   0  4
.text
.thumb
nop
.arm
nop
@ $ arm-linux-gnueabi-as -march=armv7-a -mthumb x.s -o x.o
@ $ llvm-readelf -S x.o
@ [ 1] .text             PROGBITS        00000000 000034 000008 00  AX  0   0  4
.text
.thumb
nop
.arm
nop

That said, looking at the case from pr/51929, it still doesn't have a 2B alignment for .text. Trying my best with cvise, it seems the presence of a .align 0 directive also forces GAS to emit .text with 4B alignment. So that's yet a fourth case...(but also what's occurring for pr/51929, so I suspect I'll need this plus handling .align 0 better in llvm).

nickdesaulniers abandoned this revision.Sep 29 2021, 12:27 PM

https://lore.kernel.org/linux-arm-kernel/20210929192026.1604095-1-ndesaulniers@google.com/ is the kernel patch to address pr/51929. At this point, I don't care to see this land, but happy to land it if folks prefer.