MCFragment: Use trailing data for fixed-size part

[MaskRay]

The fixed-size content of the MCFragment object is now stored as
trailing data, replacing ContentStart/ContentEnd with ContentSize. The
available space for trailing data is tracked using FragSpace. If the
available space is insufficient, a new block is allocated within the
bump allocator MCObjectStreamer::FragStorage.

FragList::Tail cannot be reused when switching sections or subsections,
as it is not associated with the fragment space tracked by FragSpace.
Instead, allocate a new fragment, which becomes less expensive after #150574.

Data can only be appended to the tail fragment of a subsection, not to
fragments in the middle. Post-assembler-layout adjustments (such as
.llvm_addrsig and .llvm.call-graph-profile) have been updated to use the
variable-size part instead.

[llvmbot]

@llvm/pr-subscribers-mc

@llvm/pr-subscribers-backend-mips

Author: Fangrui Song (MaskRay)

Changes

The fixed-size content of the MCFragment object is now stored as
trailing data, replacing ContentStart/ContentEnd with ContentSize. The
available space for trailing data is tracked using FragSpace. If the
available space is insufficient, a new block is allocated within the
bump allocator MCObjectStreamer::FragStorage.

When switching section or subsection, we cannot reuse the
FragList::Tail, because it is no associated with fragment space tracked
by FragSpace. We need to allocate a new fragment, which was less
expensive after #150574.

Data can only be appended to the tail fragment of a subsection, not to
fragments in the middle. Post-assembler-layout adjustments (such as
.llvm_addrsig and .llvm.call-graph-profile) have been updated to use the
variable-size part instead.

---

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

7 Files Affected:

• (modified) llvm/include/llvm/MC/MCObjectStreamer.h (+10)
• (modified) llvm/include/llvm/MC/MCSection.h (+5-35)
• (modified) llvm/lib/MC/MCMachOStreamer.cpp (+3-2)
• (modified) llvm/lib/MC/MCObjectStreamer.cpp (+107-30)
• (modified) llvm/lib/MC/MCWin64EH.cpp (+2)
• (modified) llvm/lib/MC/MachObjectWriter.cpp (+1-1)
• (modified) llvm/lib/Target/Mips/MCTargetDesc/MipsTargetStreamer.cpp (+6)

