[AsmParser] handle .align 0 per MCAsmInfo

[nickdesaulniers]

Summary

While .align 0 may be converted to .balign 1 for x86_64 elf, on THUMB2
it's converted to .balign 4.

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

[nickdesaulniers]

Hey @peter.smith , I'm not sure if you have cycles to look at this, but I could use some help. I've been trying to understand why the fragments created seem to have an offset of zero (I suspect the offset should be 2B into the function?) but I don't quite understand the fragment layout process very well. The added test case is failing on the Size: check; GAS passes this test.

[craig.topper]

llvm/include/llvm/MC/MCAsmInfo.h

861

Why not make Alignment a reference?

[nickdesaulniers]

llvm/include/llvm/MC/MCAsmInfo.h

861

It's been beaten into my head the Google C++ coding style where in/out params are passed by pointer, so that it's obvious at the call site that they're being modified since their address must be taken. If this is explicitly not the LLVM style guide, I'm happy to change it to a non-const reference.

(The Google C++ coding style recommends const-reference params or pointer params; for in vs in-out params).

[nickdesaulniers]

ARMAsmParser::parseDirectiveAlign falls back to AsmParser::parseDirectiveAlign, which doesn't handle .align 0 correctly for THUMB.

[nickdesaulniers]

(oh, this is actually good enough to fix pr/51929). Maybe I can add a fixme to the test on the Size noting that GAS adds explicit nops. We don't necessarily need the nops; we need to not treat .align 0 as .align 1 on THUMB2 (and probably ARM; unverified).

[craig.topper]

In D110660#3028806, @nickdesaulniers wrote:

(oh, this is actually good enough to fix pr/51929). Maybe I can add a fixme to the test on the Size noting that GAS adds explicit nops. We don't necessarily need the nops; we need to not treat .align 0 as .align 1 on THUMB2 (and probably ARM; unverified).

Is this an issue for RISCV too?

[craig.topper]

In D110660#3028842, @craig.topper wrote:

In D110660#3028806, @nickdesaulniers wrote:

(oh, this is actually good enough to fix pr/51929). Maybe I can add a fixme to the test on the Size noting that GAS adds explicit nops. We don't necessarily need the nops; we need to not treat .align 0 as .align 1 on THUMB2 (and probably ARM; unverified).

Is this an issue for RISCV too?

Nevermind, after poking around I see this is a specific to ARM in gas.

[nickdesaulniers]

llvm/test/MC/ARM/thumb-text-align0.s

9

mov r0, r1
.align 0
mov pc, lr

would be a more demonstrative test.

[nickdesaulniers]

• rewrite tests

[MaskRay]

While .align 0 may be converted to .balign 1 for x86_64 elf, on THUMB2 it's converted to .balign 4.

Does gas do so? This is a surprising behavior. I hope we don't follow. The user should be fixed instead.

https://sourceware.org/binutils/docs/as/Align.html#Align says a default value is used. If thumb typically uses 2-byte alignment, .align 0 meaning .balign 4 seems unexpected.

[MaskRay]

I really don't want to see that MC adopts such a behavior.

I will disappear for a few days and be back next ~Wednesday.

[nickdesaulniers]

I don't disagree. It looks like the 32b ARM linux kernel is using .align 0 by default for all symbol descriptions; I think changing it to .align 2 (to match aarch64) would be NFC for GAS and fix the underlying THUMB2 related boot failure for clang. Let me submit such a patch on the kernel side for comment+review. Thanks for the feedback.

llvm/include/llvm/MC/MCAsmInfo.h

 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_MC_MCASMINFO_H
 #define LLVM_MC_MCASMINFO_H
 
 #include "llvm/ADT/StringRef.h"
 #include "llvm/MC/MCDirectives.h"
 #include "llvm/MC/MCTargetOptions.h"
+#include <cstdint>
 #include <vector>
 
 namespace llvm {
 
 class MCContext;
 class MCCFIInstruction;
 class MCExpr;
 class MCSection;
public:
 
   bool shouldUseLogicalShr() const { return UseLogicalShr; }
 
   bool canRelaxRelocations() const { return RelaxELFRelocations; }
   void setRelaxELFRelocations(bool V) { RelaxELFRelocations = V; }
   bool hasMipsExpressions() const { return HasMipsExpressions; }
   bool needsFunctionDescriptors() const { return NeedsFunctionDescriptors; }
   bool shouldUseMotorolaIntegers() const { return UseMotorolaIntegers; }
+
+  /// .align 0 may have architecture specific meaning.
+  virtual void handleAlignmentZero(int64_t *Alignment) const;
 };
 
 } // end namespace llvm
 
 #endif // LLVM_MC_MCASMINFO_H

llvm/lib/MC/MCAsmInfo.cpp

bool MCAsmInfo::isValidUnquotedName(StringRef Name) const {
   return true;
 }
 
 bool MCAsmInfo::shouldOmitSectionDirective(StringRef SectionName) const {
   // FIXME: Does .section .bss/.data/.text work everywhere??
   return SectionName == ".text" || SectionName == ".data" ||
         (SectionName == ".bss" && !usesELFSectionDirectiveForBSS());
 }
