Skip to content

[lld-macho] Handle InputSection targets in branch range extension logic #126347

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

Closed
wants to merge 1 commit into from
Closed

[lld-macho] Handle InputSection targets in branch range extension logic #126347

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
34 changes: 32 additions & 2 deletions lld/MachO/ConcatOutputSection.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,30 @@ void ConcatOutputSection::addInput(ConcatInputSection *input) {

DenseMap<Symbol *, ThunkInfo> lld::macho::thunkMap;

// Returns the target Symbol that a relocation refers to.
// A Reloc can refer to either a Symbol directly, or to an InputSection.
// For InputSection referents, we return the first Symbol at offset 0.
// This conversion is necessary because the thunk generation algorithm
// can only handle Symbols as branch targets, not InputSections.
static Symbol *getReferentSymbol(const Reloc &r) {
if (auto *sym = r.referent.dyn_cast<Symbol *>()) {
Copy link
Contributor

@kazutakahirata kazutakahirata Feb 9, 2025

Choose a reason for hiding this comment

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

We are migrating away from PointerUnion::dyn_cast:

llvm-project/llvm/include/llvm/ADT/PointerUnion.h

Lines 146 to 147 in c89735d

// FIXME: Replace the uses of is(), get() and dyn_cast() with
// isa<T>, cast<T> and the llvm::dyn_cast<T>

AFAICT from llvm_unrechable below, you expect r.referent.isNull() to be false, so may I suggest the following?

Suggested change
if (auto *sym = r.referent.dyn_cast<Symbol *>()) {
if (auto *sym = llvm::dyn_cast<Symbol *>(r.referent)) {

return sym;
} else if (auto *isec = r.referent.dyn_cast<InputSection *>()) {
Copy link
Contributor

@kazutakahirata kazutakahirata Feb 9, 2025

Choose a reason for hiding this comment

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

Likewise, may I suggest the following?

Suggested change
} else if (auto *isec = r.referent.dyn_cast<InputSection *>()) {
} else if (auto *isec = llvm::dyn_cast<InputSection *>(r.referent)) {

Copy link
Contributor

@ellishg ellishg Feb 10, 2025

Choose a reason for hiding this comment

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

Also, don't use else after a return
https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return

// Use the first symbol at offset 0 in the InputSection
for (Defined *sym : isec->symbols) {
if (sym->value == 0) {
return sym;
}
}
Comment on lines +129 to +133
Copy link
Contributor

@ellishg ellishg Feb 10, 2025

Choose a reason for hiding this comment

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

Suggested change
for (Defined *sym : isec->symbols) {
if (sym->value == 0) {
return sym;
}
}
for (Defined *sym : isec->symbols)
if (sym->value == 0)
return sym;

// Handle absence of suitable symbol
warn("Branch-range extension: No symbol at offset 0 in InputSection '" +
Copy link
Contributor

@yozhu yozhu Feb 9, 2025

Choose a reason for hiding this comment

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

If no symbol at offset 0, could we create a local fake symbol at offset 0, and later after relocation is done skip emitting this created symbol into symbol table?

Copy link
Contributor Author

@alx32 alx32 Feb 10, 2025

Choose a reason for hiding this comment

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

We could but I haven't encountered this scenario yet so we might not want to add the extra complexity without a way of testing it.

toString(isec) + "', possible branch out of range errors may occur.");
Copy link
Member

@DataCorrupted DataCorrupted Feb 10, 2025

Choose a reason for hiding this comment

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

Suggested change
toString(isec) + "', possible branch out of range errors may occur.");
toString(isec) + "', branch out of range errors may occur.");

"possible" and "may" are redundant.

return nullptr;
} else {
llvm_unreachable("Unexpected referent type");
}
Comment on lines +138 to +140
Copy link
Contributor

@ellishg ellishg Feb 10, 2025

Choose a reason for hiding this comment

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

Suggested change
} else {
llvm_unreachable("Unexpected referent type");
}
}
llvm_unreachable("Unexpected referent type");

}

// Determine whether we need thunks, which depends on the target arch -- RISC
// (i.e., ARM) generally does because it has limited-range branch/call
// instructions, whereas CISC (i.e., x86) generally doesn't. RISC only needs
Expand Down Expand Up @@ -145,7 +169,10 @@ bool TextOutputSection::needsThunks() const {
for (Reloc &r : isec->relocs) {
if (!target->hasAttr(r.type, RelocAttrBits::BRANCH))
continue;
auto *sym = cast<Symbol *>(r.referent);
// Get the Symbol that the relocation targets.
Symbol *sym = getReferentSymbol(r);
if (!sym)
continue;
// Pre-populate the thunkMap and memoize call site counts for every
// InputSection and ThunkInfo. We do this for the benefit of
// estimateStubsInRangeVA().
Expand Down Expand Up @@ -325,7 +352,10 @@ void TextOutputSection::finalize() {
backwardBranchRange < callVA ? callVA - backwardBranchRange : 0;
uint64_t highVA = callVA + forwardBranchRange;
// Calculate our call referent address
auto *funcSym = cast<Symbol *>(r.referent);
Symbol *funcSym = getReferentSymbol(r);
if (!funcSym)
continue;

ThunkInfo &thunkInfo = thunkMap[funcSym];
// The referent is not reachable, so we need to use a thunk ...
if (funcSym->isInStubs() && callVA >= stubsInRangeVA) {
Expand Down