release/21.x: [ARM] Use TargetMachine over Subtarget in ARMAsmPrinter (#166329)

[llvmbot]

Backport 4d1f249

Requested by: @davemgreen

[llvmbot]

@efriedma-quic What do you think about merging this PR to the release branch?

[llvmbot]

@llvm/pr-subscribers-backend-arm

Author: None (llvmbot)

Changes

Backport 4d1f249

Requested by: @davemgreen

---

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

4 Files Affected:

• (modified) llvm/lib/Target/ARM/ARMAsmPrinter.cpp (+11-10)
• (modified) llvm/lib/Target/ARM/ARMSubtarget.cpp (+1-11)
• (modified) llvm/lib/Target/ARM/ARMTargetMachine.h (+14)
• (added) llvm/test/CodeGen/ARM/xxstructor-nodef.ll (+7)

diff --git a/llvm/lib/Target/ARM/ARMAsmPrinter.cpp b/llvm/lib/Target/ARM/ARMAsmPrinter.cpp
index 850b00406f09e..aa6ef55dad26c 100644
--- a/llvm/lib/Target/ARM/ARMAsmPrinter.cpp
+++ b/llvm/lib/Target/ARM/ARMAsmPrinter.cpp
@@ -97,7 +97,8 @@ void ARMAsmPrinter::emitXXStructor(const DataLayout &DL, const Constant *CV) {
 
   const MCExpr *E = MCSymbolRefExpr::create(
       GetARMGVSymbol(GV, ARMII::MO_NO_FLAG),
-      (Subtarget->isTargetELF() ? ARM::S_TARGET1 : ARM::S_None), OutContext);
+      (TM.getTargetTriple().isOSBinFormatELF() ? ARM::S_TARGET1 : ARM::S_None),
+      OutContext);
 
   OutStreamer->emitValue(E, Size);
 }
@@ -595,8 +596,7 @@ void ARMAsmPrinter::emitEndOfAsmFile(Module &M) {
   ARMTargetStreamer &ATS = static_cast<ARMTargetStreamer &>(TS);
 
   if (OptimizationGoals > 0 &&
-      (Subtarget->isTargetAEABI() || Subtarget->isTargetGNUAEABI() ||
-       Subtarget->isTargetMuslAEABI()))
+      (TT.isTargetAEABI() || TT.isTargetGNUAEABI() || TT.isTargetMuslAEABI()))
     ATS.emitAttribute(ARMBuildAttrs::ABI_optimization_goals, OptimizationGoals);
   OptimizationGoals = -1;
 