+
+void MCAsmInfo::handleAlignmentZero(int64_t *Alignment) const {
+  assert(*Alignment == 0 && "caller should check that alignment is zero");
+  *Alignment = 1;
+}

llvm/lib/MC/MCParser/AsmParser.cpp

bool AsmParser::parseDirectiveAlign(bool IsPow2, unsigned ValueSize) {
   }
   if (parseAlign())
     return true;
 
   // Always emit an alignment here even if we thrown an error.
   bool ReturnVal = false;
 
   // Compute alignment in bytes.
-  if (IsPow2) {
+  if (Alignment == 0) {
+    Lexer.getMAI().handleAlignmentZero(&Alignment);
+  } else if (IsPow2) {
     // FIXME: Diagnose overflow.
     if (Alignment >= 32) {
       ReturnVal |= Error(AlignmentLoc, "invalid alignment value");
       Alignment = 31;
     }
 
     Alignment = 1ULL << Alignment;
   } else {
-    // Reject alignments that aren't either a power of two or zero,
-    // for gas compatibility. Alignment of zero is silently rounded
-    // up to one.
-    if (Alignment == 0)
-      Alignment = 1;
+    // Reject alignments that aren't either a power of two or > 2**32
+    // for gas compatibility.
     if (!isPowerOf2_64(Alignment))
       ReturnVal |= Error(AlignmentLoc, "alignment must be a power of 2");
     if (!isUInt<32>(Alignment))
       ReturnVal |= Error(AlignmentLoc, "alignment must be smaller than 2**32");
   }
 
   // Diagnose non-sensical max bytes to align.
   if (MaxBytesLoc.isValid()) {

llvm/lib/Target/ARM/MCTargetDesc/ARMMCAsmInfo.h

 
 class ARMELFMCAsmInfo : public MCAsmInfoELF {
   void anchor() override;
 
 public:
   explicit ARMELFMCAsmInfo(const Triple &TT);
 
   void setUseIntegratedAssembler(bool Value) override;
+  void handleAlignmentZero(int64_t *Alignment) const override;
 };
 
 class ARMCOFFMCAsmInfoMicrosoft : public MCAsmInfoMicrosoft {
   void anchor() override;
 
 public:
   explicit ARMCOFFMCAsmInfoMicrosoft();
 };

llvm/lib/Target/ARM/MCTargetDesc/ARMMCAsmInfo.cpp

void ARMELFMCAsmInfo::setUseIntegratedAssembler(bool Value) {
   if (!UseIntegratedAssembler) {
     // gas doesn't handle VFP register names in cfi directives,
     // so don't use register names with external assembler.
     // See https://sourceware.org/bugzilla/show_bug.cgi?id=16694
     DwarfRegNumForCFI = true;
   }
 }
 
+void ARMELFMCAsmInfo::handleAlignmentZero(int64_t *Alignment) const {
+  *Alignment = 4;
+}
+
 void ARMCOFFMCAsmInfoMicrosoft::anchor() { }
 
 ARMCOFFMCAsmInfoMicrosoft::ARMCOFFMCAsmInfoMicrosoft() {
   AlignmentIsInBytes = false;
   SupportsDebugInformation = true;
   ExceptionsType = ExceptionHandling::WinEH;
   PrivateGlobalPrefix = "$M";
   PrivateLabelPrefix = "$M";

llvm/test/MC/ARM/thumb-text-align0.s

+@@ Test that .align 0 realigns to 4B.
+
+@ RUN: llvm-mc %s -triple=thumbv7-linux-gnueabi -filetype=obj -o %t
+@ RUN: llvm-objdump --triple=thumbv7 -d %t | FileCheck %s --check-prefixes=CHECK,CHECK-THUMB
+@ RUN: llvm-mc %s -triple=armv7-linux-gnueabi -filetype=obj -o %t
+@ RUN: llvm-objdump --triple=armv7 -d %t | FileCheck %s --check-prefixes=CHECK,CHECK-ARM
+
+.global foo
+foo:
+mov r1, r0
+.align 0
+mov pc, lr
+
+@ CHECK-LABEL:     foo>:
+@ CHECK-ARM-NEXT:    mov r1, r0
+@ CHECK-ARM-NEXT:    mov pc, lr
+@ CHECK-THUMB-NEXT:  mov r1, r0
+@ CHECK-THUMB-NEXT:  nop
+@ CHECK-THUMB-NEXT:  mov pc, lr

llvm/test/MC/X86/text-align0.s

+// Test that .align 0 realigns to 1B.
+
+// RUN: llvm-mc %s -triple=x86_64-linux-gnu -filetype=obj -o %t
+// RUN: llvm-objdump --triple=x86_64 -d %t | FileCheck %s
+
+.global foo
+foo:
+mov %rdi, %rdx
+.align 0
+ret
+
+// CHECK-LABEL: foo>:
+// CHECK-NEXT:    movq %rdi, %rdx
+// CHECK-NEXT:    retq