diff --git a/llvm/include/llvm/MC/MCObjectStreamer.h b/llvm/include/llvm/MC/MCObjectStreamer.h
index bbbee15abf700..b6e24816d94db 100644
--- a/llvm/include/llvm/MC/MCObjectStreamer.h
+++ b/llvm/include/llvm/MC/MCObjectStreamer.h
@@ -52,6 +52,10 @@ class MCObjectStreamer : public MCStreamer {
   DenseMap<const MCSymbol *, SmallVector<PendingAssignment, 1>>
       pendingAssignments;
 
+  SmallVector<std::unique_ptr<char[]>, 0> FragStorage;
+  // Available bytes in the current block for trailing data or new fragments.
+  size_t FragSpace = 0;
+
   void emitInstToData(const MCInst &Inst, const MCSubtargetInfo &);
   void emitCFIStartProcImpl(MCDwarfFrameInfo &Frame) override;
   void emitCFIEndProcImpl(MCDwarfFrameInfo &Frame) override;
@@ -84,9 +88,15 @@ class MCObjectStreamer : public MCStreamer {
   // Add a fragment with a variable-size tail and start a new empty fragment.
   void insert(MCFragment *F);
 
+  char *getCurFragEnd() const {
+    return reinterpret_cast<char *>(CurFrag + 1) + CurFrag->getFixedSize();
+  }
+  MCFragment *allocFragSpace(size_t Headroom);
   // Add a new fragment to the current section without a variable-size tail.
   void newFragment();
 
+  void ensureHeadroom(size_t Headroom);
+  void appendContents(ArrayRef<char> Contents);
   void appendContents(size_t Num, char Elt);
   void addFixup(const MCExpr *Value, MCFixupKind Kind);
 
diff --git a/llvm/include/llvm/MC/MCSection.h b/llvm/include/llvm/MC/MCSection.h
index 7989310e5a8f2..1579fa6646c3c 100644
--- a/llvm/include/llvm/MC/MCSection.h
+++ b/llvm/include/llvm/MC/MCSection.h
@@ -93,8 +93,7 @@ class MCFragment {
   // Track content and fixups for the fixed-size part as fragments are
   // appended to the section. The content remains immutable, except when
   // modified by applyFixup.
-  uint32_t ContentStart = 0;
-  uint32_t ContentEnd = 0;
+  uint32_t FixedSize = 0;
   uint32_t FixupStart = 0;
   uint32_t FixupEnd = 0;
 
@@ -205,18 +204,6 @@ class MCFragment {
   //== Content-related functions manage parent's storage using ContentStart and
   // ContentSize.
 
-  // Get a SmallVector reference. The caller should call doneAppending to update
-  // `ContentEnd`.
-  SmallVectorImpl<char> &getContentsForAppending();
-  void doneAppending();
-  void appendContents(ArrayRef<char> Contents) {
-    getContentsForAppending().append(Contents.begin(), Contents.end());
-    doneAppending();
-  }
-  void appendContents(size_t Num, char Elt) {
-    getContentsForAppending().append(Num, Elt);
-    doneAppending();
-  }
   MutableArrayRef<char> getContents();
   ArrayRef<char> getContents() const;
 
@@ -225,10 +212,10 @@ class MCFragment {
   MutableArrayRef<char> getVarContents();
   ArrayRef<char> getVarContents() const;
 
-  size_t getFixedSize() const { return ContentEnd - ContentStart; }
+  size_t getFixedSize() const { return FixedSize; }
   size_t getVarSize() const { return VarContentEnd - VarContentStart; }
   size_t getSize() const {
-    return ContentEnd - ContentStart + (VarContentEnd - VarContentStart);
+    return FixedSize + (VarContentEnd - VarContentStart);
   }
 
   //== Fixup-related functions manage parent's storage using FixupStart and
@@ -651,28 +638,11 @@ class LLVM_ABI MCSection {
   bool isBssSection() const { return IsBss; }
 };
 
-inline SmallVectorImpl<char> &MCFragment::getContentsForAppending() {
-  SmallVectorImpl<char> &S = getParent()->ContentStorage;
-  if (LLVM_UNLIKELY(ContentEnd != S.size())) {
-    // Move the elements to the end. Reserve space to avoid invalidating
-    // S.begin()+I for `append`.
-    auto Size = ContentEnd - ContentStart;
-    auto I = std::exchange(ContentStart, S.size());
-    S.reserve(S.size() + Size);
-    S.append(S.begin() + I, S.begin() + I + Size);
-  }
-  return S;
-}
-inline void MCFragment::doneAppending() {
-  ContentEnd = getParent()->ContentStorage.size();
-}
 inline MutableArrayRef<char> MCFragment::getContents() {
-  return MutableArrayRef(getParent()->ContentStorage)
-      .slice(ContentStart, ContentEnd - ContentStart);
+  return {reinterpret_cast<char *>(this + 1), FixedSize};
 }
 inline ArrayRef<char> MCFragment::getContents() const {
-  return ArrayRef(getParent()->ContentStorage)
-      .slice(ContentStart, ContentEnd - ContentStart);
+  return {reinterpret_cast<const char *>(this + 1), FixedSize};
 }
 
 inline MutableArrayRef<char> MCFragment::getVarContents() {
diff --git a/llvm/lib/MC/MCMachOStreamer.cpp b/llvm/lib/MC/MCMachOStreamer.cpp
index 107466912d090..1c5abd6ddacc7 100644
--- a/llvm/lib/MC/MCMachOStreamer.cpp
+++ b/llvm/lib/MC/MCMachOStreamer.cpp
@@ -484,7 +484,8 @@ void MCMachOStreamer::finalizeCGProfile() {
   // For each entry, reserve space for 2 32-bit indices and a 64-bit count.
   size_t SectionBytes =
       W.getCGProfile().size() * (2 * sizeof(uint32_t) + sizeof(uint64_t));
-  (*CGProfileSection->begin()).appendContents(SectionBytes, 0);
+  (*CGProfileSection->begin())
+      .setVarContents(std::vector<char>(SectionBytes, 0));
 }
 
 MCStreamer *llvm::createMachOStreamer(MCContext &Context,
@@ -520,5 +521,5 @@ void MCMachOStreamer::createAddrSigSection() {
   // (instead of emitting a zero-sized section) so these relocations are
   // technically valid, even though we don't expect these relocations to
   // actually be applied by the linker.
-  Frag->appendContents(8, 0);
+  Frag->setVarContents(std::vector<char>(8, 0));
 }
diff --git a/llvm/lib/MC/MCObjectStreamer.cpp b/llvm/lib/MC/MCObjectStreamer.cpp
index 9c7b05bd9a45a..28293501fb8e6 100644
--- a/llvm/lib/MC/MCObjectStreamer.cpp
+++ b/llvm/lib/MC/MCObjectStreamer.cpp
@@ -46,23 +46,84 @@ MCAssembler *MCObjectStreamer::getAssemblerPtr() {
   return nullptr;
 }
 
+constexpr size_t FragChunkSize = 16384;
+// Ensure the new fragment can at least at least a few bytes.
+constexpr size_t NewFragHeadroom = 8;
+
+static_assert(NewFragHeadroom >= alignof(MCFragment));
+static_assert(FragChunkSize >= sizeof(MCFragment) + NewFragHeadroom);
+
+MCFragment *MCObjectStreamer::allocFragSpace(size_t Headroom) {
+  auto Size = std::max(FragChunkSize, sizeof(MCFragment) + Headroom);
+  FragSpace = Size - sizeof(MCFragment);
+  auto Chunk = std::unique_ptr<char[]>(new char[Size]);
+  auto *F = reinterpret_cast<MCFragment *>(Chunk.get());
+  FragStorage.push_back(std::move(Chunk));
+  return F;
+}
+
 void MCObjectStreamer::newFragment() {
-  addFragment(getContext().allocFragment<MCFragment>());
+  MCFragment *F;
+  if (LLVM_LIKELY(sizeof(MCFragment) + NewFragHeadroom <= FragSpace)) {
+    auto End = size_t(getCurFragEnd());
+    F = reinterpret_cast<MCFragment *>(
+        alignToPowerOf2(End, alignof(MCFragment)));
+    FragSpace -= size_t(F) - End + sizeof(MCFragment);
+  } else {
+    F = allocFragSpace(0);
+  }
+  new (F) MCFragment();
+  addFragment(F);
+}
+
+void MCObjectStreamer::ensureHeadroom(size_t Headroom) {
+  if (Headroom <= FragSpace)
+    return;
+  auto *F = allocFragSpace(Headroom);
+  new (F) MCFragment();
+  addFragment(F);
 }
 
-void MCObjectStreamer::insert(MCFragment *F) {
-  assert(F->getKind() != MCFragment::FT_Data &&
+void MCObjectStreamer::insert(MCFragment *Frag) {
+  assert(Frag->getKind() != MCFragment::FT_Data &&
          "F should have a variable-size tail");
+  // Allocate an empty fragment, potentially reusing the space associated with
+  // CurFrag.
+  MCFragment *F;
+  if (LLVM_LIKELY(sizeof(MCFragment) + NewFragHeadroom <= FragSpace)) {
+    auto End = size_t(getCurFragEnd());
+    F = reinterpret_cast<MCFragment *>(
+        alignToPowerOf2(End, alignof(MCFragment)));
+    FragSpace -= size_t(F) - End + sizeof(MCFragment);
+  } else {
+    F = allocFragSpace(0);
+  }
+
+  // Add Frag, which destroys CurFrag and the associated space.
+  addFragment(Frag);
+  new (F) MCFragment();
+  // Add the empty fragment, which restores CurFrag and the associated space.
   addFragment(F);
-  newFragment();
+}
+
+void MCObjectStreamer::appendContents(ArrayRef<char> Contents) {
+  ensureHeadroom(Contents.size());
+  assert(FragSpace >= Contents.size());
+  llvm::copy(Contents, getCurFragEnd());
+  CurFrag->FixedSize += Contents.size();
+  FragSpace -= Contents.size();
 }
 
 void MCObjectStreamer::appendContents(size_t Num, char Elt) {
-  CurFrag->appendContents(Num, Elt);
+  ensureHeadroom(Num);
+  MutableArrayRef<char> Data(getCurFragEnd(), Num);
+  llvm::fill(Data, Elt);
+  CurFrag->FixedSize += Num;
+  FragSpace -= Num;
 }
 
 void MCObjectStreamer::addFixup(const MCExpr *Value, MCFixupKind Kind) {
-  CurFrag->addFixup(MCFixup::create(CurFrag->getFixedSize(), Value, Kind));
+  CurFrag->addFixup(MCFixup::create(getCurFragSize(), Value, Kind));
 }
 
 // As a compile-time optimization, avoid allocating and evaluating an MCExpr
@@ -111,6 +172,8 @@ void MCObjectStreamer::reset() {
   }
   EmitEHFrame = true;
   EmitDebugFrame = false;
+  FragStorage.clear();
+  FragSpace = 0;
   MCStreamer::reset();
 }
 
@@ -139,7 +202,6 @@ void MCObjectStreamer::emitCFISections(bool EH, bool Debug, bool SFrame) {
 void MCObjectStreamer::emitValueImpl(const MCExpr *Value, unsigned Size,
                                      SMLoc Loc) {
   MCStreamer::emitValueImpl(Value, Size, Loc);
-  MCFragment *DF = getCurrentFragment();
 
   MCDwarfLineEntry::make(this, getCurrentSectionOnly());
 
@@ -154,9 +216,9 @@ void MCObjectStreamer::emitValueImpl(const MCExpr *Value, unsigned Size,
     emitIntValue(AbsValue, Size);
     return;
   }
-  DF->addFixup(MCFixup::create(DF->getContents().size(), Value,
-                               MCFixup::getDataKindForSize(Size)));
-  DF->appendContents(Size, 0);
+  ensureHeadroom(Size);
+  addFixup(Value, MCFixup::getDataKindForSize(Size));
+  appendContents(Size, 0);
 }
 
 MCSymbol *MCObjectStreamer::emitCFILabel() {
@@ -190,7 +252,7 @@ void MCObjectStreamer::emitLabel(MCSymbol *Symbol, SMLoc Loc) {
   // section.
   MCFragment *F = CurFrag;
   Symbol->setFragment(F);
-  Symbol->setOffset(F->getContents().size());
+  Symbol->setOffset(F->getFixedSize());
 
   emitPendingAssignments(Symbol);
 }
@@ -256,6 +318,21 @@ void MCObjectStreamer::changeSection(MCSection *Section, uint32_t Subsection) {
     F0 = CurFrag;
   }
 
+  // CurFrag will be changed. To ensure that FragSpace remains connected with
+  // CurFrag, allocate an empty fragment and add it to the fragment list.
+  // (Subsections[I].second.Tail is disconnected with FragSpace.)
+  MCFragment *F;
+  if (LLVM_LIKELY(sizeof(MCFragment) + NewFragHeadroom <= FragSpace)) {
+    auto End = size_t(getCurFragEnd());
+    F = reinterpret_cast<MCFragment *>(
+        alignToPowerOf2(End, alignof(MCFragment)));
+    FragSpace -= size_t(F) - End + sizeof(MCFragment);
+  } else {
+    F = allocFragSpace(0);
+  }
+  new (F) MCFragment();
+  F->setParent(Section);
+
   auto &Subsections = Section->Subsections;
   size_t I = 0, E = Subsections.size();
   while (I != E && Subsections[I].first < Subsection)
@@ -263,13 +340,16 @@ void MCObjectStreamer::changeSection(MCSection *Section, uint32_t Subsection) {
   // If the subsection number is not in the sorted Subsections list, create a
   // new fragment list.
   if (I == E || Subsections[I].first != Subsection) {
-    auto *F = getContext().allocFragment<MCFragment>();
-    F->setParent(Section);
     Subsections.insert(Subsections.begin() + I,
                        {Subsection, MCSection::FragList{F, F}});
+    Section->CurFragList = &Subsections[I].second;
+    CurFrag = F;
+  } else {
+    Section->CurFragList = &Subsections[I].second;
+    CurFrag = Subsections[I].second.Tail;
+    // Ensure CurFrag is associated with FragSpace.
+    addFragment(F);
   }
-  Section->CurFragList = &Subsections[I].second;
-  CurFrag = Section->CurFragList->Tail;
 
   // Define the section symbol at subsection 0's initial fragment if required.
   if (!NewSec)
@@ -340,11 +420,11 @@ void MCObjectStreamer::emitInstToData(const MCInst &Inst,
   MCFragment *F = getCurrentFragment();
 
   // Append the instruction to the data fragment.
-  size_t CodeOffset = F->getContents().size();
+  size_t CodeOffset = getCurFragSize();
+  SmallString<16> Content;
   SmallVector<MCFixup, 1> Fixups;
-  getAssembler().getEmitter().encodeInstruction(
-      Inst, F->getContentsForAppending(), Fixups, STI);
-  F->doneAppending();
+  getAssembler().getEmitter().encodeInstruction(Inst, Content, Fixups, STI);
+  appendContents(Content);
   F->setHasInstructions(STI);
 
   if (Fixups.empty())
@@ -352,19 +432,17 @@ void MCObjectStreamer::emitInstToData(const MCInst &Inst,
   bool MarkedLinkerRelaxable = false;
   for (auto &Fixup : Fixups) {
     Fixup.setOffset(Fixup.getOffset() + CodeOffset);
-    if (!Fixup.isLinkerRelaxable() || MarkedLinkerRelaxable)
+    if (!Fixup.isLinkerRelaxable())
       continue;
-    MarkedLinkerRelaxable = true;
-    // Set the fragment's order within the subsection for use by
-    // MCAssembler::relaxAlign.
-    auto *Sec = F->getParent();
-    if (!Sec->isLinkerRelaxable())
-      Sec->setLinkerRelaxable();
+    F->setLinkerRelaxable();
     // Do not add data after a linker-relaxable instruction. The difference
     // between a new label and a label at or before the linker-relaxable
     // instruction cannot be resolved at assemble-time.
-    F->setLinkerRelaxable();
-    newFragment();
+    if (!MarkedLinkerRelaxable) {
+      MarkedLinkerRelaxable = true;
+      getCurrentSectionOnly()->setLinkerRelaxable();
+      newFragment();
+    }
   }
   F->appendFixups(Fixups);
 }
@@ -538,8 +616,7 @@ void MCObjectStreamer::emitCVFileChecksumOffsetDirective(unsigned FileNo) {
 
 void MCObjectStreamer::emitBytes(StringRef Data) {
   MCDwarfLineEntry::make(this, getCurrentSectionOnly());
-  MCFragment *DF = getCurrentFragment();
-  DF->appendContents(ArrayRef(Data.data(), Data.size()));
+  appendContents(ArrayRef(Data.data(), Data.size()));
 }
 
 void MCObjectStreamer::emitValueToAlignment(Align Alignment, int64_t Fill,
diff --git a/llvm/lib/MC/MCWin64EH.cpp b/llvm/lib/MC/MCWin64EH.cpp
index 72a8dd7031198..e212663d53a79 100644
--- a/llvm/lib/MC/MCWin64EH.cpp
+++ b/llvm/lib/MC/MCWin64EH.cpp
@@ -318,6 +318,8 @@ static void EmitUnwindInfo(MCStreamer &streamer, WinEH::FrameInfo *info) {
 
   // Emit the epilog instructions.
   if (EnableUnwindV2) {
+    OS->ensureHeadroom(info->EpilogMap.size() * 2);
+
     bool IsLast = true;
     for (const auto &Epilog : llvm::reverse(info->EpilogMap)) {
       if (IsLast) {
diff --git a/llvm/lib/MC/MachObjectWriter.cpp b/llvm/lib/MC/MachObjectWriter.cpp
index 7b5c3c00f2519..e87696a93cddd 100644
--- a/llvm/lib/MC/MachObjectWriter.cpp
+++ b/llvm/lib/MC/MachObjectWriter.cpp
@@ -806,7 +806,7 @@ uint64_t MachObjectWriter::writeObject() {
     }
     MCSection *Sec = getContext().getMachOSection("__LLVM", "__cg_profile", 0,
                                                   SectionKind::getMetadata());
-    llvm::copy(OS.str(), Sec->curFragList()->Head->getContents().data());
+    llvm::copy(OS.str(), Sec->curFragList()->Head->getVarContents().data());
   }
 
   unsigned NumSections = Asm.end() - Asm.begin();
diff --git a/llvm/lib/Target/Mips/MCTargetDesc/MipsTargetStreamer.cpp b/llvm/lib/Target/Mips/MCTargetDesc/MipsTargetStreamer.cpp
index d9680c7739a1b..7a8395a2e582b 100644
--- a/llvm/lib/Target/Mips/MCTargetDesc/MipsTargetStreamer.cpp
+++ b/llvm/lib/Target/Mips/MCTargetDesc/MipsTargetStreamer.cpp
@@ -1034,12 +1034,14 @@ MCELFStreamer &MipsTargetELFStreamer::getStreamer() {
 
 void MipsTargetELFStreamer::emitGPRel32Value(const MCExpr *Value) {
   auto &S = getStreamer();
+  S.ensureHeadroom(4);
   S.addFixup(Value, Mips::fixup_Mips_GPREL32);
   S.appendContents(4, 0);
 }
 
 void MipsTargetELFStreamer::emitGPRel64Value(const MCExpr *Value) {
   auto &S = getStreamer();
+  S.ensureHeadroom(8);
   // fixup_Mips_GPREL32 desginates R_MIPS_GPREL32+R_MIPS_64 on MIPS64.
   S.addFixup(Value, Mips::fixup_Mips_GPREL32);
   S.appendContents(8, 0);
@@ -1047,24 +1049,28 @@ void MipsTargetELFStreamer::emitGPRel64Value(const MCExpr *Value) {
 
 void MipsTargetELFStreamer::emitDTPRel32Value(const MCExpr *Value) {
   auto &S = getStreamer();
+  S.ensureHeadroom(4);
   S.addFixup(Value, Mips::fixup_Mips_DTPREL32);
   S.appendContents(4, 0);
 }
 
 void MipsTargetELFStreamer::emitDTPRel64Value(const MCExpr *Value) {
   auto &S = getStreamer();
+  S.ensureHeadroom(8);
   S.addFixup(Value, Mips::fixup_Mips_DTPREL64);
   S.appendContents(8, 0);
 }
 
 void MipsTargetELFStreamer::emitTPRel32Value(const MCExpr *Value) {
   auto &S = getStreamer();
+  S.ensureHeadroom(4);
   S.addFixup(Value, Mips::fixup_Mips_TPREL32);
   S.appendContents(4, 0);
 }
 
 void MipsTargetELFStreamer::emitTPRel64Value(const MCExpr *Value) {
   auto &S = getStreamer();
+  S.ensureHeadroom(8);
   S.addFixup(Value, Mips::fixup_Mips_TPREL64);
   S.appendContents(8, 0);
 }

[MaskRay]

While I am still waiting for results on https://llvm-compile-time-tracker.com/ , I think this is in good shape for review (thanks a lot for past reviews and suggestions!)

[MaskRay]

minuscule instructions:u decrease for stage2-O0-g:

https://llvm-compile-time-tracker.com/compare.php?from=85213f2f884cd68586723163629474a8eea46a74&to=847901f5d28cc25ead3183c6b08b3d6840d3ac95&stat=instructions:u

(Is there a convenient way to generate markdown from the HTML code? The link may be dead in the future and we ought to paste a markdown. For now I am using https://jmalarcon.github.io/markdowntables/ )

Benchmark	Old	New
kimwitu++	22211M	22209M (-0.01%)
sqlite3	3944M	3948M (+0.10%)
consumer-typeset	11003M	11001M (-0.02%)
Bullet	58147M	58092M (-0.09%)
tramp3d-v4	18668M	18629M (-0.21%)
mafft	5891M	5889M (-0.03%)
ClamAV	12101M	12092M (-0.08%)
lencod	11149M	11141M (-0.07%)
SPASS	12719M	12706M (-0.10%)
7zip	128392M	128305M (-0.07%)
geomean	16341M	16332M (-0.06%)

[MaskRay]

The uint32_t FixedSize member saving does not lead to sizeof(MCFragment) saving due to a 4-byte padding after VarFixupEnd

[aengelke]

Looks good, some minor comments. This is a clever but also complex design, it might be worth documenting the new fragment layout somewhere.

[aengelke]

constexpr array, no need for a heap allocation here.

[aengelke]

We could consider growing the size exponentially, similar to what BumpPtrAllocator does. This should reduce the number of individual heap allocations for large files.

[MaskRay]

Here is the statistics of the Headroom parameter when compiling sqlite3.i with -O0 -g -w

0 181
1 12
2 2
3 3
4 12
5 2
6 1
7 2
8 3
10 1
11 1
1347 1

The smaller values mostly come from encodeInstruction. 1347 is from a AsmPrinter emitting a large string (.asciz), which is followed by many small writes. I believe 2*Headroom is not necessary.

[aengelke]

reinterpret_cast<uintptr_t>?

[aengelke]

This looks like some kind of change that can easily cause hard-to-diagnose bugs when missed. This seems hard to catch with assertions, though, and I've no good idea how to improve this.

[MaskRay]

I've replaced MCFragment::appendContents with MCObjectStreamer::appendContents, which calls ensureHeadroom internally. Therefore, the ensureHeadroom is redundant. I'll remove them.

MCFragment::appendContents has been removed. Hopefully it makes harder to make a mistake.

[MaskRay]

While we could replace uint32_t VarFixupEnd with uint8_t VarFixupSize to make MCFragment smaller, I will try to make fixups detached from MCFragment.

edit:
CodeViewContext::encodeDefRange might create many VarFixups. I don't know whether it's bound by 255.

[chelcassanova]

@MaskRay This change and 3 others related to it (2bebbe1, 769b0e6 and 96a9e8c) looks to have broken the sanitized build on LLDB GreenDragon and is causing 9 tests to fail due to crashes: https://green.lab.llvm.org/job/llvm.org/view/LLDB/job/lldb-cmake-sanitized/2001/console

Failed Tests (9):
07:11:04 lldb-shell :: Commands/command-disassemble.s
07:11:04 lldb-shell :: ScriptInterpreter/Python/sb_function_ranges.s
07:11:04 lldb-shell :: SymbolFile/DWARF/x86/discontinuous-function.s
07:11:04 lldb-shell :: SymbolFile/DWARF/x86/member-pointers.cpp
07:11:04 lldb-shell :: SymbolFile/DWARF/x86/unused-inlined-params.test
07:11:04 lldb-shell :: SymbolFile/NativePDB/blocks.s
07:11:04 lldb-shell :: SymbolFile/NativePDB/incomplete-tag-type.cpp
07:11:04 lldb-shell :: SymbolFile/NativePDB/inline_sites.test
07:11:04 lldb-shell :: SymbolFile/NativePDB/tag-types.cpp

PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
07:04:17  Stack dump:
07:04:17  0.	Program arguments: /Users/ec2-user/jenkins/workspace/llvm.org/lldb-cmake-sanitized/lldb-build/bin/clang --driver-mode=cl --target=specify-a-target-or-use-a-_host-substitution --target=x86_64-windows-msvc /O1 /Z7 -c /Fo/Users/ec2-user/jenkins/workspace/llvm.org/lldb-cmake-sanitized/lldb-build/tools/lldb/test/Shell/SymbolFile/NativePDB/Output/incomplete-tag-type.cpp.tmp2.obj -- /Users/ec2-user/jenkins/workspace/llvm.org/lldb-cmake-sanitized/llvm-project/lldb/test/Shell/SymbolFile/NativePDB/incomplete-tag-type.cpp
07:04:17  1.	<eof> parser at end of file
07:04:17  2.	Code generation
07:04:17   #0 0x00000001138f8a80 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/Users/ec2-user/jenkins/workspace/llvm.org/lldb-cmake-sanitized/lldb-build/bin/clang-22+0x10ef8ca80)
.
.
.
07:04:17  #11 0x0000000112a765b0 llvm::MCAssembler::layout() (/Users/ec2-user/jenkins/workspace/llvm.org/lldb-cmake-sanitized/lldb-build/bin/clang-22+0x10e10a5b0)
07:04:17  #12 0x0000000112a77ec4 llvm::MCAssembler::Finish() (/Users/ec2-user/jenkins/workspace/llvm.org/lldb-cmake-sanitized/lldb-build/bin/clang-22+0x10e10bec4)
07:04:17  #13 0x0000000112b69104 llvm::MCStreamer::finish(llvm::SMLoc) (/Users/ec2-user/jenkins/workspace/llvm.org/lldb-cmake-sanitized/lldb-build/bin/clang-22+0x10e1fd104)

[npanchen]

@MaskRay our pulldown also identified same problem as @chelcassanova reported above.
That reminded me of #148544 (comment) issue of ~2w ago right in the same place and it seems make more sense to add an assert there.
Are you working on a fix or this commit can be reverted for now ?

[chelcassanova]

@MaskRay We've identified that this specific commit is the one that breaks our sanitized test suite, can this commit be reverted in isolation without disrupting the build given the other changes made to the MC layer?

[npanchen]

#151383 reverts that change for now

[MaskRay]

@MaskRay our pulldown also identified same problem as @chelcassanova reported above. That reminded me of #148544 (comment) issue of ~2w ago right in the same place and it seems make more sense to add an assert there. Are you working on a fix or this commit can be reverted for now ?

Thanks for skipping the emitInstToData change that this patch accidentally changed.

To reland this patch, avoiding the pointer-overflow sanitizer issue, I will replace MutableArrayRef<char> Data (fragment content) with char *Data (just the relocated location) in

llvm-project/llvm/include/llvm/MC/MCAsmBackend.h

Line 126 in 228e96b

const MCValue &Target, MutableArrayRef<char> Data,

The backend applyFixup asserts should be changed to assert(Offset + NumBytes <= F.getSize() && "Invalid fixup offset!"); to not rely on the size field (otherwise unused).

[MaskRay]

For the complex aarch64 Linux kernel ClangBuiltLinux/linux#2116 ,
"MCFragment: Use trailing data for fixed-size part" creates some 0-size Data fragments, which triggers an edge case that is not correctly handled by MCAssembler::layout. Adding an extra relaxOnce and layoutSection makes the test pass. I will try improving the layout algorithm.

I reverted the commit a2fef66 ("MCFragment: Use trailing data for fixed-size part") as a workaround.

[DavidSpickett]

The new test is failing on Linaro's 32-bit builders:

******************** TEST 'LLVM :: MC/ELF/many-instructions.s' FAILED ********************
Exit Code: 1
Command Output (stderr):
--
/home/tcwg-buildbot/worker/clang-armv7-global-isel/stage1/bin/llvm-mc -filetype=obj -triple=x86_64 /home/tcwg-buildbot/worker/clang-armv7-global-isel/llvm/llvm/test/MC/ELF/many-instructions.s -o /dev/null -debug-only=mc-dump 2>&1 | grep -E -o '[0-9]+ Data Size:[0-9]+' | /home/tcwg-buildbot/worker/clang-armv7-global-isel/stage1/bin/FileCheck /home/tcwg-buildbot/worker/clang-armv7-global-isel/llvm/llvm/test/MC/ELF/many-instructions.s # RUN: at line 2
+ /home/tcwg-buildbot/worker/clang-armv7-global-isel/stage1/bin/llvm-mc -filetype=obj -triple=x86_64 /home/tcwg-buildbot/worker/clang-armv7-global-isel/llvm/llvm/test/MC/ELF/many-instructions.s -o /dev/null -debug-only=mc-dump
+ grep -E -o '[0-9]+ Data Size:[0-9]+'
+ /home/tcwg-buildbot/worker/clang-armv7-global-isel/stage1/bin/FileCheck /home/tcwg-buildbot/worker/clang-armv7-global-isel/llvm/llvm/test/MC/ELF/many-instructions.s
/home/tcwg-buildbot/worker/clang-armv7-global-isel/llvm/llvm/test/MC/ELF/many-instructions.s:5:10: error: CHECK: expected string not found in input
# CHECK: 0 Data Size:16220
         ^
<stdin>:1:1: note: scanning from here
0 Data Size:16240
^
Input file: <stdin>
Check file: /home/tcwg-buildbot/worker/clang-armv7-global-isel/llvm/llvm/test/MC/ELF/many-instructions.s
-dump-input=help explains the following input dump.
Input was:
<<<<<<
         1: 0 Data Size:16240 
check:5     X~~~~~~~~~~~~~~~~~ error: no match found
         2: 16240 Data Size:140 
check:5     ~~~~~~~~~~~~~~~~~~~~
>>>>>>
--

https://lab.llvm.org/buildbot/#/builders/39/builds/7363 is one example.

I think these stats are the size of structures inside of MC, which would mean they could change size if the ABI is different? (we only have 32-bit Arm builds, not 32-bit x86 so idk if it's a problem there)

[MaskRay]

The new test is failing on Linaro's 32-bit builders:

******************** TEST 'LLVM :: MC/ELF/many-instructions.s' FAILED ********************
Exit Code: 1
Command Output (stderr):
--
/home/tcwg-buildbot/worker/clang-armv7-global-isel/stage1/bin/llvm-mc -filetype=obj -triple=x86_64 /home/tcwg-buildbot/worker/clang-armv7-global-isel/llvm/llvm/test/MC/ELF/many-instructions.s -o /dev/null -debug-only=mc-dump 2>&1 | grep -E -o '[0-9]+ Data Size:[0-9]+' | /home/tcwg-buildbot/worker/clang-armv7-global-isel/stage1/bin/FileCheck /home/tcwg-buildbot/worker/clang-armv7-global-isel/llvm/llvm/test/MC/ELF/many-instructions.s # RUN: at line 2
+ /home/tcwg-buildbot/worker/clang-armv7-global-isel/stage1/bin/llvm-mc -filetype=obj -triple=x86_64 /home/tcwg-buildbot/worker/clang-armv7-global-isel/llvm/llvm/test/MC/ELF/many-instructions.s -o /dev/null -debug-only=mc-dump
+ grep -E -o '[0-9]+ Data Size:[0-9]+'
+ /home/tcwg-buildbot/worker/clang-armv7-global-isel/stage1/bin/FileCheck /home/tcwg-buildbot/worker/clang-armv7-global-isel/llvm/llvm/test/MC/ELF/many-instructions.s
/home/tcwg-buildbot/worker/clang-armv7-global-isel/llvm/llvm/test/MC/ELF/many-instructions.s:5:10: error: CHECK: expected string not found in input
# CHECK: 0 Data Size:16220
         ^
<stdin>:1:1: note: scanning from here
0 Data Size:16240
^
Input file: <stdin>
Check file: /home/tcwg-buildbot/worker/clang-armv7-global-isel/llvm/llvm/test/MC/ELF/many-instructions.s
-dump-input=help explains the following input dump.
Input was:
<<<<<<
         1: 0 Data Size:16240 
check:5     X~~~~~~~~~~~~~~~~~ error: no match found
         2: 16240 Data Size:140 
check:5     ~~~~~~~~~~~~~~~~~~~~
>>>>>>
--

lab.llvm.org/buildbot#/builders/39/builds/7363 is one example.

I think these stats are the size of structures inside of MC, which would mean they could change size if the ABI is different? (we only have 32-bit Arm builds, not 32-bit x86 so idk if it's a problem there)

Yes. sizeof(MCFragment) differences can change the fragment splitting behavior. We should add an REQUIRES that this is only for 64-bit platforms for simplicity.

[DavidSpickett]

Understood. I'll get that done today.

[MaskRay]

Thanks for adding REQUIRES: llvm-64-bits! Nice that we have a dedicated lit feature.