[lld-macho] Fix compatibility between --icf=safe_thunks and --keep-icf-stabs #116687
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-lld-macho Author: None (alx32) ChangesCurrently when With this change, if Full diff: https://github.com/llvm/llvm-project/pull/116687.diff 7 Files Affected:
diff --git a/lld/MachO/Arch/ARM64.cpp b/lld/MachO/Arch/ARM64.cpp
index 195a8f09f47c1a..882873ae5de0c1 100644
--- a/lld/MachO/Arch/ARM64.cpp
+++ b/lld/MachO/Arch/ARM64.cpp
@@ -44,6 +44,7 @@ struct ARM64 : ARM64Common {
void initICFSafeThunkBody(InputSection *thunk,
InputSection *branchTarget) const override;
+ InputSection *getThunkBranchTarget(InputSection *thunk) const override;
uint32_t getICFSafeThunkSize() const override;
};
@@ -197,6 +198,16 @@ void ARM64::initICFSafeThunkBody(InputSection *thunk,
/*referent=*/branchTarget);
}
+InputSection *ARM64::getThunkBranchTarget(InputSection *thunk) const {
+ assert(thunk->relocs.size() == 1 &&
+ "expected a single reloc on ARM64 ICF thunk");
+ auto &reloc = thunk->relocs[0];
+ assert(reloc.referent.is<InputSection *>() &&
+ "ARM64 thunk reloc is expected to point to an InputSection");
+
+ return reloc.referent.dyn_cast<InputSection *>();
+}
+
uint32_t ARM64::getICFSafeThunkSize() const { return sizeof(icfSafeThunkCode); }
ARM64::ARM64() : ARM64Common(LP64()) {
diff --git a/lld/MachO/ICF.cpp b/lld/MachO/ICF.cpp
index aedaecfdeb2c01..451aaa7caaf0c1 100644
--- a/lld/MachO/ICF.cpp
+++ b/lld/MachO/ICF.cpp
@@ -481,6 +481,34 @@ void macho::markAddrSigSymbols() {
}
}
+// Given a symbol that was folded into a thunk, return the symbol pointing to
+// the actual body of the function. We use this approach rather than storing the
+// needed info in the Defined itself in order to minimize memory usage.
+Defined *macho::getBodyForThunkFoldedSym(Defined *foldedSym) {
+ assert(isa<ConcatInputSection>(foldedSym->originalIsec) &&
+ "thunk-folded ICF symbol expected to be on a ConcatInputSection");
+ // foldedSec is the InputSection that was marked as deleted upon fold
+ ConcatInputSection *foldedSec =
+ cast<ConcatInputSection>(foldedSym->originalIsec);
+
+ // thunkBody is the actual live thunk, containing the code that branches to
+ // the actual body of the function.
+ InputSection *thunkBody = foldedSec->replacement;
+
+ // the actual (merged) body of the function that the thunk jumps to. This will
+ // end up in the final binary.
+ InputSection *functionBody = target->getThunkBranchTarget(thunkBody);
+
+ for (Symbol *sym : functionBody->symbols) {
+ Defined *d = dyn_cast<Defined>(sym);
+ // The symbol needs to be at the start of the InputSection
+ if (d && d->value == 0)
+ return d;
+ }
+
+ llvm_unreachable("could not find body symbol for ICF-generated thunk");
+ return nullptr;
+}
void macho::foldIdenticalSections(bool onlyCfStrings) {
TimeTraceScope timeScope("Fold Identical Code Sections");
// The ICF equivalence-class segregation algorithm relies on pre-computed
diff --git a/lld/MachO/ICF.h b/lld/MachO/ICF.h
index 34ceb1cf284bf3..e382fd6c60956c 100644
--- a/lld/MachO/ICF.h
+++ b/lld/MachO/ICF.h
@@ -15,11 +15,17 @@
namespace lld::macho {
class Symbol;
+class Defined;
void markAddrSigSymbols();
void markSymAsAddrSig(Symbol *s);
void foldIdenticalSections(bool onlyCfStrings);
+// Given a symbol that was folded into a thunk, return the symbol pointing to
+// the actual body of the function. We expose this function to allow getting the
+// main function body for a symbol that was folded via a thunk.
+Defined *getBodyForThunkFoldedSym(Defined *foldedSym);
+
} // namespace lld::macho
#endif
diff --git a/lld/MachO/SyntheticSections.cpp b/lld/MachO/SyntheticSections.cpp
index eee87b3a6cb4de..7e0ba8d501c994 100644
--- a/lld/MachO/SyntheticSections.cpp
+++ b/lld/MachO/SyntheticSections.cpp
@@ -10,6 +10,7 @@
#include "ConcatOutputSection.h"
#include "Config.h"
#include "ExportTrie.h"
+#include "ICF.h"
#include "InputFiles.h"
#include "MachOStructs.h"
#include "ObjC.h"
@@ -1204,6 +1205,14 @@ void SymtabSection::emitEndFunStab(Defined *defined) {
stabs.emplace_back(std::move(stab));
}
+Defined *SymtabSection::getFuncBodySym(Defined *originalSym) {
+ // If the Defined is not a thunk, we can use it directly
+ if (originalSym->identicalCodeFoldingKind != Symbol::ICFFoldKind::Thunk)
+ return originalSym;
+
+ return macho::getBodyForThunkFoldedSym(originalSym);
+}
+
void SymtabSection::emitStabs() {
if (config->omitDebugInfo)
return;
@@ -1214,9 +1223,14 @@ void SymtabSection::emitStabs() {
stabs.emplace_back(std::move(astStab));
}
- // Cache the file ID for each symbol in an std::pair for faster sorting.
- using SortingPair = std::pair<Defined *, int>;
- std::vector<SortingPair> symbolsNeedingStabs;
+ struct SymbolStabInfo {
+ Defined *originalSym; // Original Defined symbol - this may be an ICF thunk
+ int fileId; // File ID associated with the STABS symbol
+ Defined *mainBodySym; // Symbol that consists of the full function body -
+ // use this for the STABS entry
+ };
+
+ std::vector<SymbolStabInfo> symbolsNeedingStabs;
for (const SymtabEntry &entry :
concat<SymtabEntry>(localSymbols, externalSymbols)) {
Symbol *sym = entry.sym;
@@ -1229,20 +1243,10 @@ void SymtabSection::emitStabs() {
if (defined->isAbsolute())
continue;
- // Never generate a STABS entry for a symbol that has been ICF'ed using a
- // thunk, just as we do for fully ICF'ed functions. Otherwise, we end up
- // generating invalid DWARF as dsymutil will assume the entire function
- // body is at that location, when, in reality, only the thunk is
- // present. This will end up causing overlapping DWARF entries.
- // TODO: Find an implementation that works in combination with
- // `--keep-icf-stabs`.
- if (defined->identicalCodeFoldingKind == Symbol::ICFFoldKind::Thunk)
- continue;
-
// Constant-folded symbols go in the executable's symbol table, but don't
- // get a stabs entry unless --keep-icf-stabs flag is specified
+ // get a stabs entry unless --keep-icf-stabs flag is specified.
if (!config->keepICFStabs &&
- defined->identicalCodeFoldingKind == Symbol::ICFFoldKind::Body)
+ defined->identicalCodeFoldingKind != Symbol::ICFFoldKind::None)
continue;
ObjFile *file = defined->getObjectFile();
@@ -1251,26 +1255,26 @@ void SymtabSection::emitStabs() {
// We use 'originalIsec' to get the file id of the symbol since 'isec()'
// might point to the merged ICF symbol's file
- symbolsNeedingStabs.emplace_back(defined,
- defined->originalIsec->getFile()->id);
+ Defined *funcBodySym = getFuncBodySym(defined);
+ symbolsNeedingStabs.emplace_back(SymbolStabInfo{
+ defined, funcBodySym->originalIsec->getFile()->id, funcBodySym});
}
}
llvm::stable_sort(symbolsNeedingStabs,
- [&](const SortingPair &a, const SortingPair &b) {
- return a.second < b.second;
+ [&](const SymbolStabInfo &a, const SymbolStabInfo &b) {
+ return a.fileId < b.fileId;
});
// Emit STABS symbols so that dsymutil and/or the debugger can map address
// regions in the final binary to the source and object files from which they
// originated.
InputFile *lastFile = nullptr;
- for (SortingPair &pair : symbolsNeedingStabs) {
- Defined *defined = pair.first;
+ for (const SymbolStabInfo &info : symbolsNeedingStabs) {
// We use 'originalIsec' of the symbol since we care about the actual origin
// of the symbol, not the canonical location returned by `isec()`.
- InputSection *isec = defined->originalIsec;
- ObjFile *file = cast<ObjFile>(isec->getFile());
+ InputSection *bodyIsec = info.mainBodySym->originalIsec;
+ ObjFile *file = cast<ObjFile>(bodyIsec->getFile());
if (lastFile == nullptr || lastFile != file) {
if (lastFile != nullptr)
@@ -1282,16 +1286,16 @@ void SymtabSection::emitStabs() {
}
StabsEntry symStab;
- symStab.sect = isec->parent->index;
- symStab.strx = stringTableSection.addString(defined->getName());
- symStab.value = defined->getVA();
+ symStab.sect = bodyIsec->parent->index;
+ symStab.strx = stringTableSection.addString(info.originalSym->getName());
+ symStab.value = info.mainBodySym->getVA();
- if (isCodeSection(isec)) {
+ if (isCodeSection(bodyIsec)) {
symStab.type = N_FUN;
stabs.emplace_back(std::move(symStab));
- emitEndFunStab(defined);
+ emitEndFunStab(info.mainBodySym);
} else {
- symStab.type = defined->isExternal() ? N_GSYM : N_STSYM;
+ symStab.type = info.originalSym->isExternal() ? N_GSYM : N_STSYM;
stabs.emplace_back(std::move(symStab));
}
}
diff --git a/lld/MachO/SyntheticSections.h b/lld/MachO/SyntheticSections.h
index a4c7f58481aa14..af99f22788d6e9 100644
--- a/lld/MachO/SyntheticSections.h
+++ b/lld/MachO/SyntheticSections.h
@@ -485,6 +485,7 @@ class SymtabSection : public LinkEditSection {
void emitEndSourceStab();
void emitObjectFileStab(ObjFile *);
void emitEndFunStab(Defined *);
+ Defined *getFuncBodySym(Defined *);
void emitStabs();
protected:
diff --git a/lld/MachO/Target.h b/lld/MachO/Target.h
index eaa0336e70cb6b..b5b80e083a6c34 100644
--- a/lld/MachO/Target.h
+++ b/lld/MachO/Target.h
@@ -80,6 +80,12 @@ class TargetInfo {
llvm_unreachable("target does not support ICF safe thunks");
}
+ // Given a thunk for which `initICFSafeThunkBody` was called, return the
+ // branchTarget it was initialized with.
+ virtual InputSection *getThunkBranchTarget(InputSection *thunk) const {
+ llvm_unreachable("target does not support ICF safe thunks");
+ }
+
virtual uint32_t getICFSafeThunkSize() const {
llvm_unreachable("target does not support ICF safe thunks");
}
diff --git a/lld/test/MachO/icf-safe-thunks-dwarf.ll b/lld/test/MachO/icf-safe-thunks-dwarf.ll
index 1e4422a3313239..9edad60946837e 100644
--- a/lld/test/MachO/icf-safe-thunks-dwarf.ll
+++ b/lld/test/MachO/icf-safe-thunks-dwarf.ll
@@ -20,6 +20,59 @@
; VERIFY-STABS: N_FUN{{.*}}_func_A
; VERIFY-STABS: N_FUN{{.*}}_take_func_addr
+;;;;;;;;;;;;;;;;;;;;;;;;;;;; Check safe_thunks ICF + keeping STABS entries ;;;;;;;;;;;;;;;;;;;;;;;;;;;;
+
+;;; Check scenario with where we do safe_thunks ICF and also generate STABS entries
+; RUN: %lld -arch arm64 -lSystem --icf=safe_thunks --keep-icf-stabs -dylib -o %t/a_thunks.dylib %t/a.o
+; RUN: dsymutil -s %t/a_thunks.dylib > %t/a_thunks.txt
+
+
+; RUN: dsymutil --flat --verify-dwarf=none %t/a_thunks.dylib -o %t/a_thunks.dSYM
+; RUN: dsymutil -s %t/a_thunks.dSYM >> %t/a_thunks.txt
+; RUN: llvm-dwarfdump -a %t/a_thunks.dSYM >> %t/a_thunks.txt
+
+; RUN: cat %t/a_thunks.txt | FileCheck %s --check-prefix=VERIFY-THUNK
+
+# VERIFY-THUNK-LABEL: Symbol table for: '{{.*}}/a_thunks.dylib'
+# Capture the 'n_value's for N_FUN entries of _func_A, _func_B, and _func_C
+# VERIFY-THUNK: [[MERGED_FUN_ADDR:[0-9a-f]+]] '_func_A'
+# VERIFY-THUNK: [[MERGED_FUN_ADDR]] '_func_B'
+# VERIFY-THUNK: [[MERGED_FUN_ADDR]] '_func_C'
+
+# Capture the 'n_value's for SECT EXT entries in the first part
+# VERIFY-THUNK: SECT EXT{{.*}} [[SECT_EXT_A_NVALUE:[0-9a-f]+]] '_func_A'
+# VERIFY-THUNK: SECT EXT{{.*}} [[SECT_EXT_B_NVALUE:[0-9a-f]+]] '_func_B'
+# VERIFY-THUNK: SECT EXT{{.*}} [[SECT_EXT_C_NVALUE:[0-9a-f]+]] '_func_C'
+
+# VERIFY-THUNK: ----------------------------------------------------------------------
+# VERIFY-THUNK-LABEL: Symbol table for: '{{.*}}/a_thunks.dSYM'
+
+# Verify that the SECT EXT 'n_value's in the second part match the first part
+# VERIFY-THUNK: SECT EXT{{.*}} [[SECT_EXT_A_NVALUE]] '_func_A'
+# VERIFY-THUNK: SECT EXT{{.*}} [[SECT_EXT_B_NVALUE]] '_func_B'
+# VERIFY-THUNK: SECT EXT{{.*}} [[SECT_EXT_C_NVALUE]] '_func_C'
+
+# Ensure that N_FUN 'n_value's match the DW_TAG_subprogram's DW_AT_low_pc
+# and that the DW_AT_name is at a specific relative position
+
+# VERIFY-THUNK-LABEL: .debug_info contents:
+# VERIFY-THUNK: Compile Unit: length = {{.*}}
+
+# Match the subprogram for func_A
+# VERIFY-THUNK: : DW_TAG_subprogram
+# VERIFY-THUNK-NEXT: {{ +}}DW_AT_low_pc (0x[[MERGED_FUN_ADDR]])
+# VERIFY-THUNK-NEXT-NEXT-NEXT-NEXT-NEXT: {{ +}}DW_AT_name ("func_A")
+
+# Match the subprogram for func_B
+# VERIFY-THUNK: : DW_TAG_subprogram
+# VERIFY-THUNK-NEXT: {{ +}}DW_AT_low_pc (0x[[MERGED_FUN_ADDR]])
+# VERIFY-THUNK-NEXT-NEXT-NEXT-NEXT-NEXT: {{ +}}DW_AT_name ("func_B")
+
+# Match the subprogram for func_C
+# VERIFY-THUNK: : DW_TAG_subprogram
+# VERIFY-THUNK-NEXT: {{ +}}DW_AT_low_pc (0x[[MERGED_FUN_ADDR]])
+# VERIFY-THUNK-NEXT-NEXT-NEXT-NEXT-NEXT: {{ +}}DW_AT_name ("func_C")
+
;--- a.cpp
#define ATTR __attribute__((noinline)) extern "C"
typedef unsigned long long ULL;
|
|
@llvm/pr-subscribers-lld Author: None (alx32) ChangesCurrently when With this change, if Full diff: https://github.com/llvm/llvm-project/pull/116687.diff 7 Files Affected:
diff --git a/lld/MachO/Arch/ARM64.cpp b/lld/MachO/Arch/ARM64.cpp
index 195a8f09f47c1a..882873ae5de0c1 100644
--- a/lld/MachO/Arch/ARM64.cpp
+++ b/lld/MachO/Arch/ARM64.cpp
@@ -44,6 +44,7 @@ struct ARM64 : ARM64Common {
void initICFSafeThunkBody(InputSection *thunk,
InputSection *branchTarget) const override;
+ InputSection *getThunkBranchTarget(InputSection *thunk) const override;
uint32_t getICFSafeThunkSize() const override;
};
@@ -197,6 +198,16 @@ void ARM64::initICFSafeThunkBody(InputSection *thunk,
/*referent=*/branchTarget);
}
+InputSection *ARM64::getThunkBranchTarget(InputSection *thunk) const {
+ assert(thunk->relocs.size() == 1 &&
+ "expected a single reloc on ARM64 ICF thunk");
+ auto &reloc = thunk->relocs[0];
+ assert(reloc.referent.is<InputSection *>() &&
+ "ARM64 thunk reloc is expected to point to an InputSection");
+
+ return reloc.referent.dyn_cast<InputSection *>();
+}
+
uint32_t ARM64::getICFSafeThunkSize() const { return sizeof(icfSafeThunkCode); }
ARM64::ARM64() : ARM64Common(LP64()) {
diff --git a/lld/MachO/ICF.cpp b/lld/MachO/ICF.cpp
index aedaecfdeb2c01..451aaa7caaf0c1 100644
--- a/lld/MachO/ICF.cpp
+++ b/lld/MachO/ICF.cpp
@@ -481,6 +481,34 @@ void macho::markAddrSigSymbols() {
}
}
+// Given a symbol that was folded into a thunk, return the symbol pointing to
+// the actual body of the function. We use this approach rather than storing the
+// needed info in the Defined itself in order to minimize memory usage.
+Defined *macho::getBodyForThunkFoldedSym(Defined *foldedSym) {
+ assert(isa<ConcatInputSection>(foldedSym->originalIsec) &&
+ "thunk-folded ICF symbol expected to be on a ConcatInputSection");
+ // foldedSec is the InputSection that was marked as deleted upon fold
+ ConcatInputSection *foldedSec =
+ cast<ConcatInputSection>(foldedSym->originalIsec);
+
+ // thunkBody is the actual live thunk, containing the code that branches to
+ // the actual body of the function.
+ InputSection *thunkBody = foldedSec->replacement;
+
+ // the actual (merged) body of the function that the thunk jumps to. This will
+ // end up in the final binary.
+ InputSection *functionBody = target->getThunkBranchTarget(thunkBody);
+
+ for (Symbol *sym : functionBody->symbols) {
+ Defined *d = dyn_cast<Defined>(sym);
+ // The symbol needs to be at the start of the InputSection
+ if (d && d->value == 0)
+ return d;
+ }
+
+ llvm_unreachable("could not find body symbol for ICF-generated thunk");
+ return nullptr;
+}
void macho::foldIdenticalSections(bool onlyCfStrings) {
TimeTraceScope timeScope("Fold Identical Code Sections");
// The ICF equivalence-class segregation algorithm relies on pre-computed
diff --git a/lld/MachO/ICF.h b/lld/MachO/ICF.h
index 34ceb1cf284bf3..e382fd6c60956c 100644
--- a/lld/MachO/ICF.h
+++ b/lld/MachO/ICF.h
@@ -15,11 +15,17 @@
namespace lld::macho {
class Symbol;
+class Defined;
void markAddrSigSymbols();
void markSymAsAddrSig(Symbol *s);
void foldIdenticalSections(bool onlyCfStrings);
+// Given a symbol that was folded into a thunk, return the symbol pointing to
+// the actual body of the function. We expose this function to allow getting the
+// main function body for a symbol that was folded via a thunk.
+Defined *getBodyForThunkFoldedSym(Defined *foldedSym);
+
} // namespace lld::macho
#endif
diff --git a/lld/MachO/SyntheticSections.cpp b/lld/MachO/SyntheticSections.cpp
index eee87b3a6cb4de..7e0ba8d501c994 100644
--- a/lld/MachO/SyntheticSections.cpp
+++ b/lld/MachO/SyntheticSections.cpp
@@ -10,6 +10,7 @@
#include "ConcatOutputSection.h"
#include "Config.h"
#include "ExportTrie.h"
+#include "ICF.h"
#include "InputFiles.h"
#include "MachOStructs.h"
#include "ObjC.h"
@@ -1204,6 +1205,14 @@ void SymtabSection::emitEndFunStab(Defined *defined) {
stabs.emplace_back(std::move(stab));
}
+Defined *SymtabSection::getFuncBodySym(Defined *originalSym) {
+ // If the Defined is not a thunk, we can use it directly
+ if (originalSym->identicalCodeFoldingKind != Symbol::ICFFoldKind::Thunk)
+ return originalSym;
+
+ return macho::getBodyForThunkFoldedSym(originalSym);
+}
+
void SymtabSection::emitStabs() {
if (config->omitDebugInfo)
return;
@@ -1214,9 +1223,14 @@ void SymtabSection::emitStabs() {
stabs.emplace_back(std::move(astStab));
}
- // Cache the file ID for each symbol in an std::pair for faster sorting.
- using SortingPair = std::pair<Defined *, int>;
- std::vector<SortingPair> symbolsNeedingStabs;
+ struct SymbolStabInfo {
+ Defined *originalSym; // Original Defined symbol - this may be an ICF thunk
+ int fileId; // File ID associated with the STABS symbol
+ Defined *mainBodySym; // Symbol that consists of the full function body -
+ // use this for the STABS entry
+ };
+
+ std::vector<SymbolStabInfo> symbolsNeedingStabs;
for (const SymtabEntry &entry :
concat<SymtabEntry>(localSymbols, externalSymbols)) {
Symbol *sym = entry.sym;
@@ -1229,20 +1243,10 @@ void SymtabSection::emitStabs() {
if (defined->isAbsolute())
continue;
- // Never generate a STABS entry for a symbol that has been ICF'ed using a
- // thunk, just as we do for fully ICF'ed functions. Otherwise, we end up
- // generating invalid DWARF as dsymutil will assume the entire function
- // body is at that location, when, in reality, only the thunk is
- // present. This will end up causing overlapping DWARF entries.
- // TODO: Find an implementation that works in combination with
- // `--keep-icf-stabs`.
- if (defined->identicalCodeFoldingKind == Symbol::ICFFoldKind::Thunk)
- continue;
-
// Constant-folded symbols go in the executable's symbol table, but don't
- // get a stabs entry unless --keep-icf-stabs flag is specified
+ // get a stabs entry unless --keep-icf-stabs flag is specified.
if (!config->keepICFStabs &&
- defined->identicalCodeFoldingKind == Symbol::ICFFoldKind::Body)
+ defined->identicalCodeFoldingKind != Symbol::ICFFoldKind::None)
continue;
ObjFile *file = defined->getObjectFile();
@@ -1251,26 +1255,26 @@ void SymtabSection::emitStabs() {
// We use 'originalIsec' to get the file id of the symbol since 'isec()'
// might point to the merged ICF symbol's file
- symbolsNeedingStabs.emplace_back(defined,
- defined->originalIsec->getFile()->id);
+ Defined *funcBodySym = getFuncBodySym(defined);
+ symbolsNeedingStabs.emplace_back(SymbolStabInfo{
+ defined, funcBodySym->originalIsec->getFile()->id, funcBodySym});
}
}
llvm::stable_sort(symbolsNeedingStabs,
- [&](const SortingPair &a, const SortingPair &b) {
- return a.second < b.second;
+ [&](const SymbolStabInfo &a, const SymbolStabInfo &b) {
+ return a.fileId < b.fileId;
});
// Emit STABS symbols so that dsymutil and/or the debugger can map address
// regions in the final binary to the source and object files from which they
// originated.
InputFile *lastFile = nullptr;
- for (SortingPair &pair : symbolsNeedingStabs) {
- Defined *defined = pair.first;
+ for (const SymbolStabInfo &info : symbolsNeedingStabs) {
// We use 'originalIsec' of the symbol since we care about the actual origin
// of the symbol, not the canonical location returned by `isec()`.
- InputSection *isec = defined->originalIsec;
- ObjFile *file = cast<ObjFile>(isec->getFile());
+ InputSection *bodyIsec = info.mainBodySym->originalIsec;
+ ObjFile *file = cast<ObjFile>(bodyIsec->getFile());
if (lastFile == nullptr || lastFile != file) {
if (lastFile != nullptr)
@@ -1282,16 +1286,16 @@ void SymtabSection::emitStabs() {
}
StabsEntry symStab;
- symStab.sect = isec->parent->index;
- symStab.strx = stringTableSection.addString(defined->getName());
- symStab.value = defined->getVA();
+ symStab.sect = bodyIsec->parent->index;
+ symStab.strx = stringTableSection.addString(info.originalSym->getName());
+ symStab.value = info.mainBodySym->getVA();
- if (isCodeSection(isec)) {
+ if (isCodeSection(bodyIsec)) {
symStab.type = N_FUN;
stabs.emplace_back(std::move(symStab));
- emitEndFunStab(defined);
+ emitEndFunStab(info.mainBodySym);
} else {
- symStab.type = defined->isExternal() ? N_GSYM : N_STSYM;
+ symStab.type = info.originalSym->isExternal() ? N_GSYM : N_STSYM;
stabs.emplace_back(std::move(symStab));
}
}
diff --git a/lld/MachO/SyntheticSections.h b/lld/MachO/SyntheticSections.h
index a4c7f58481aa14..af99f22788d6e9 100644
--- a/lld/MachO/SyntheticSections.h
+++ b/lld/MachO/SyntheticSections.h
@@ -485,6 +485,7 @@ class SymtabSection : public LinkEditSection {
void emitEndSourceStab();
void emitObjectFileStab(ObjFile *);
void emitEndFunStab(Defined *);
+ Defined *getFuncBodySym(Defined *);
void emitStabs();
protected:
diff --git a/lld/MachO/Target.h b/lld/MachO/Target.h
index eaa0336e70cb6b..b5b80e083a6c34 100644
--- a/lld/MachO/Target.h
+++ b/lld/MachO/Target.h
@@ -80,6 +80,12 @@ class TargetInfo {
llvm_unreachable("target does not support ICF safe thunks");
}
+ // Given a thunk for which `initICFSafeThunkBody` was called, return the
+ // branchTarget it was initialized with.
+ virtual InputSection *getThunkBranchTarget(InputSection *thunk) const {
+ llvm_unreachable("target does not support ICF safe thunks");
+ }
+
virtual uint32_t getICFSafeThunkSize() const {
llvm_unreachable("target does not support ICF safe thunks");
}
diff --git a/lld/test/MachO/icf-safe-thunks-dwarf.ll b/lld/test/MachO/icf-safe-thunks-dwarf.ll
index 1e4422a3313239..9edad60946837e 100644
--- a/lld/test/MachO/icf-safe-thunks-dwarf.ll
+++ b/lld/test/MachO/icf-safe-thunks-dwarf.ll
@@ -20,6 +20,59 @@
; VERIFY-STABS: N_FUN{{.*}}_func_A
; VERIFY-STABS: N_FUN{{.*}}_take_func_addr
+;;;;;;;;;;;;;;;;;;;;;;;;;;;; Check safe_thunks ICF + keeping STABS entries ;;;;;;;;;;;;;;;;;;;;;;;;;;;;
+
+;;; Check scenario with where we do safe_thunks ICF and also generate STABS entries
+; RUN: %lld -arch arm64 -lSystem --icf=safe_thunks --keep-icf-stabs -dylib -o %t/a_thunks.dylib %t/a.o
+; RUN: dsymutil -s %t/a_thunks.dylib > %t/a_thunks.txt
+
+
+; RUN: dsymutil --flat --verify-dwarf=none %t/a_thunks.dylib -o %t/a_thunks.dSYM
+; RUN: dsymutil -s %t/a_thunks.dSYM >> %t/a_thunks.txt
+; RUN: llvm-dwarfdump -a %t/a_thunks.dSYM >> %t/a_thunks.txt
+
+; RUN: cat %t/a_thunks.txt | FileCheck %s --check-prefix=VERIFY-THUNK
+
+# VERIFY-THUNK-LABEL: Symbol table for: '{{.*}}/a_thunks.dylib'
+# Capture the 'n_value's for N_FUN entries of _func_A, _func_B, and _func_C
+# VERIFY-THUNK: [[MERGED_FUN_ADDR:[0-9a-f]+]] '_func_A'
+# VERIFY-THUNK: [[MERGED_FUN_ADDR]] '_func_B'
+# VERIFY-THUNK: [[MERGED_FUN_ADDR]] '_func_C'
+
+# Capture the 'n_value's for SECT EXT entries in the first part
+# VERIFY-THUNK: SECT EXT{{.*}} [[SECT_EXT_A_NVALUE:[0-9a-f]+]] '_func_A'
+# VERIFY-THUNK: SECT EXT{{.*}} [[SECT_EXT_B_NVALUE:[0-9a-f]+]] '_func_B'
+# VERIFY-THUNK: SECT EXT{{.*}} [[SECT_EXT_C_NVALUE:[0-9a-f]+]] '_func_C'
+
+# VERIFY-THUNK: ----------------------------------------------------------------------
+# VERIFY-THUNK-LABEL: Symbol table for: '{{.*}}/a_thunks.dSYM'
+
+# Verify that the SECT EXT 'n_value's in the second part match the first part
+# VERIFY-THUNK: SECT EXT{{.*}} [[SECT_EXT_A_NVALUE]] '_func_A'
+# VERIFY-THUNK: SECT EXT{{.*}} [[SECT_EXT_B_NVALUE]] '_func_B'
+# VERIFY-THUNK: SECT EXT{{.*}} [[SECT_EXT_C_NVALUE]] '_func_C'
+
+# Ensure that N_FUN 'n_value's match the DW_TAG_subprogram's DW_AT_low_pc
+# and that the DW_AT_name is at a specific relative position
+
+# VERIFY-THUNK-LABEL: .debug_info contents:
+# VERIFY-THUNK: Compile Unit: length = {{.*}}
+
+# Match the subprogram for func_A
+# VERIFY-THUNK: : DW_TAG_subprogram
+# VERIFY-THUNK-NEXT: {{ +}}DW_AT_low_pc (0x[[MERGED_FUN_ADDR]])
+# VERIFY-THUNK-NEXT-NEXT-NEXT-NEXT-NEXT: {{ +}}DW_AT_name ("func_A")
+
+# Match the subprogram for func_B
+# VERIFY-THUNK: : DW_TAG_subprogram
+# VERIFY-THUNK-NEXT: {{ +}}DW_AT_low_pc (0x[[MERGED_FUN_ADDR]])
+# VERIFY-THUNK-NEXT-NEXT-NEXT-NEXT-NEXT: {{ +}}DW_AT_name ("func_B")
+
+# Match the subprogram for func_C
+# VERIFY-THUNK: : DW_TAG_subprogram
+# VERIFY-THUNK-NEXT: {{ +}}DW_AT_low_pc (0x[[MERGED_FUN_ADDR]])
+# VERIFY-THUNK-NEXT-NEXT-NEXT-NEXT-NEXT: {{ +}}DW_AT_name ("func_C")
+
;--- a.cpp
#define ATTR __attribute__((noinline)) extern "C"
typedef unsigned long long ULL;
|
ellishg
reviewed
Nov 18, 2024
There was a problem hiding this comment.
Also CC @thevinster and @keith who have worked in these files before.
ellishg
reviewed
Nov 19, 2024
ellishg
reviewed
Nov 19, 2024
lld/MachO/SyntheticSections.cpp
Outdated
| // symbol itself, but if the symbol was folded using a thunk, we retrieve the | ||
| // target function body from the thunk. | ||
| Defined *SymtabSection::getFuncBodySym(Defined *originalSym) { | ||
| if (originalSym->identicalCodeFoldingKind != Symbol::ICFFoldKind::Thunk) |
There was a problem hiding this comment.
This would be slightly more intuitive to me if the condition was flipped.
There was a problem hiding this comment.
Oh sorry, I meant that you should flip the condition and the return. So that if it is a thunk, we return the thunk. Otherwise we return the original symbol.
kyulee-com
reviewed
Nov 19, 2024
kyulee-com
reviewed
Nov 19, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently when
--icf=safe_thunksis used,STABSentries cannot be generated for ICF'ed functions. This is because if ICF converts a full function into a thunk and then we generate aSTABSentry for the thunk,dsymutilwill expect to find the entire function body at the location of the thunk. Because just a thunk will be present at the location of theSTABSentry - dsymutil will generate invalid debug info for such scenarios.With this change, if
--icf=safe_thunksis used and--keep-icf-stabsis also specified, STABS entries will be created for all functions, even merged ones. However, the STABS entries will point at the actual (full) function body while having the name of the thunk. This way we still get program correctness as well as correct DWARF data. When doing this, the debug data will be identical to the scenario where we're using--icf=alland--keep-icf-stabs, but the actual program will also contain thunks, which won't show up in the DWARF data.