Skip to content

[dwarf] make dwarf fission compatible with RISCV relaxations #164128

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[dwarf] make dwarf fission compatible with RISCV relaxations #164128

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8400679
Revert "[Driver] Error for -gsplit-dwarf with RISC-V linker relaxation"
dlav-sc Sep 17, 2025
bb526f6
[dwarf] make dwarf fission compatible with RISCV relaxations
dlav-sc Sep 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions clang/include/clang/Basic/DiagnosticDriverKinds.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -833,9 +833,6 @@ def warn_drv_sarif_format_unstable : Warning<
"diagnostic formatting in SARIF mode is currently unstable">,
InGroup<DiagGroup<"sarif-format-unstable">>;

def err_drv_riscv_unsupported_with_linker_relaxation : Error<
"%0 is unsupported with RISC-V linker relaxation (-mrelax)">;

Comment on lines -836 to -838
Copy link
Collaborator

Choose a reason for hiding this comment

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

The clang change will need to go in in a separate commit - splitting independent patches. Add the functionality to LLVM first, then enable its use in clang in a separate follow-up patch.

def warn_drv_loongarch_conflicting_implied_val : Warning<
"ignoring '%0' as it conflicts with that implied by '%1' (%2)">,
InGroup<OptionIgnored>;
Expand Down
11 changes: 2 additions & 9 deletions clang/lib/Driver/ToolChains/Arch/RISCV.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,17 +130,10 @@ void riscv::getRISCVTargetFeatures(const Driver &D, const llvm::Triple &Triple,
#undef RESERVE_REG

// -mrelax is default, unless -mno-relax is specified.
if (Args.hasFlag(options::OPT_mrelax, options::OPT_mno_relax, true)) {
if (Args.hasFlag(options::OPT_mrelax, options::OPT_mno_relax, true))
Features.push_back("+relax");
// -gsplit-dwarf -mrelax requires DW_AT_high_pc/DW_AT_ranges/... indexing
// into .debug_addr, which is currently not implemented.
Arg *A;
if (getDebugFissionKind(D, Args, A) != DwarfFissionKind::None)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Huh, how'd RISCV survive even in non-split DWARF where high_pc would've been a constant relative to the low_pc without this patch/work?

Copy link
Member

Choose a reason for hiding this comment

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

RISC-V has relocations for PC offsets in debug info. IIRC, R_RISCV_SET6 and R_RISCV_SUB6 would be placed as a pair on the relevant part of the debug info. There are variants of this for ulebs too, which came later.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, so this is only related to Split DWARF, good to understand.
Could you break it up into a few steps, though - the trait-based refactoring (if it's worth it), each piece of the DWARF (range list, loc list, attribute, whatever else), then the clang change.

D.Diag(clang::diag::err_drv_riscv_unsupported_with_linker_relaxation)
<< A->getAsString(Args);
} else {
else
Features.push_back("-relax");
}

// If -mstrict-align, -mno-strict-align, -mscalar-strict-align, or
// -mno-scalar-strict-align is passed, use it. Otherwise, the
Expand Down
7 changes: 0 additions & 7 deletions clang/test/Driver/riscv-features.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,13 +68,6 @@
// DEFAULT-LINUX-SAME: "-target-feature" "+d"
// DEFAULT-LINUX-SAME: "-target-feature" "+c"

// RUN: not %clang -c --target=riscv64-linux-gnu -gsplit-dwarf %s 2>&1 | FileCheck %s --check-prefix=ERR-SPLIT-DWARF
// RUN: not %clang -c --target=riscv64 -gsplit-dwarf=single %s 2>&1 | FileCheck %s --check-prefix=ERR-SPLIT-DWARF
// RUN: %clang -### -c --target=riscv64 -mno-relax -g -gsplit-dwarf %s 2>&1 | FileCheck %s --check-prefix=SPLIT-DWARF

// ERR-SPLIT-DWARF: error: -gsplit-dwarf{{.*}} is unsupported with RISC-V linker relaxation (-mrelax)
// SPLIT-DWARF: "-split-dwarf-file"

// RUN: %clang -mabi=lp64d --target=riscv64-unknown-fuchsia -### %s -fsyntax-only 2>&1 | FileCheck %s -check-prefixes=FUCHSIA
// FUCHSIA: "-target-feature" "+m"
// FUCHSIA-SAME: "-target-feature" "+a"
Expand Down
2 changes: 2 additions & 0 deletions llvm/include/llvm/MC/MCSymbol.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -383,6 +383,8 @@ inline raw_ostream &operator<<(raw_ostream &OS, const MCSymbol &Sym) {
return OS;
}

bool isRangeRelaxable(const MCSymbol *Begin, const MCSymbol *End);

} // end namespace llvm

#endif // LLVM_MC_MCSYMBOL_H
8 changes: 5 additions & 3 deletions llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -493,10 +493,12 @@ void DwarfCompileUnit::attachLowHighPC(DIE &D, const MCSymbol *Begin,
assert(End->isDefined() && "Invalid end label");

addLabelAddress(D, dwarf::DW_AT_low_pc, Begin);
if (DD->getDwarfVersion() < 4)
addLabelAddress(D, dwarf::DW_AT_high_pc, End);
else
if (DD->getDwarfVersion() >= 4 &&
(!isDwoUnit() || !llvm::isRangeRelaxable(Begin, End))) {
addLabelDelta(D, dwarf::DW_AT_high_pc, End, Begin);
return;
}
addLabelAddress(D, dwarf::DW_AT_high_pc, End);
Comment on lines +496 to +501
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should probably be a separate patch too - you can do the uses of isRangeRelaxable in any order (& whichever's the first can have the isRangeRelaxable function, so it can be tested when it's added, etc), but should be separate with incremental test coverage being added, etc.

Though perhaps the Split DWARF test coverage (if we have an assert checking that relocations aren't used in .dwo files) can only be added at the end once all the functionality is in.

}

// Add info for Wasm-global-based relocation.
Expand Down
147 changes: 112 additions & 35 deletions llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3275,34 +3275,70 @@ static MCSymbol *emitLoclistsTableHeader(AsmPrinter *Asm,
return TableEnd;
}

template <typename Ranges, typename PayloadEmitter>
static void emitRangeList(
DwarfDebug &DD, AsmPrinter *Asm, MCSymbol *Sym, const Ranges &R,
const DwarfCompileUnit &CU, unsigned BaseAddressx, unsigned OffsetPair,
unsigned StartxLength, unsigned EndOfList,
StringRef (*StringifyEnum)(unsigned),
bool ShouldUseBaseAddress,
PayloadEmitter EmitPayload) {
namespace {

struct DebugLocSpanList {
MCSymbol *Label;
const DwarfCompileUnit *CU;
llvm::ArrayRef<llvm::DebugLocStream::Entry> Ranges;
};
Comment on lines +3280 to +3284
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure that this structure's pulling its weight here - does it make much of a difference to stick with the original API, passing a few more parameters directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I introduced this struct mainly because I needed a template parameter to specialize DwarfRangeListTraits. Furthermore, since we already have RangesSpanList, it makes sense to add a corresponding struct for DebugLocs for symmetry. This approach also helped clarify the parameter list, as you noticed.


template <typename DWARFSpanList> struct DwarfRangeListTraits {};

template <> struct DwarfRangeListTraits<DebugLocSpanList> {
static constexpr unsigned BaseAddressx = dwarf::DW_LLE_base_addressx;
static constexpr unsigned OffsetPair = dwarf::DW_LLE_offset_pair;
static constexpr unsigned StartxLength = dwarf::DW_LLE_startx_length;
static constexpr unsigned StartxEndx = dwarf::DW_LLE_startx_endx;
static constexpr unsigned EndOfList = dwarf::DW_LLE_end_of_list;

static StringRef StringifyRangeKind(unsigned Encoding) {
return llvm::dwarf::LocListEncodingString(Encoding);
}
};

template <> struct DwarfRangeListTraits<RangeSpanList> {
static constexpr unsigned BaseAddressx = dwarf::DW_RLE_base_addressx;
static constexpr unsigned OffsetPair = dwarf::DW_RLE_offset_pair;
static constexpr unsigned StartxLength = dwarf::DW_RLE_startx_length;
static constexpr unsigned StartxEndx = dwarf::DW_RLE_startx_endx;
static constexpr unsigned EndOfList = dwarf::DW_RLE_end_of_list;

static StringRef StringifyRangeKind(unsigned Encoding) {
return llvm::dwarf::RangeListEncodingString(Encoding);
}
};

} // namespace

template <
typename Ranges, typename PayloadEmitter,
std::enable_if_t<DwarfRangeListTraits<Ranges>::BaseAddressx, bool> = true>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably doesn't need the enable_if SFINAE? (emitRangeList isn't overloaded, right? So it can just fail to compile if someone passes in a range that doesn't have matching traits?)

& could you split this change out - do the trait-based change as NFC (but still send it for review) then do the relaxation stuff on top of that, would help isolate the changes?

static void emitRangeList(DwarfDebug &DD, AsmPrinter *Asm, const Ranges &R,
bool ShouldUseBaseAddress,
PayloadEmitter EmitPayload) {

auto Size = Asm->MAI->getCodePointerSize();
bool UseDwarf5 = DD.getDwarfVersion() >= 5;

// Emit our symbol so we can find the beginning of the range.
Asm->OutStreamer->emitLabel(Sym);
Asm->OutStreamer->emitLabel(R.Label);

// Gather all the ranges that apply to the same section so they can share
// a base address entry.
SmallMapVector<const MCSection *, std::vector<decltype(&*R.begin())>, 16>
SmallMapVector<const MCSection *, std::vector<decltype(&*R.Ranges.begin())>,
16>
SectionRanges;

for (const auto &Range : R)
for (const auto &Range : R.Ranges)
SectionRanges[&Range.Begin->getSection()].push_back(&Range);

const MCSymbol *CUBase = CU.getBaseAddress();
const MCSymbol *CUBase = R.CU->getBaseAddress();
bool BaseIsSet = false;
for (const auto &P : SectionRanges) {
auto *Base = CUBase;
if ((Asm->TM.getTargetTriple().isNVPTX() && DD.tuneForGDB())) {
if ((Asm->TM.getTargetTriple().isNVPTX() && DD.tuneForGDB()) ||
(DD.useSplitDwarf() && P.first->isLinkerRelaxable())) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be worth separating these two changes too - one for DWARFv4 and one for DWARFv5?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, indeed, I've additionally added UseDwarf5 here.

// PTX does not support subtracting labels from the code section in the
// debug_loc section. To work around this, the NVPTX backend needs the
// compile unit to have no low_pc in order to have a zero base_address
Expand All @@ -3327,8 +3363,10 @@ static void emitRangeList(
// * or, there's more than one entry to share the base address
Base = NewBase;
BaseIsSet = true;
Asm->OutStreamer->AddComment(StringifyEnum(BaseAddressx));
Asm->emitInt8(BaseAddressx);
Asm->OutStreamer->AddComment(
DwarfRangeListTraits<Ranges>::StringifyRangeKind(
DwarfRangeListTraits<Ranges>::BaseAddressx));
Asm->emitInt8(DwarfRangeListTraits<Ranges>::BaseAddressx);
Asm->OutStreamer->AddComment(" base address index");
Asm->emitULEB128(DD.getAddressPool().getIndex(Base));
}
Expand All @@ -3339,16 +3377,57 @@ static void emitRangeList(
Asm->OutStreamer->emitIntValue(0, Size);
}

for (const auto *RS : P.second) {
if (DD.useSplitDwarf() && UseDwarf5) {
// In .dwo files, we must ensure no relocations are present. For
// .debug_ranges.dwo. this means that if there is at least one
// relocation between the start and end of a range, we must
// represent the range boundaries using indirect addresses from
// the .debug_addr section.
//
// The DWARFv5 specification (section 2.17.3) does not require
// range entries to be ordered. Therefore, we emit such range
// entries here to allow using more optimal formats (e.g.
// StartxLength) for other ranges.

auto RelaxableRanges = llvm::make_filter_range(P.second, [](auto &&RS) {
return llvm::isRangeRelaxable(RS->Begin, RS->End);
});

for (auto &&RS : RelaxableRanges) {
const auto *Begin = RS->Begin;
const auto *End = RS->End;
Asm->OutStreamer->AddComment(
DwarfRangeListTraits<Ranges>::StringifyRangeKind(
DwarfRangeListTraits<Ranges>::StartxEndx));
Asm->emitInt8(DwarfRangeListTraits<Ranges>::StartxEndx);
Asm->OutStreamer->AddComment(" start index");
Asm->emitULEB128(DD.getAddressPool().getIndex(Begin));
Asm->OutStreamer->AddComment(" end index");
Asm->emitULEB128(DD.getAddressPool().getIndex(End));
EmitPayload(*RS);
}
}

auto NonRelaxableRanges = llvm::make_filter_range(
P.second,
[HasRelaxableRanges = DD.useSplitDwarf() && UseDwarf5](auto &&RS) {
if (!HasRelaxableRanges)
return true;
return !llvm::isRangeRelaxable(RS->Begin, RS->End);
});

for (const auto *RS : NonRelaxableRanges) {
const MCSymbol *Begin = RS->Begin;
const MCSymbol *End = RS->End;
assert(Begin && "Range without a begin symbol?");
assert(End && "Range without an end symbol?");
if (Base) {
if (UseDwarf5) {
// Emit offset_pair when we have a base.
Asm->OutStreamer->AddComment(StringifyEnum(OffsetPair));
Asm->emitInt8(OffsetPair);
Asm->OutStreamer->AddComment(
DwarfRangeListTraits<Ranges>::StringifyRangeKind(
DwarfRangeListTraits<Ranges>::OffsetPair));
Asm->emitInt8(DwarfRangeListTraits<Ranges>::OffsetPair);
Asm->OutStreamer->AddComment(" starting offset");
Asm->emitLabelDifferenceAsULEB128(Begin, Base);
Asm->OutStreamer->AddComment(" ending offset");
Expand All @@ -3358,8 +3437,10 @@ static void emitRangeList(
Asm->emitLabelDifference(End, Base, Size);
}
} else if (UseDwarf5) {
Asm->OutStreamer->AddComment(StringifyEnum(StartxLength));
Asm->emitInt8(StartxLength);
Asm->OutStreamer->AddComment(
DwarfRangeListTraits<Ranges>::StringifyRangeKind(
DwarfRangeListTraits<Ranges>::StartxLength));
Asm->emitInt8(DwarfRangeListTraits<Ranges>::StartxLength);
Asm->OutStreamer->AddComment(" start index");
Asm->emitULEB128(DD.getAddressPool().getIndex(Begin));
Asm->OutStreamer->AddComment(" length");
Expand All @@ -3373,8 +3454,10 @@ static void emitRangeList(
}

if (UseDwarf5) {
Asm->OutStreamer->AddComment(StringifyEnum(EndOfList));
Asm->emitInt8(EndOfList);
Asm->OutStreamer->AddComment(
DwarfRangeListTraits<Ranges>::StringifyRangeKind(
DwarfRangeListTraits<Ranges>::EndOfList));
Asm->emitInt8(DwarfRangeListTraits<Ranges>::EndOfList);
} else {
// Terminate the list with two 0 values.
Asm->OutStreamer->emitIntValue(0, Size);
Expand All @@ -3384,11 +3467,9 @@ static void emitRangeList(

// Handles emission of both debug_loclist / debug_loclist.dwo
static void emitLocList(DwarfDebug &DD, AsmPrinter *Asm, const DebugLocStream::List &List) {
emitRangeList(DD, Asm, List.Label, DD.getDebugLocs().getEntries(List),
*List.CU, dwarf::DW_LLE_base_addressx,
dwarf::DW_LLE_offset_pair, dwarf::DW_LLE_startx_length,
dwarf::DW_LLE_end_of_list, llvm::dwarf::LocListEncodingString,
/* ShouldUseBaseAddress */ true,
DebugLocSpanList Ranges = {List.Label, List.CU,
DD.getDebugLocs().getEntries(List)};
emitRangeList(DD, Asm, Ranges, /* ShouldUseBaseAddress */ true,
[&](const DebugLocStream::Entry &E) {
DD.emitDebugLocEntryLocation(E, List.CU);
});
Expand All @@ -3413,10 +3494,9 @@ void DwarfDebug::emitDebugLocImpl(MCSection *Sec) {

// Emit locations into the .debug_loc/.debug_loclists section.
void DwarfDebug::emitDebugLoc() {
emitDebugLocImpl(
getDwarfVersion() >= 5
? Asm->getObjFileLowering().getDwarfLoclistsSection()
: Asm->getObjFileLowering().getDwarfLocSection());
emitDebugLocImpl(getDwarfVersion() >= 5
? Asm->getObjFileLowering().getDwarfLoclistsSection()
: Asm->getObjFileLowering().getDwarfLocSection());
}

// Emit locations into the .debug_loc.dwo/.debug_loclists.dwo section.
Expand Down Expand Up @@ -3611,10 +3691,7 @@ void DwarfDebug::emitDebugARanges() {
/// Emit a single range list. We handle both DWARF v5 and earlier.
static void emitRangeList(DwarfDebug &DD, AsmPrinter *Asm,
const RangeSpanList &List) {
emitRangeList(DD, Asm, List.Label, List.Ranges, *List.CU,
dwarf::DW_RLE_base_addressx, dwarf::DW_RLE_offset_pair,
dwarf::DW_RLE_startx_length, dwarf::DW_RLE_end_of_list,
llvm::dwarf::RangeListEncodingString,
emitRangeList(DD, Asm, List,
List.CU->getCUNode()->getRangesBaseAddress() ||
DD.getDwarfVersion() >= 5,
[](auto) {});
Expand Down
22 changes: 19 additions & 3 deletions llvm/lib/MC/MCSymbol.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,24 @@ void MCSymbol::print(raw_ostream &OS, const MCAsmInfo *MAI) const {
OS << '"';
}

#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
LLVM_DUMP_METHOD void MCSymbol::dump() const {
dbgs() << *this;
bool llvm::isRangeRelaxable(const MCSymbol *Begin, const MCSymbol *End) {
assert(Begin && "Range without a begin symbol?");
assert(End && "Range without an end symbol?");
llvm::SmallVector<const MCFragment *> RangeFragments{};
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not quite following - why is this vector needed? (rather than computing the IsRelaxableRange via a single iteration over the fragments in this loop below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to separate the logic for obtaining fragments contained in the range and then use llvm::any_of to check if any of them are relaxable. However, we can use just a single loop if you prefer.

for (const auto *Fragment = Begin->getFragment();
Fragment != End->getFragment(); Fragment = Fragment->getNext()) {
assert(Fragment);
RangeFragments.push_back(Fragment);
}
assert(End->getFragment());
RangeFragments.push_back(End->getFragment());

bool IsRelaxableRange = llvm::any_of(RangeFragments, [](auto &&Fragment) {
return Fragment->isLinkerRelaxable();
});
return IsRelaxableRange;
}

#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
LLVM_DUMP_METHOD void MCSymbol::dump() const { dbgs() << *this; }
#endif
Loading