Skip to content

Commit

Permalink
[MC] Remove pending labels
Browse files Browse the repository at this point in the history 
This commit removes the complexity introduced by pending labels in
https://reviews.llvm.org/D5915 by using a simpler approach. D5915 aimed
to ensure padding placement before `.Ltmp0` for the following code, but
at the cost of expensive per-instruction `flushPendingLabels`.

```
// similar to llvm/test/MC/X86/AlignedBundling/labeloffset.s
.bundle_lock align_to_end
  calll   .L0$pb
.bundle_unlock
.L0$pb:
  popl    %eax
.Ltmp0:   //// padding should be inserted before this label instead of after
  addl    $_GLOBAL_OFFSET_TABLE_+(.Ltmp0-.L0$pb), %eax
```

(D5915 was adjusted by https://reviews.llvm.org/D8072 and
https://reviews.llvm.org/D71368)

This patch achieves the same goal by setting the offset of the empty
MCDataFragment (`Prev`) in `layoutBundle`. This eliminates the need for
pending labels and simplifies the code.

llvm/test/MC/MachO/pending-labels.s (D71368): relocation symbols are
changed, but the result is still supported by linkers.
  • Loading branch information
@MaskRay
MaskRay committed Jun 22, 2024
1 parent a091bfe commit 7500646
Show file tree
Hide file tree
Showing 8 changed files with 23 additions and 157 deletions.
2 changes: 1 addition & 1 deletion llvm/include/llvm/MC/MCAsmLayout.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ class MCAsmLayout {
/// its bundle padding will be recomputed.
void invalidateFragmentsFrom(MCFragment *F);

void layoutBundle(MCFragment *F);
void layoutBundle(MCFragment *Prev, MCFragment *F);

/// \name Section Access (in layout order)
/// @{
Expand Down
12 changes: 1 addition & 11 deletions llvm/include/llvm/MC/MCObjectStreamer.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,6 @@ class MCObjectStreamer : public MCStreamer {
MCFragment *getCurrentFragment() const;

void insert(MCFragment *F) {
flushPendingLabels(F);
MCSection *CurSection = getCurrentSectionOnly();
CurSection->addFragment(*F);
F->setParent(CurSection);
Expand All @@ -106,24 +105,15 @@ class MCObjectStreamer : public MCStreamer {
protected:
bool changeSectionImpl(MCSection *Section, const MCExpr *Subsection);

/// Assign a label to the current Section and Subsection even though a
/// fragment is not yet present. Use flushPendingLabels(F) to associate
/// a fragment with this label.
void addPendingLabel(MCSymbol* label);

/// If any labels have been emitted but not assigned fragments in the current
/// Section and Subsection, ensure that they get assigned to fragment F.
/// Optionally, one can provide an offset \p FOffset as a symbol offset within
/// the fragment.
void flushPendingLabels(MCFragment *F, uint64_t FOffset = 0);
void flushPendingLabels(MCFragment *F, uint64_t FOffset = 0) {}

public:
void visitUsedSymbol(const MCSymbol &Sym) override;

/// Create a data fragment for any pending labels across all Sections
/// and Subsections.
void flushPendingLabels();

MCAssembler &getAssembler() { return *Assembler; }
MCAssembler *getAssemblerPtr() override;
/// \name MCStreamer Interface
Expand Down
7 changes: 0 additions & 7 deletions llvm/include/llvm/MC/MCSection.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -226,13 +226,6 @@ class MCSection {
/// Add a pending label for the requested subsection. This label will be
/// associated with a fragment in flushPendingLabels()
void addPendingLabel(MCSymbol* label, unsigned Subsection = 0);

/// Associate all pending labels in a subsection with a fragment.
void flushPendingLabels(MCFragment *F, unsigned Subsection);

/// Associate all pending labels with empty data fragments. One fragment
/// will be created for each subsection as necessary.
void flushPendingLabels();
};

} // end namespace llvm
Expand Down
9 changes: 7 additions & 2 deletions llvm/lib/MC/MCAssembler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,7 @@ uint64_t MCAssembler::computeFragmentSize(const MCAsmLayout &Layout,
llvm_unreachable("invalid fragment kind");
}

void MCAsmLayout::layoutBundle(MCFragment *F) {
void MCAsmLayout::layoutBundle(MCFragment *Prev, MCFragment *F) {
// If bundling is enabled and this fragment has instructions in it, it has to
// obey the bundling restrictions. With padding, we'll have:
//
Expand Down Expand Up @@ -439,6 +439,9 @@ void MCAsmLayout::layoutBundle(MCFragment *F) {
report_fatal_error("Padding cannot exceed 255 bytes");
EF->setBundlePadding(static_cast<uint8_t>(RequiredBundlePadding));
EF->Offset += RequiredBundlePadding;
if (auto *DF = dyn_cast_or_null<MCDataFragment>(Prev))
if (DF->getContents().empty())
DF->Offset = EF->Offset;
}

uint64_t MCAsmLayout::getFragmentOffset(const MCFragment *F) const {
Expand All @@ -451,14 +454,16 @@ void MCAsmLayout::ensureValid(const MCFragment *Frag) const {
if (Sec.hasLayout())
return;
Sec.setHasLayout(true);
MCFragment *Prev = nullptr;
uint64_t Offset = 0;
for (MCFragment &F : Sec) {
F.Offset = Offset;
if (Assembler.isBundlingEnabled() && F.hasInstructions()) {
const_cast<MCAsmLayout *>(this)->layoutBundle(&F);
const_cast<MCAsmLayout *>(this)->layoutBundle(Prev, &F);
Offset = F.Offset;
}
Offset += getAssembler().computeFragmentSize(*this, F);
Prev = &F;
}
}

Expand Down
102 changes: 4 additions & 98 deletions llvm/lib/MC/MCObjectStreamer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,59 +46,6 @@ MCAssembler *MCObjectStreamer::getAssemblerPtr() {
return nullptr;
}

void MCObjectStreamer::addPendingLabel(MCSymbol* S) {
MCSection *CurSection = getCurrentSectionOnly();
if (CurSection) {
// Register labels that have not yet been assigned to a Section.
if (!PendingLabels.empty()) {
for (MCSymbol* Sym : PendingLabels)
CurSection->addPendingLabel(Sym);
PendingLabels.clear();
}

// Add this label to the current Section / Subsection.
CurSection->addPendingLabel(S, CurSubsectionIdx);

// Add this Section to the list of PendingLabelSections.
PendingLabelSections.insert(CurSection);
} else
// There is no Section / Subsection for this label yet.
PendingLabels.push_back(S);
}

void MCObjectStreamer::flushPendingLabels(MCFragment *F, uint64_t FOffset) {
assert(F);
MCSection *CurSection = getCurrentSectionOnly();
if (!CurSection) {
assert(PendingLabels.empty());
return;
}
// Register labels that have not yet been assigned to a Section.
if (!PendingLabels.empty()) {
for (MCSymbol* Sym : PendingLabels)
CurSection->addPendingLabel(Sym, CurSubsectionIdx);
PendingLabels.clear();
}

// Associate the labels with F.
CurSection->flushPendingLabels(F, CurSubsectionIdx);
}

void MCObjectStreamer::flushPendingLabels() {
// Register labels that have not yet been assigned to a Section.
if (!PendingLabels.empty()) {
MCSection *CurSection = getCurrentSectionOnly();
assert(CurSection);
for (MCSymbol* Sym : PendingLabels)
CurSection->addPendingLabel(Sym, CurSubsectionIdx);
PendingLabels.clear();
}

// Assign an empty data fragment to all remaining pending labels.
for (MCSection* Section : PendingLabelSections)
Section->flushPendingLabels();
}

// When fixup's offset is a forward declared label, e.g.:
//
// .reloc 1f, R_MIPS_JALR, foo
Expand All @@ -113,7 +60,6 @@ void MCObjectStreamer::resolvePendingFixups() {
"unresolved relocation offset");
continue;
}
flushPendingLabels(PendingFixup.DF, PendingFixup.DF->getContents().size());
PendingFixup.Fixup.setOffset(PendingFixup.Sym->getOffset() +
PendingFixup.Fixup.getOffset());

Expand Down Expand Up @@ -245,7 +191,6 @@ void MCObjectStreamer::emitValueImpl(const MCExpr *Value, unsigned Size,
SMLoc Loc) {
MCStreamer::emitValueImpl(Value, Size, Loc);
MCDataFragment *DF = getOrCreateDataFragment();
flushPendingLabels(DF, DF->getContents().size());

MCDwarfLineEntry::make(this, getCurrentSectionOnly());

Expand Down Expand Up @@ -291,17 +236,9 @@ void MCObjectStreamer::emitLabel(MCSymbol *Symbol, SMLoc Loc) {
// If there is a current fragment, mark the symbol as pointing into it.
// Otherwise queue the label and set its fragment pointer when we emit the
// next fragment.
auto *F = dyn_cast_or_null<MCDataFragment>(getCurrentFragment());
if (F) {
Symbol->setFragment(F);
Symbol->setOffset(F->getContents().size());
} else {
// Assign all pending labels to offset 0 within the dummy "pending"
// fragment. (They will all be reassigned to a real fragment in
// flushPendingLabels())
Symbol->setOffset(0);
addPendingLabel(Symbol);
}
MCDataFragment *F = getOrCreateDataFragment();
Symbol->setFragment(F);
Symbol->setOffset(F->getContents().size());

emitPendingAssignments(Symbol);
}
Expand Down Expand Up @@ -598,11 +535,9 @@ void MCObjectStreamer::emitCVInlineLinetableDirective(
void MCObjectStreamer::emitCVDefRangeDirective(
ArrayRef<std::pair<const MCSymbol *, const MCSymbol *>> Ranges,
StringRef FixedSizePortion) {
MCFragment *Frag =
getContext().getCVContext().emitDefRange(*this, Ranges, FixedSizePortion);
getContext().getCVContext().emitDefRange(*this, Ranges, FixedSizePortion);
// Attach labels that were pending before we created the defrange fragment to
// the beginning of the new fragment.
flushPendingLabels(Frag, 0);
this->MCStreamer::emitCVDefRangeDirective(Ranges, FixedSizePortion);
}

Expand All @@ -620,7 +555,6 @@ void MCObjectStreamer::emitCVFileChecksumOffsetDirective(unsigned FileNo) {
void MCObjectStreamer::emitBytes(StringRef Data) {
MCDwarfLineEntry::make(this, getCurrentSectionOnly());
MCDataFragment *DF = getOrCreateDataFragment();
flushPendingLabels(DF, DF->getContents().size());
DF->getContents().append(Data.begin(), Data.end());
}

Expand Down Expand Up @@ -653,8 +587,6 @@ void MCObjectStreamer::emitValueToOffset(const MCExpr *Offset,
// Associate DTPRel32 fixup with data and resize data area
void MCObjectStreamer::emitDTPRel32Value(const MCExpr *Value) {
MCDataFragment *DF = getOrCreateDataFragment();
flushPendingLabels(DF, DF->getContents().size());

DF->getFixups().push_back(MCFixup::create(DF->getContents().size(),
Value, FK_DTPRel_4));
DF->getContents().resize(DF->getContents().size() + 4, 0);
Expand All @@ -663,8 +595,6 @@ void MCObjectStreamer::emitDTPRel32Value(const MCExpr *Value) {
// Associate DTPRel64 fixup with data and resize data area
void MCObjectStreamer::emitDTPRel64Value(const MCExpr *Value) {
MCDataFragment *DF = getOrCreateDataFragment();
flushPendingLabels(DF, DF->getContents().size());

DF->getFixups().push_back(MCFixup::create(DF->getContents().size(),
Value, FK_DTPRel_8));
DF->getContents().resize(DF->getContents().size() + 8, 0);
Expand All @@ -673,8 +603,6 @@ void MCObjectStreamer::emitDTPRel64Value(const MCExpr *Value) {
// Associate TPRel32 fixup with data and resize data area
void MCObjectStreamer::emitTPRel32Value(const MCExpr *Value) {
MCDataFragment *DF = getOrCreateDataFragment();
flushPendingLabels(DF, DF->getContents().size());

DF->getFixups().push_back(MCFixup::create(DF->getContents().size(),
Value, FK_TPRel_4));
DF->getContents().resize(DF->getContents().size() + 4, 0);
Expand All @@ -683,8 +611,6 @@ void MCObjectStreamer::emitTPRel32Value(const MCExpr *Value) {
// Associate TPRel64 fixup with data and resize data area
void MCObjectStreamer::emitTPRel64Value(const MCExpr *Value) {
MCDataFragment *DF = getOrCreateDataFragment();
flushPendingLabels(DF, DF->getContents().size());

DF->getFixups().push_back(MCFixup::create(DF->getContents().size(),
Value, FK_TPRel_8));
DF->getContents().resize(DF->getContents().size() + 8, 0);
Expand All @@ -693,8 +619,6 @@ void MCObjectStreamer::emitTPRel64Value(const MCExpr *Value) {
// Associate GPRel32 fixup with data and resize data area
void MCObjectStreamer::emitGPRel32Value(const MCExpr *Value) {
MCDataFragment *DF = getOrCreateDataFragment();
flushPendingLabels(DF, DF->getContents().size());

DF->getFixups().push_back(
MCFixup::create(DF->getContents().size(), Value, FK_GPRel_4));
DF->getContents().resize(DF->getContents().size() + 4, 0);
Expand All @@ -703,8 +627,6 @@ void MCObjectStreamer::emitGPRel32Value(const MCExpr *Value) {
// Associate GPRel64 fixup with data and resize data area
void MCObjectStreamer::emitGPRel64Value(const MCExpr *Value) {
MCDataFragment *DF = getOrCreateDataFragment();
flushPendingLabels(DF, DF->getContents().size());

DF->getFixups().push_back(
MCFixup::create(DF->getContents().size(), Value, FK_GPRel_4));
DF->getContents().resize(DF->getContents().size() + 8, 0);
Expand Down Expand Up @@ -789,8 +711,6 @@ MCObjectStreamer::emitRelocDirective(const MCExpr &Offset, StringRef Name,
MCSymbolRefExpr::create(getContext().createTempSymbol(), getContext());

MCDataFragment *DF = getOrCreateDataFragment(&STI);
flushPendingLabels(DF, DF->getContents().size());

MCValue OffsetVal;
if (!Offset.evaluateAsRelocatable(OffsetVal, nullptr, nullptr))
return std::make_pair(false,
Expand Down Expand Up @@ -830,9 +750,6 @@ MCObjectStreamer::emitRelocDirective(const MCExpr &Offset, StringRef Name,

void MCObjectStreamer::emitFill(const MCExpr &NumBytes, uint64_t FillValue,
SMLoc Loc) {
MCDataFragment *DF = getOrCreateDataFragment();
flushPendingLabels(DF, DF->getContents().size());

assert(getCurrentSectionOnly() && "need a section");
insert(
getContext().allocFragment<MCFillFragment>(FillValue, 1, NumBytes, Loc));
Expand Down Expand Up @@ -861,22 +778,14 @@ void MCObjectStreamer::emitFill(const MCExpr &NumValues, int64_t Size,
}

// Otherwise emit as fragment.
MCDataFragment *DF = getOrCreateDataFragment();
flushPendingLabels(DF, DF->getContents().size());

assert(getCurrentSectionOnly() && "need a section");
insert(
getContext().allocFragment<MCFillFragment>(Expr, Size, NumValues, Loc));
}

void MCObjectStreamer::emitNops(int64_t NumBytes, int64_t ControlledNopLength,
SMLoc Loc, const MCSubtargetInfo &STI) {
// Emit an NOP fragment.
MCDataFragment *DF = getOrCreateDataFragment();
flushPendingLabels(DF, DF->getContents().size());

assert(getCurrentSectionOnly() && "need a section");

insert(getContext().allocFragment<MCNopsFragment>(
NumBytes, ControlledNopLength, Loc, STI));
}
Expand Down Expand Up @@ -916,9 +825,6 @@ void MCObjectStreamer::finishImpl() {
// Emit pseudo probes for the current module.
MCPseudoProbeTable::emit(this);

// Update any remaining pending labels with empty data fragments.
flushPendingLabels();

resolvePendingFixups();
getAssembler().Finish();
}
27 changes: 0 additions & 27 deletions llvm/lib/MC/MCSection.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,33 +80,6 @@ void MCSection::switchSubsection(unsigned Subsection) {
StringRef MCSection::getVirtualSectionKind() const { return "virtual"; }

void MCSection::addPendingLabel(MCSymbol *label, unsigned Subsection) {
PendingLabels.push_back(PendingLabel(label, Subsection));
}

void MCSection::flushPendingLabels(MCFragment *F, unsigned Subsection) {
// Set the fragment and fragment offset for all pending symbols in the
// specified Subsection, and remove those symbols from the pending list.
for (auto It = PendingLabels.begin(); It != PendingLabels.end(); ++It) {
PendingLabel& Label = *It;
if (Label.Subsection == Subsection) {
Label.Sym->setFragment(F);
assert(Label.Sym->getOffset() == 0);
PendingLabels.erase(It--);
}
}
}

void MCSection::flushPendingLabels() {
// Make sure all remaining pending labels point to data fragments, by
// creating new empty data fragments for each Subsection with labels pending.
while (!PendingLabels.empty()) {
PendingLabel& Label = PendingLabels[0];
switchSubsection(Label.Subsection);
MCFragment *F = new MCDataFragment();
addFragment(*F);
F->setParent(this);
flushPendingLabels(F, Label.Subsection);
}
}

#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
Expand Down
20 changes: 10 additions & 10 deletions llvm/test/MC/MachO/pending-labels.s
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,17 +32,17 @@ __data:
.quad L8-.
// CHECK: 0000000000000038 X86_64_RELOC_SUBTRACTOR _function1-__data
// CHECK: 0000000000000038 X86_64_RELOC_UNSIGNED _function1
// CHECK: 0000000000000030 X86_64_RELOC_SUBTRACTOR _function1-__data
// CHECK: 0000000000000030 X86_64_RELOC_UNSIGNED _function1
// CHECK: 0000000000000030 X86_64_RELOC_SUBTRACTOR __text-__data
// CHECK: 0000000000000030 X86_64_RELOC_UNSIGNED __text
// CHECK: 0000000000000028 X86_64_RELOC_SUBTRACTOR _function2-__data
// CHECK: 0000000000000028 X86_64_RELOC_UNSIGNED _function2
// CHECK: 0000000000000020 X86_64_RELOC_SUBTRACTOR _function2-__data
// CHECK: 0000000000000020 X86_64_RELOC_UNSIGNED _function2
// CHECK: 0000000000000018 X86_64_RELOC_SUBTRACTOR _function2-__data
// CHECK: 0000000000000018 X86_64_RELOC_UNSIGNED _function2
// CHECK: 0000000000000010 X86_64_RELOC_SUBTRACTOR _function1-__data
// CHECK: 0000000000000010 X86_64_RELOC_UNSIGNED _function1
// CHECK: 0000000000000008 X86_64_RELOC_SUBTRACTOR _function2-__data
// CHECK: 0000000000000008 X86_64_RELOC_UNSIGNED _function2
// CHECK: 0000000000000000 X86_64_RELOC_SUBTRACTOR _function1-__data
// CHECK: 0000000000000000 X86_64_RELOC_UNSIGNED _function1
// CHECK: 0000000000000018 X86_64_RELOC_SUBTRACTOR __text_cold-__data
// CHECK: 0000000000000018 X86_64_RELOC_UNSIGNED __text_cold
// CHECK: 0000000000000010 X86_64_RELOC_SUBTRACTOR __text-__data
// CHECK: 0000000000000010 X86_64_RELOC_UNSIGNED __text
// CHECK: 0000000000000008 X86_64_RELOC_SUBTRACTOR __text_cold-__data
// CHECK: 0000000000000008 X86_64_RELOC_UNSIGNED __text_cold
// CHECK: 0000000000000000 X86_64_RELOC_SUBTRACTOR __text-__data
// CHECK: 0000000000000000 X86_64_RELOC_UNSIGNED __text
1 change: 0 additions & 1 deletion llvm/tools/dsymutil/MachOUtils.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,6 @@ bool generateDsymCompanion(
auto &Writer = static_cast<MachObjectWriter &>(MCAsm.getWriter());

// Layout but don't emit.
ObjectStreamer.flushPendingLabels();
MCAsmLayout Layout(MCAsm);
MCAsm.layout(Layout);

Expand Down

8 comments on commit 7500646

@zeroomega
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

We are working on building LLDB and QEMU for mac-arm64 and we noticed that their dependency libffi has build failures under tip of tree clang. We bisect the clang and trace it back to this patch.

The error message is:

/Volumes/Work/s/w/ir/x/w/llvm-third_party-libffi/src/java_raw_api.c:328:46: warning: 'ffi_java_raw_size' is deprecated [-Wdeprecated-declarations]
  328 |   ffi_java_raw *raw = (ffi_java_raw*)alloca (ffi_java_raw_size (cif));
      |                                              ^
include/ffi.h:326:56: note: 'ffi_java_raw_size' has been explicitly marked deprecated here
  326 | size_t ffi_java_raw_size (ffi_cif *cif) __attribute__((deprecated));
      |                                                        ^
/Volumes/Work/s/w/ir/x/w/llvm-third_party-libffi/src/java_raw_api.c:331:3: warning: 'ffi_java_ptrarray_to_raw' is deprecated [-Wdeprecated-declarations]
  331 |   ffi_java_ptrarray_to_raw (cif, avalue, raw);
      |   ^
include/ffi.h:322:93: note: 'ffi_java_ptrarray_to_raw' has been explicitly marked deprecated here
  322 | void ffi_java_ptrarray_to_raw (ffi_cif *cif, void **args, ffi_java_raw *raw) __attribute__((deprecated));
      |                                                                                             ^
2 warnings generated.
/Volumes/Work/s/w/ir/x/t/sysv-106cae.s:27:2: error: invalid CFI advance_loc expression
 .cfi_def_cfa x1, 40;
 ^
/Volumes/Work/s/w/ir/x/t/sysv-106cae.s:188:2: error: invalid CFI advance_loc expression
 .cfi_adjust_cfa_offset (8*2 + (8 * 16 + 8 * 8) + 64)
 ^

Full build output is https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket/8736348038352263873/+/u/build_llvm_dependencies/libffi/build/stdout

Could you take a look please?

@MaskRay
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

libffi has a known issue with a pending fix libffi/libffi#857 .

Older Clang versions were not rigid enough to detect this issue. That said, the issue is unrelated to this commit and I believe your bisection result pointed to a wrong commit.

@zeroomega
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx for pointing out the upstream issue. We will try a libffi roll once it is landed.

But bisection result is correct.

Clang build for revision "75006466296ed4b0f845cbbec4bf77c21de43b40" (this patch). failed to build libffi (https://ci.chromium.org/ui/p/fuchsia/builders/toolchain.ci.shadow/lldb-mac-arm64/b8736348038352263873/overview) . You can fetch the clang from cas in task: https://ci.chromium.org/ui/p/fuchsia/builders/toolchain.ci.shadow/clang-mac-arm64/b8736387029429984945/overview (CAS URL :https://cas-viewer.appspot.com/projects/chromium-swarm/instances/default_instance/blobs/8abd312ecdf45b6b028366dd47473a3242d6c0c9b31c5d29960e8b1546206b4f/578/tree)

Clang build for revision "a091bfe71fdde4358dbfc73926f875cb05121c00" (this patch's parent) pass the libffi build (https://ci.chromium.org/ui/p/fuchsia/builders/toolchain.ci.shadow/lldb-mac-arm64/b8736347996491450193/overview I purposely crashes the build after successfully built libffi). You can fetch this version of clang from task: https://ci.chromium.org/ui/p/fuchsia/builders/toolchain.ci.shadow/clang-mac-arm64/b8736386985363052689/overview (CAS URL: https://cas-viewer.appspot.com/projects/chromium-swarm/instances/default_instance/blobs/dd046fa76346b08fe9643685f1b64a223fb54b19286cd29daeb67af4e34c092a/578/tree)

I was a bit skeptical as well, but this patch did trigger the libffi build failure.

@MaskRay
Copy link
Member Author

@MaskRay MaskRay commented on 7500646 Sep 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

/Volumes/Work/s/w/ir/x/t/sysv-106cae.s:27:2: error: invalid CFI advance_loc expression
 .cfi_def_cfa x1, 40;
 ^
/Volumes/Work/s/w/ir/x/t/sysv-106cae.s:188:2: error: invalid CFI advance_loc expression
 .cfi_adjust_cfa_offset (8*2 + (8 * 16 + 8 * 8) + 64)
 ^

The file looks like the file, which will be modified by libffi/libffi#857 .

Do you have a smaller assembly reproduce? A commit last year https://reviews.llvm.org/D153167 should technically catch this assembler issue, though it's possible there were some scenarios only caught by this commit or some assembler changes I made in recent months. Perhaps "pending labels" did paper over some issues.

I'd like to improve llvm/test/MC testsuite to report errors for such cases we previously missed.

@petrhosek
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We confirmed that this change is indeed the culprit. This assembler constraint didn't exist before and you've now imposed it on all users without any way to opt out. Although this change might be desirable, it's a breaking change that is going to affect every existing user of libffi on Apple Silicon (and potentially other projects as well). Even if libffi/libffi#857 is merged, it'll take years for every project that depends on libffi to pick it up, and in the meantime users won't be able to build with Clang 19 and newer. I think we should revert this change and cherry-pick the revert into the 19.1.1 release.

@MaskRay
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: this is a workaround even if you don't modify the libffi source.
You can undefine HAVE_AS_CFI_PSEUDO_OP to make #define cfi_* no-op, since the CFI was broken by older Clang anyway.

I don't think "this assembler constraint didn't exist before" is a good reason to restore the previous broken behavior.
Even outside assemblers, we've made a lot of changes to change silent breakage to explicit rejection.

FWIW: https://reviews.llvm.org/D153167 (2023-06) rejected a lot of invalid cases.
Pending labels were probably more broken on Mach-O (https://reviews.llvm.org/D71368 2019), so removing pending labels might lead to more legitimate rejection.

@petrhosek
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: this is a workaround even if you don't modify the libffi source. You can undefine HAVE_AS_CFI_PSEUDO_OP to make #define cfi_* no-op, since the CFI was broken by older Clang anyway.

That doesn't work, at least not without patching the code, since HAVE_AS_CFI_PSEUDO_OP is defined in the generated header.

I don't think "this assembler constraint didn't exist before" is a good reason to restore the previous broken behavior. Even outside assemblers, we've made a lot of changes to change silent breakage to explicit rejection.

FWIW: https://reviews.llvm.org/D153167 (2023-06) rejected a lot of invalid cases. Pending labels were probably more broken on Mach-O (https://reviews.llvm.org/D71368 2019), so removing pending labels might lead to more legitimate rejection.

There's no documented assembler semantics that says that this syntax is invalid. The correct solution is to address the issue with pending labels implementation rather than breaking the compatibility with other assemblers.

@MaskRay
Copy link
Member Author

@MaskRay MaskRay commented on 7500646 Sep 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apple's cctools as was modified from binutils. LLVM integrated assembler ported its behavior but the implementation has always been brittle and there hasn't been many diagnostics.
There isn't a spec about .subsections_via_symbols's behavior. We have to reason about them with code.

As early as 2010 (even before ede8e6d),
_bar below triggers a new data fragment and becomes its atom.
L1 belongs to the earlier fragment with _foo as the atom, while L2 belongs to the latter fragment with _bar as the atom.

Label differences with different atoms (e.g. L2 - L1) are invalid when relocations are disallowed.
(The linker might move the two subsection to different place, and L2 - L1 without relocations cannot be represented.)

.globl _foo, _bar
_foo:
  nop
L1:

_bar:
L2:
  nop

Similarly (if we replace L1 with a .cfi_startproc), when a non-private foo appears between .cfi_startproc/.cfi_endproc, this should be invalid.

.globl _bar
.cfi_startproc
_bar:
  .cfi_offset 0, 0
.cfi_endproc

(https://reviews.llvm.org/D5915 (2014), intended for ELF, changed Mach-O behavior as well and made the invalid .cfi_offset legal.)

Actually there is a parse-time error when the assembler has seen .globl (https://reviews.llvm.org/D153167 2023):

% myllvm-mc -filetype=obj -triple=x86_64-apple-darwin a.s -o a.o
a.s:3:1: error: non-private labels cannot appear between .cfi_startproc / .cfi_endproc pairs
...

If .globl foo is moved after .cfi_endproc, there will not be such an error, but the case is still invalid.

I want to see arguments that libffi is so important that requires a workaround implemented in the assembler.
If that is justified, we could add a temporary cl::opt<bool> to change evaluateAsAbsolute to evaluateKnownAbsolute in MCAssembler::relaxDwarfCallFrameFragment.

Please sign in to comment.