[llvm] [lldb] [lldb] Fix missing comsumeError() with LLDB_LOG in ObjectFileCOFF/PECOFF (PR #70793)
Stefan Gränitz via llvm-commits
llvm-commits at lists.llvm.org
Tue Oct 31 05:29:15 PDT 2023
https://github.com/weliveindetail created https://github.com/llvm/llvm-project/pull/70793
I actually ran into it with a downstream fork:
```
llvm::Error::fatalUncheckedError(), llvm-project\llvm\lib\Support\Error.cpp, line 117
ObjectFilePECOFF::AppendFromCOFFSymbolTable(), llvm-project\lldb\source\Plugins\ObjectFile\PECOFF\ObjectFilePECOFF.cpp, line 806
ObjectFilePECOFF::ParseSymtab(), llvm-project\lldb\source\Plugins\ObjectFile\PECOFF\ObjectFilePECOFF.cpp, line 777
```
If logging is disabled `LLDB_LOG_ERROR` calls `llvm::consumeError()`, which marks the error as checked. All `llvm::Error`s must be checked before destruction. This patch fixes one more such case in `ObjectFileCOFF::ParseSymtab()`.
>From 923cabecd8486514f00c9cf81263169e5b263ef0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Stefan=20Gr=C3=A4nitz?= <stefan.graenitz at gmail.com>
Date: Wed, 18 Oct 2023 14:27:51 +0200
Subject: [PATCH 1/4] [llvm-jitlink] Avoid assertion failure in make_error
If GOTSym is not defined this failed with:
```
Assertion failed: (Base->isDefined() && "Not a defined symbol"), function getBlock, file JITLink.h, line 554.
```
---
llvm/tools/llvm-jitlink/llvm-jitlink-elf.cpp | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/llvm/tools/llvm-jitlink/llvm-jitlink-elf.cpp b/llvm/tools/llvm-jitlink/llvm-jitlink-elf.cpp
index 7881660d1a73851..2adf6df9b7615e9 100644
--- a/llvm/tools/llvm-jitlink/llvm-jitlink-elf.cpp
+++ b/llvm/tools/llvm-jitlink/llvm-jitlink-elf.cpp
@@ -55,7 +55,11 @@ static Expected<Symbol &> getELFStubTarget(LinkGraph &G, Block &B) {
if (!E)
return E.takeError();
auto &GOTSym = E->getTarget();
- if (!GOTSym.isDefined() || !isELFGOTSection(GOTSym.getBlock().getSection()))
+ if (!GOTSym.isDefined())
+ return make_error<StringError>(
+ "Stubs entry in " + G.getName() + " does not point to GOT entry",
+ inconvertibleErrorCode());
+ if (!isELFGOTSection(GOTSym.getBlock().getSection()))
return make_error<StringError>(
"Stubs entry in " + G.getName() + ", \"" +
GOTSym.getBlock().getSection().getName() +
>From c07c00bc4dc11430b965cdcade028d9375cf8cbc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Stefan=20Gr=C3=A4nitz?= <stefan.graenitz at gmail.com>
Date: Wed, 18 Oct 2023 14:30:11 +0200
Subject: [PATCH 2/4] [JITLink] Fix typos: symobls -> symbols (NFC)
---
llvm/include/llvm-c/Orc.h | 2 +-
llvm/include/llvm/ExecutionEngine/JITLink/JITLink.h | 2 +-
llvm/include/llvm/ExecutionEngine/JITLink/aarch64.h | 2 +-
llvm/include/llvm/ExecutionEngine/JITLink/loongarch.h | 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/llvm/include/llvm-c/Orc.h b/llvm/include/llvm-c/Orc.h
index a40b17b712fb3f2..3d17073dfdd8d41 100644
--- a/llvm/include/llvm-c/Orc.h
+++ b/llvm/include/llvm-c/Orc.h
@@ -346,7 +346,7 @@ typedef struct LLVMOrcOpaqueLookupState *LLVMOrcLookupStateRef;
* into.
*
* The JDLookupFlags argument can be inspected to determine whether the original
- * lookup included non-exported symobls.
+ * lookup included non-exported symbols.
*
* Finally, the LookupSet argument contains the set of symbols that could not
* be found in JD already (the set of generation candidates).
diff --git a/llvm/include/llvm/ExecutionEngine/JITLink/JITLink.h b/llvm/include/llvm/ExecutionEngine/JITLink/JITLink.h
index fb758f7a66cf55f..8a019492c12d593 100644
--- a/llvm/include/llvm/ExecutionEngine/JITLink/JITLink.h
+++ b/llvm/include/llvm/ExecutionEngine/JITLink/JITLink.h
@@ -1862,7 +1862,7 @@ Error makeAlignmentError(llvm::orc::ExecutorAddr Loc, uint64_t Value, int N,
const Edge &E);
/// Creates a new pointer block in the given section and returns an
-/// Anonymous symobl pointing to it.
+/// Anonymous symbol pointing to it.
///
/// The pointer block will have the following default values:
/// alignment: PointerSize
diff --git a/llvm/include/llvm/ExecutionEngine/JITLink/aarch64.h b/llvm/include/llvm/ExecutionEngine/JITLink/aarch64.h
index 8d3f29b545f21ad..27a90ebef3d6d6a 100644
--- a/llvm/include/llvm/ExecutionEngine/JITLink/aarch64.h
+++ b/llvm/include/llvm/ExecutionEngine/JITLink/aarch64.h
@@ -618,7 +618,7 @@ extern const char NullPointerContent[PointerSize];
extern const char PointerJumpStubContent[12];
/// Creates a new pointer block in the given section and returns an
-/// Anonymous symobl pointing to it.
+/// Anonymous symbol pointing to it.
///
/// If InitialTarget is given then an Pointer64 relocation will be added to the
/// block pointing at InitialTarget.
diff --git a/llvm/include/llvm/ExecutionEngine/JITLink/loongarch.h b/llvm/include/llvm/ExecutionEngine/JITLink/loongarch.h
index bec657723a38cf9..26351ed9971cc46 100644
--- a/llvm/include/llvm/ExecutionEngine/JITLink/loongarch.h
+++ b/llvm/include/llvm/ExecutionEngine/JITLink/loongarch.h
@@ -280,7 +280,7 @@ inline ArrayRef<char> getStubBlockContent(LinkGraph &G) {
}
/// Creates a new pointer block in the given section and returns an
-/// Anonymous symobl pointing to it.
+/// Anonymous symbol pointing to it.
///
/// If InitialTarget is given then an Pointer64 relocation will be added to the
/// block pointing at InitialTarget.
>From 1767f99cb93fd3d1f9bf84765564d069cd19a9ee Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Stefan=20Gr=C3=A4nitz?= <stefan.graenitz at gmail.com>
Date: Wed, 18 Oct 2023 14:49:32 +0200
Subject: [PATCH 3/4] [JITLink][AArch32] Wire up GOT/PLT and test existing
implementation of ThumbV7 stubs
---
.../llvm/ExecutionEngine/JITLink/aarch32.h | 31 ++++++++++---
.../ExecutionEngine/JITLink/ELF_aarch32.cpp | 5 ++-
llvm/lib/ExecutionEngine/JITLink/aarch32.cpp | 32 +++++++++++--
.../JITLink/AArch32/ELF_thumb_stubs.s | 45 +++++++++++++++++++
4 files changed, 101 insertions(+), 12 deletions(-)
create mode 100644 llvm/test/ExecutionEngine/JITLink/AArch32/ELF_thumb_stubs.s
diff --git a/llvm/include/llvm/ExecutionEngine/JITLink/aarch32.h b/llvm/include/llvm/ExecutionEngine/JITLink/aarch32.h
index 3f36b53d6684a79..d92b2aea5745397 100644
--- a/llvm/include/llvm/ExecutionEngine/JITLink/aarch32.h
+++ b/llvm/include/llvm/ExecutionEngine/JITLink/aarch32.h
@@ -271,21 +271,37 @@ inline Error applyFixup(LinkGraph &G, Block &B, const Edge &E,
llvm_unreachable("Relocation must be of class Data, Arm or Thumb");
}
-/// Stubs builder for a specific StubsFlavor
+/// Global Offset Table builder
+class GOTTableManager : public TableManager<GOTTableManager> {
+public:
+ static StringRef getSectionName() { return "$__GOT"; }
+
+ bool visitEdge(LinkGraph &G, Block *B, Edge &E);
+ Symbol &createEntry(LinkGraph &G, Symbol &Target);
+
+private:
+ Section &getGOTSection(LinkGraph &G) {
+ if (!GOTSection)
+ GOTSection = &G.createSection(getSectionName(), orc::MemProt::Read);
+ return *GOTSection;
+ }
+
+ Section *GOTSection = nullptr;
+};
+
+/// PLT stubs builder for a specific StubsFlavor
///
/// Right now we only have one default stub kind, but we want to extend this
/// and allow creation of specific kinds in the future (e.g. branch range
/// extension or interworking).
///
-/// Let's keep it simple for the moment and not wire this through a GOT.
-///
template <StubsFlavor Flavor>
-class StubsManager : public TableManager<StubsManager<Flavor>> {
+class PLTTableManager : public TableManager<PLTTableManager<Flavor>> {
public:
- StubsManager() = default;
+ PLTTableManager(GOTTableManager &GOT) : GOT(GOT) {}
/// Name of the object file section that will contain all our stubs.
- static StringRef getSectionName() { return "__llvm_jitlink_STUBS"; }
+ static StringRef getSectionName() { return "$__STUBS"; }
/// Implements link-graph traversal via visitExistingEdges().
bool visitEdge(LinkGraph &G, Block *B, Edge &E) {
@@ -328,12 +344,13 @@ class StubsManager : public TableManager<StubsManager<Flavor>> {
return *StubsSection;
}
+ GOTTableManager &GOT;
Section *StubsSection = nullptr;
};
/// Create a branch range extension stub with Thumb encoding for v7 CPUs.
template <>
-Symbol &StubsManager<Thumbv7>::createEntry(LinkGraph &G, Symbol &Target);
+Symbol &PLTTableManager<Thumbv7>::createEntry(LinkGraph &G, Symbol &Target);
} // namespace aarch32
} // namespace jitlink
diff --git a/llvm/lib/ExecutionEngine/JITLink/ELF_aarch32.cpp b/llvm/lib/ExecutionEngine/JITLink/ELF_aarch32.cpp
index 23946c7de9adbff..2ec9c238b2d14b7 100644
--- a/llvm/lib/ExecutionEngine/JITLink/ELF_aarch32.cpp
+++ b/llvm/lib/ExecutionEngine/JITLink/ELF_aarch32.cpp
@@ -214,8 +214,9 @@ template <aarch32::StubsFlavor Flavor>
Error buildTables_ELF_aarch32(LinkGraph &G) {
LLVM_DEBUG(dbgs() << "Visiting edges in graph:\n");
- aarch32::StubsManager<Flavor> PLT;
- visitExistingEdges(G, PLT);
+ aarch32::GOTTableManager GOT;
+ aarch32::PLTTableManager<Flavor> PLT(GOT);
+ visitExistingEdges(G, GOT, PLT);
return Error::success();
}
diff --git a/llvm/lib/ExecutionEngine/JITLink/aarch32.cpp b/llvm/lib/ExecutionEngine/JITLink/aarch32.cpp
index 4aed64966654420..b31dd5c22b2eb03 100644
--- a/llvm/lib/ExecutionEngine/JITLink/aarch32.cpp
+++ b/llvm/lib/ExecutionEngine/JITLink/aarch32.cpp
@@ -619,6 +619,31 @@ Error applyFixupThumb(LinkGraph &G, Block &B, const Edge &E,
}
}
+static Symbol &createAnonymousPointer(LinkGraph &G, Section &PointerSection,
+ Symbol *InitialTarget,
+ uint64_t InitialAddend = 0) {
+ // TODO: Do we have an alignment of 4 in ARM and if so how do we distinguish
+ // it at this place?
+ constexpr uint8_t AlignmentThumb = 2;
+ constexpr orc::ExecutorAddr MaxAddrThumb{~uint32_t(AlignmentThumb - 1)};
+ constexpr uint64_t PointerSize = 4;
+ constexpr char NullPointerContent[] { 0x00, 0x00, 0x00, 0x00 };
+ auto &B = G.createContentBlock(PointerSection, NullPointerContent,
+ MaxAddrThumb, AlignmentThumb, 0);
+ if (InitialTarget)
+ B.addEdge(Data_Pointer32, 0, *InitialTarget, InitialAddend);
+ return G.addAnonymousSymbol(B, 0, PointerSize, false, false);
+}
+
+bool GOTTableManager::visitEdge(LinkGraph &G, Block *B, Edge &E) {
+ return false;
+}
+
+Symbol &GOTTableManager::createEntry(LinkGraph &G, Symbol &Target) {
+ // Add new block in GOT section and return an anonymous symbol pointing to it.
+ return createAnonymousPointer(G, getGOTSection(G), &Target);
+}
+
const uint8_t Thumbv7ABS[] = {
0x40, 0xf2, 0x00, 0x0c, // movw r12, #0x0000 ; lower 16-bit
0xc0, 0xf2, 0x00, 0x0c, // movt r12, #0x0000 ; upper 16-bit
@@ -626,7 +651,7 @@ const uint8_t Thumbv7ABS[] = {
};
template <>
-Symbol &StubsManager<Thumbv7>::createEntry(LinkGraph &G, Symbol &Target) {
+Symbol &PLTTableManager<Thumbv7>::createEntry(LinkGraph &G, Symbol &Target) {
constexpr uint64_t Alignment = 4;
Block &B = addStub(G, Thumbv7ABS, Alignment);
LLVM_DEBUG({
@@ -636,8 +661,9 @@ Symbol &StubsManager<Thumbv7>::createEntry(LinkGraph &G, Symbol &Target) {
checkRegister<Thumb_MovtAbs>(StubPtr + 4, Reg12) &&
"Linker generated stubs may only corrupt register r12 (IP)");
});
- B.addEdge(Thumb_MovwAbsNC, 0, Target, 0);
- B.addEdge(Thumb_MovtAbs, 4, Target, 0);
+ Symbol &GOTEntry = GOT.getEntryForTarget(G, Target);
+ B.addEdge(Thumb_MovwAbsNC, 0, GOTEntry, 0);
+ B.addEdge(Thumb_MovtAbs, 4, GOTEntry, 0);
Symbol &Stub = G.addAnonymousSymbol(B, 0, B.getSize(), true, false);
Stub.setTargetFlags(ThumbSymbol);
return Stub;
diff --git a/llvm/test/ExecutionEngine/JITLink/AArch32/ELF_thumb_stubs.s b/llvm/test/ExecutionEngine/JITLink/AArch32/ELF_thumb_stubs.s
new file mode 100644
index 000000000000000..18d06bbded2e128
--- /dev/null
+++ b/llvm/test/ExecutionEngine/JITLink/AArch32/ELF_thumb_stubs.s
@@ -0,0 +1,45 @@
+# RUN: rm -rf %t && mkdir -p %t
+# RUN: llvm-mc -triple=thumbv7-linux-gnueabi -arm-add-build-attributes \
+# RUN: -filetype=obj -filetype=obj -o %t/elf_stubs.o %s
+# RUN: llvm-jitlink -noexec -slab-address 0x76ff0000 \
+# RUN: -slab-allocate 10Kb -slab-page-size 4096 \
+# RUN: -abs external_func=0x76bbe880 \
+# RUN: -check %s %t/elf_stubs.o
+
+ .text
+ .syntax unified
+
+# Check that calls/jumps to external functions trigger the generation of stubs
+# and GOT entries. The GOT entry contains the absolute address of the external
+# function. The stub loads it and branches there.
+#
+# jitlink-check: *{4}(got_addr(elf_stubs.o, external_func)) = external_func
+# jitlink-check: decode_operand(test_external_call, 2) = stub_addr(elf_stubs.o, external_func) - next_pc(test_external_call)
+# jitlink-check: decode_operand(test_external_jump, 0) = stub_addr(elf_stubs.o, external_func) - next_pc(test_external_jump)
+ .globl test_external_call
+ .type test_external_call,%function
+ .p2align 1
+ .code 16
+ .thumb_func
+test_external_call:
+ bl external_func
+ .size test_external_call, .-test_external_call
+
+ .globl test_external_jump
+ .type test_external_call,%function
+ .p2align 1
+ .code 16
+ .thumb_func
+test_external_jump:
+ b external_func
+ .size test_external_jump, .-test_external_jump
+
+# Empty main function for jitlink to be happy
+ .globl main
+ .type main,%function
+ .p2align 1
+ .code 16
+ .thumb_func
+main:
+ bx lr
+ .size main, .-main
>From 5091d1ffec05ec65e4e762d2bea8735d0de7b6fb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Stefan=20Gr=C3=A4nitz?= <stefan.graenitz at gmail.com>
Date: Tue, 31 Oct 2023 13:12:21 +0100
Subject: [PATCH 4/4] [lldb] Fix missing comsumeError() with LLDB_LOG in
ObjectFileCOFF/PECOFF
---
lldb/source/Plugins/ObjectFile/COFF/ObjectFileCOFF.cpp | 6 +++---
.../Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp | 9 ++++-----
2 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/lldb/source/Plugins/ObjectFile/COFF/ObjectFileCOFF.cpp b/lldb/source/Plugins/ObjectFile/COFF/ObjectFileCOFF.cpp
index 03c454bf3efab14..a7ad5d27b237f12 100644
--- a/lldb/source/Plugins/ObjectFile/COFF/ObjectFileCOFF.cpp
+++ b/lldb/source/Plugins/ObjectFile/COFF/ObjectFileCOFF.cpp
@@ -271,9 +271,9 @@ void ObjectFileCOFF::ParseSymtab(lldb_private::Symtab &symtab) {
const auto COFFSymRef = m_object->getCOFFSymbol(SymRef);
Expected<StringRef> NameOrErr = SymRef.getName();
- if (auto error = NameOrErr.takeError()) {
- LLDB_LOG(log, "ObjectFileCOFF: failed to get symbol name: {0}",
- llvm::fmt_consume(std::move(error)));
+ if (!NameOrErr) {
+ LLDB_LOG_ERROR(log, NameOrErr.takeError(),
+ "ObjectFileCOFF: failed to get symbol name: {0}");
continue;
}
diff --git a/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp b/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
index 7fb10a69391c566..be0020cad5bee8e 100644
--- a/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ b/lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -791,11 +791,10 @@ void ObjectFilePECOFF::AppendFromCOFFSymbolTable(
for (const auto &sym_ref : m_binary->symbols()) {
const auto coff_sym_ref = m_binary->getCOFFSymbol(sym_ref);
auto name_or_error = sym_ref.getName();
- if (auto err = name_or_error.takeError()) {
- LLDB_LOG(log,
- "ObjectFilePECOFF::AppendFromCOFFSymbolTable - failed to get "
- "symbol table entry name: {0}",
- llvm::fmt_consume(std::move(err)));
+ if (!name_or_error) {
+ LLDB_LOG_ERROR(log, name_or_error.takeError(),
+ "ObjectFilePECOFF::AppendFromCOFFSymbolTable - failed to "
+ "get symbol table entry name: {0}");
continue;
}
const llvm::StringRef sym_name = *name_or_error;
More information about the llvm-commits
mailing list