[MC][CodeGen] Emit constant pools earlier

[aeubanks]

Summary

Previously we would emit constant pool entries for ldr inline asm at the
very end of AsmPrinter::doFinalization(). However, if we're emitting
dwarf aranges, that would end all sections with aranges. Then if we have
constant pool entries to be emitted in those same sections, we'd hit an
assert that the section has already been ended.

We want to emit constant pool entries before emitting dwarf aranges.
This patch splits out arm32/64's constant pool entry emission into its
own MCTargetStreamer virtual method.

Fixes PR51208

[aeubanks]

suggestions welcome, I'm not familiar with the MC layer

[MaskRay]

LG. Calling MCStreamer::endSection early may look weird, but otherwise we need to call it elsewhere. Keeping MCStreamer::endSection in DwarfDebug.cpp as is looks fine for now.

llvm/test/MC/ARM/arange-ldr.ll

1 ↗

(On Diff #363591)

How about test/DebugInfo/ARM/

Add a file-level comment about the purpose of the test

6 ↗

(On Diff #363591)

CHECK:       ldr r7, .Ltmp[[#TMP:]]

CHECK:      .tmp[[#TMP]]:
CHECK-NEXT: .long  83040

8 ↗

(On Diff #363591)

Have some basic checking for .debug_aranges

[MaskRay]

llvm/include/llvm/MC/MCStreamer.h

130

I may place this above, grouping with a few other emit*.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

1818

Perhaps a comment that this needs to be before debug information.

[probinson]

llvm/test/MC/ARM/arange-ldr.ll

1 ↗

(On Diff #363591)

I disagree about moving the test, test/DebugInfo is for tests of reading debug info, not producing it. This patch is about producing constants and debug info correctly.

[aeubanks]

address comments

[MaskRay]

llvm/test/MC/ARM/arange-ldr.ll

1 ↗

(On Diff #363591)

test/DebugInfo/ has many .ll and .mir tests (producer).

test/CodeGen/ has a lot as well.

test/MC/ is uncommon for .ll tests.

[MaskRay]

LGTM, with a comment that the test does not belong to test/MC/ (which is mainly for .s tests)

[MaskRay]

llvm/test/MC/ARM/arange-ldr.ll

4 ↗

(On Diff #363795)

Add a period to the end of a complete sentence.

[probinson]

In D107314#2923381, @MaskRay wrote:

LGTM, with a comment that the test does not belong to test/MC/ (which is mainly for .s tests)

Hmmm okay. test/CodeGen seems like the most appropriate place for this one.

[aeubanks]

move test
add periods

[MaskRay]

llvm/test/MC/ARM/arange-ldr.ll

1 ↗

(On Diff #363591)

Even though the test location has settled for this patch, I'd like the test/DebugInfo story be clarified.

test/DebugInfo/ has plenty of .ll tests, so I don't think it is consumer only.

[probinson]

Even though the test location has settled for this patch, I'd like the test/DebugInfo story be clarified.
test/DebugInfo/ has plenty of .ll tests, so I don't think it is consumer only.

You can start an llvm-dev thread to debate that point.

I think for this particular patch, the fix is not so much about debug info, as about getting the compiler to emit object-file sections in a way that conforms to its own internal checks; in that sense it is really a CodeGen responsibility, even if one of the sections in question happens to be a debug-info section.

llvm/include/llvm/MC/MCStreamer.h

public:
 
   virtual void emitValue(const MCExpr *Value);
 
   /// Emit the bytes in \p Data into the output.
   ///
   /// This is used to emit bytes in \p Data as sequence of .byte directives.
   virtual void emitRawBytes(StringRef Data);
 
+  virtual void emitConstantPools();
+
   virtual void finish();
 };
 
 // FIXME: declared here because it is used from
 // lib/CodeGen/AsmPrinter/ARMException.cpp.
 class ARMTargetStreamer : public MCTargetStreamer {
 public:
   ARMTargetStreamer(MCStreamer &S);
   ~ARMTargetStreamer() override;
 
   virtual void emitFnStart();
public:
   void emitTargetAttributes(const MCSubtargetInfo &STI);
   virtual void finishAttributeSection();
   virtual void emitInst(uint32_t Inst, char Suffix = '\0');
 
   virtual void AnnotateTLSDescriptorSequence(const MCSymbolRefExpr *SRE);
 
   virtual void emitThumbSet(MCSymbol *Symbol, const MCExpr *Value);
 
-  void finish() override;
+  void emitConstantPools() override;
 
   /// Reset any state between object emissions, i.e. the equivalent of
   /// MCStreamer's reset method.
   virtual void reset();
 
   /// Callback used to implement the ldr= pseudo.
   /// Add a new entry to the constant pool for the current section and return an
   /// MCExpr that can be used to refer to the constant pool location.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

if (!Stubs.empty()) {
         OutStreamer->emitSymbolAttribute(Stub.first, MCSA_Global);
         OutStreamer->emitLabel(Stub.first);
         OutStreamer->emitSymbolValue(Stub.second.getPointer(),
                                      DL.getPointerSize());
       }
     }
   }
 
+  // This needs to happen before emitting debug information since that can end
+  // arbitrary sections.
+  if (auto *TS = OutStreamer->getTargetStreamer())
+    TS->emitConstantPools();
+
   // Finalize debug and EH information.
   for (const HandlerInfo &HI : Handlers) {
     NamedRegionTimer T(HI.TimerName, HI.TimerDescription, HI.TimerGroupName,
                        HI.TimerGroupDescription, TimePassesIsEnabled);
     HI.Handler->endModule();
   }
 
   // This deletes all the ephemeral handlers that AsmPrinter added, while

llvm/lib/MC/MCParser/AsmParser.cpp

for (std::tuple<SMLoc, CppHashInfoTy, MCSymbol *> &LocSym : DirLabels) {
       if (std::get<2>(LocSym)->isUndefined()) {
         // Reset the state of any "# line file" directives we've seen to the
         // context as it was at the diagnostic site.
         CppHashInfo = std::get<1>(LocSym);
         printError(std::get<0>(LocSym), "directional label undefined");
       }
     }
   }
-
   // Finalize the output stream if there are no errors and if the client wants
   // us to.
-  if (!HadError && !NoFinalize)
+  if (!HadError && !NoFinalize) {
+    if (auto *TS = Out.getTargetStreamer())
+      TS->emitConstantPools();
+
     Out.Finish(Lexer.getLoc());
+  }
 
   return HadError || getContext().hadError();
 }
 
 bool AsmParser::checkForValidSection() {
   if (!ParsingMSInlineAsm && !getStreamer().getCurrentSectionOnly()) {
     Out.InitSections(false);
     return Error(getTok().getLoc(),

llvm/lib/MC/MCStreamer.cpp

 
 // Pin the vtables to this file.
 MCTargetStreamer::~MCTargetStreamer() = default;
 
 void MCTargetStreamer::emitLabel(MCSymbol *Symbol) {}
 
 void MCTargetStreamer::finish() {}
 
+void MCTargetStreamer::emitConstantPools() {}
+
 void MCTargetStreamer::changeSection(const MCSection *CurSection,
                                      MCSection *Section,
                                      const MCExpr *Subsection,
                                      raw_ostream &OS) {
   Section->PrintSwitchToSection(*Streamer.getContext().getAsmInfo(),
                                 Streamer.getContext().getTargetTriple(), OS,
                                 Subsection);
 }

llvm/lib/Target/AArch64/MCTargetDesc/AArch64TargetStreamer.h

 namespace llvm {
 
 class AArch64TargetStreamer : public MCTargetStreamer {
 public:
   AArch64TargetStreamer(MCStreamer &S);
   ~AArch64TargetStreamer() override;
 
   void finish() override;
+  void emitConstantPools() override;
 
   /// Callback used to implement the ldr= pseudo.
   /// Add a new entry to the constant pool for the current section and return an
   /// MCExpr that can be used to refer to the constant pool location.
   const MCExpr *addConstantPoolEntry(const MCExpr *, unsigned Size, SMLoc Loc);
 
   /// Callback used to implemnt the .ltorg directive.
   /// Emit contents of constant pool for the current section.

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

const MCExpr *AArch64TargetStreamer::addConstantPoolEntry(const MCExpr *Expr,
                                                           SMLoc Loc) {
   return ConstantPools->addEntry(Streamer, Expr, Size, Loc);
 }
 
 void AArch64TargetStreamer::emitCurrentConstantPool() {
   ConstantPools->emitForCurrentSection(Streamer);
 }
 
+void AArch64TargetStreamer::emitConstantPools() {
+  ConstantPools->emitAll(Streamer);
+}
+
 // finish() - write out any non-empty assembler constant pools and
 //   write out note.gnu.properties if need.
 void AArch64TargetStreamer::finish() {
-  ConstantPools->emitAll(Streamer);
-
   if (MarkBTIProperty)
     emitNoteSection(ELF::GNU_PROPERTY_AARCH64_FEATURE_1_BTI);
 }
 
 void AArch64TargetStreamer::emitNoteSection(unsigned Flags) {
   if (Flags == 0)
     return;
 

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

 }
 
 void ARMTargetStreamer::emitCurrentConstantPool() {
   ConstantPools->emitForCurrentSection(Streamer);
   ConstantPools->clearCacheForCurrentSection(Streamer);
 }
 
 // finish() - write out any non-empty assembler constant pools.
-void ARMTargetStreamer::finish() { ConstantPools->emitAll(Streamer); }
+void ARMTargetStreamer::emitConstantPools() {
+  ConstantPools->emitAll(Streamer);
+}
 
 // reset() - Reset any state
 void ARMTargetStreamer::reset() {}
 
 void ARMTargetStreamer::emitInst(uint32_t Inst, char Suffix) {
   unsigned Size;
   char Buffer[4];
   const bool LittleEndian = getStreamer().getContext().getAsmInfo()->isLittleEndian();

llvm/test/CodeGen/ARM/arange-ldr.ll

+; RUN: llc %s -o - -generate-arange-section | FileCheck %s
+
+; Make sure that emitting constants for ldr and emitting arange work together.
+; Emitting constants must come before emitting aranges since emitting aranges can end arbitrary sections.
+
+target datalayout = "e-m:e-p:32:32-Fi8-i64:64-v128:64:128-a:0:32-n32-S64"
+target triple = "armv7-unknown-linux-android21"
+
+; CHECK:       ldr r7, .Ltmp[[#TMP:]]
+
+; CHECK:      .Ltmp[[#TMP]]:
+; CHECK-NEXT: .long  83040
+
+; CHECK: .section        .debug_aranges
+
+define dso_local void @a() local_unnamed_addr !dbg !4 {
+entry:
+  call void asm sideeffect "  ldr r7, =${0:c}\0A", "i"(i32 83040)
+  ret void
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 14.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: LineTablesOnly, enums: !2, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "/tmp/a.c", directory: "/tmp/")
+!2 = !{}
+!3 = !{i32 2, !"Debug Info Version", i32 3}
+!4 = distinct !DISubprogram(name: "a", scope: !5, file: !5, line: 1, type: !6, scopeLine: 1, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !2)
+!5 = !DIFile(filename: "/tmp/a.c", directory: "")
+!6 = !DISubroutineType(types: !2)