@@ -866,9 +866,10 @@ static uint8_t getModifierSpecifier(ARMCP::ARMCPModifier Modifier) {
 
 MCSymbol *ARMAsmPrinter::GetARMGVSymbol(const GlobalValue *GV,
                                         unsigned char TargetFlags) {
-  if (Subtarget->isTargetMachO()) {
+  const Triple &TT = TM.getTargetTriple();
+  if (TT.isOSBinFormatMachO()) {
     bool IsIndirect =
-        (TargetFlags & ARMII::MO_NONLAZY) && Subtarget->isGVIndirectSymbol(GV);
+        (TargetFlags & ARMII::MO_NONLAZY) && getTM().isGVIndirectSymbol(GV);
 
     if (!IsIndirect)
       return getSymbol(GV);
@@ -885,9 +886,8 @@ MCSymbol *ARMAsmPrinter::GetARMGVSymbol(const GlobalValue *GV,
       StubSym = MachineModuleInfoImpl::StubValueTy(getSymbol(GV),
                                                    !GV->hasInternalLinkage());
     return MCSym;
-  } else if (Subtarget->isTargetCOFF()) {
-    assert(Subtarget->isTargetWindows() &&
-           "Windows is the only supported COFF target");
+  } else if (TT.isOSBinFormatCOFF()) {
+    assert(TT.isOSWindows() && "Windows is the only supported COFF target");
 
     bool IsIndirect =
         (TargetFlags & (ARMII::MO_DLLIMPORT | ARMII::MO_COFFSTUB));
@@ -914,7 +914,7 @@ MCSymbol *ARMAsmPrinter::GetARMGVSymbol(const GlobalValue *GV,
     }
 
     return MCSym;
-  } else if (Subtarget->isTargetELF()) {
+  } else if (TT.isOSBinFormatELF()) {
     return getSymbolPreferLocal(*GV);
   }
   llvm_unreachable("unexpected target");
@@ -960,7 +960,8 @@ void ARMAsmPrinter::emitMachineConstantPoolValue(
 
     // On Darwin, const-pool entries may get the "FOO$non_lazy_ptr" mangling, so
     // flag the global as MO_NONLAZY.
-    unsigned char TF = Subtarget->isTargetMachO() ? ARMII::MO_NONLAZY : 0;
+    unsigned char TF =
+        TM.getTargetTriple().isOSBinFormatMachO() ? ARMII::MO_NONLAZY : 0;
     MCSym = GetARMGVSymbol(GV, TF);
   } else if (ACPV->isMachineBasicBlock()) {
     const MachineBasicBlock *MBB = cast<ARMConstantPoolMBB>(ACPV)->getMBB();
diff --git a/llvm/lib/Target/ARM/ARMSubtarget.cpp b/llvm/lib/Target/ARM/ARMSubtarget.cpp
index 13185a7d797a3..63d6e2ea7389b 100644
--- a/llvm/lib/Target/ARM/ARMSubtarget.cpp
+++ b/llvm/lib/Target/ARM/ARMSubtarget.cpp
@@ -316,17 +316,7 @@ bool ARMSubtarget::isRWPI() const {
 }
 
 bool ARMSubtarget::isGVIndirectSymbol(const GlobalValue *GV) const {
-  if (!TM.shouldAssumeDSOLocal(GV))
-    return true;
-
-  // 32 bit macho has no relocation for a-b if a is undefined, even if b is in
-  // the section that is being relocated. This means we have to use o load even
-  // for GVs that are known to be local to the dso.
-  if (isTargetMachO() && TM.isPositionIndependent() &&
-      (GV->isDeclarationForLinker() || GV->hasCommonLinkage()))
-    return true;
-
-  return false;
+  return TM.isGVIndirectSymbol(GV);
 }
 
 bool ARMSubtarget::isGVInGOT(const GlobalValue *GV) const {
diff --git a/llvm/lib/Target/ARM/ARMTargetMachine.h b/llvm/lib/Target/ARM/ARMTargetMachine.h
index 1d73af1da6d02..5f17a13dac40e 100644
--- a/llvm/lib/Target/ARM/ARMTargetMachine.h
+++ b/llvm/lib/Target/ARM/ARMTargetMachine.h
@@ -99,6 +99,20 @@ class ARMBaseTargetMachine : public CodeGenTargetMachineImpl {
     return true;
   }
 
+  bool isGVIndirectSymbol(const GlobalValue *GV) const {
+    if (!shouldAssumeDSOLocal(GV))
+      return true;
+
+    // 32 bit macho has no relocation for a-b if a is undefined, even if b is in
+    // the section that is being relocated. This means we have to use o load
+    // even for GVs that are known to be local to the dso.
+    if (getTargetTriple().isOSBinFormatMachO() && isPositionIndependent() &&
+        (GV->isDeclarationForLinker() || GV->hasCommonLinkage()))
+      return true;
+
+    return false;
+  }
+
   yaml::MachineFunctionInfo *createDefaultFuncInfoYAML() const override;
   yaml::MachineFunctionInfo *
   convertFuncInfoToYAML(const MachineFunction &MF) const override;
diff --git a/llvm/test/CodeGen/ARM/xxstructor-nodef.ll b/llvm/test/CodeGen/ARM/xxstructor-nodef.ll
new file mode 100644
index 0000000000000..db17b2b1c21ab
--- /dev/null
+++ b/llvm/test/CodeGen/ARM/xxstructor-nodef.ll
@@ -0,0 +1,7 @@
+; RUN: llc -mtriple=arm-unknown-linux-gnueabihf < %s | FileCheck %s
+
+; This test contains a llvm.global_ctors with no other definitions. Make sure we do not crash in that case.
+; CHECK: .section        .init_array,"aw",%init_array
+
+declare  ccc void @ghczmbignum_GHCziNumziBackendziSelected_init__prof_init()
+@llvm.global_ctors = appending global [1 x {i32, void ()*, i8* }] [{i32, void ()*, i8* }{i32  65535, void ()*  @ghczmbignum_GHCziNumziBackendziSelected_init__prof_init, i8*  null } ]

[efriedma-quic]

This should be a safe fix: it's all in the ARM backend, and it's pretty clear reading the code that the semantics should be the same outside the case that was crashing.

---

That said, I'm not sure how you end up hitting this in practice; @swt2c, could you explain why you're requesting the backport?

[swt2c]

The same reason as the original bug reporter @iliastsi - trying to fix GHC in Debian.

[dyung]

The same reason as the original bug reporter @iliastsi - trying to fix GHC in Debian.

From the original bug report, it seems that this has been broken since at least LLVM 17. If this is the case, how have you been building GHC all this time? Were you using LLVM 16?

At this point in the release, we are only accepting patches for recent regressions or major issues. Given this issues seems to have been around for quite a while (and only seems to be hit in this one case), I am leaning towards not merging this change into the 21.x release branch. If you feel strongly that it should be merged in, please let us know the rationale and I will discuss it with the other release managers to decide whether it should be included at this point of the release.

[swt2c]

It looks like GHC 9.6.6 was built using LLVM 18 and GHC 9.8.4 was built using LLVM 19. This issue seems to have been uncovered when building GHC 9.10.