github-actions[bot] commented on code in PR #64093:
URL: https://github.com/apache/doris/pull/64093#discussion_r3526264255
##########
be/src/common/phdr_cache.cpp:
##########
@@ -69,74 +90,226 @@ DLIterateFunction getOriginalDLIteratePHDR() {
return reinterpret_cast<DLIterateFunction>(func);
}
-using PHDRCache = std::vector<dl_phdr_info>;
+struct RawPHDRCacheEntry {
+ dl_phdr_info info {};
+ std::array<ElfW(Phdr), MAX_PHDR_CACHE_PROGRAM_HEADERS> phdrs {};
+ size_t phdr_count = 0;
+ std::array<char, MAX_PHDR_CACHE_OBJECT_NAME> name {};
+ uintptr_t address_begin = std::numeric_limits<uintptr_t>::max();
+ uintptr_t address_end = 0;
+};
+
+struct RawPHDRCacheSnapshot {
+ std::array<RawPHDRCacheEntry, MAX_PHDR_CACHE_LOADED_OBJECTS> entries {};
+ size_t size = 0;
+ bool overflow = false;
+ bool phdr_truncated = false;
+ bool name_truncated = false;
+};
+
+struct PHDRCacheEntry {
+ dl_phdr_info info {};
+ std::vector<ElfW(Phdr)> phdrs;
+ std::string name;
+ uintptr_t address_begin = std::numeric_limits<uintptr_t>::max();
+ uintptr_t address_end = 0;
+
+ bool contains(uintptr_t ip) const {
+ return ip == 0 || (address_begin <= ip && ip < address_end);
+ }
+};
+
+using PHDRCache = std::vector<PHDRCacheEntry>;
std::atomic<PHDRCache*> phdr_cache {};
+// This flag is flipped inside the stack-trace signal handler. Force a static
TLS access model so
+// reading it from our dl_iterate_phdr interposer does not call into the
dynamic loader's TLS path.
+__thread bool use_phdr_cache __attribute__((tls_model("initial-exec"))) =
false;
-} // namespace
+uintptr_t saturated_segment_end(uintptr_t begin, uintptr_t size) {
+ const uintptr_t max_address = std::numeric_limits<uintptr_t>::max();
+ return begin > max_address - size ? max_address : begin + size;
+}
-// Jemalloc heap profile follows libgcc's way of backtracing by default.
-// rewrites dl_iterate_phdr will cause Jemalloc to fail to run after enable
profile.
+void copy_object_name(const char* source, RawPHDRCacheEntry* entry,
+ RawPHDRCacheSnapshot* snapshot) {
+ if (source == nullptr) {
+ entry->name[0] = '\0';
+ return;
+ }
-// TODO, two solutions:
-// 1. Jemalloc specifies GNU libunwind as the prof backtracing way, but my
test failed,
-// `--enable-prof-libunwind` not work:
https://github.com/jemalloc/jemalloc/issues/2504
-// 2. ClickHouse/libunwind solves Jemalloc profile backtracing, but the branch
of ClickHouse/libunwind
-// has been out of touch with GNU libunwind and LLVM libunwind, which will
leave the fate to others.
-/*
-extern "C"
-#ifndef __clang__
- [[gnu::visibility("default")]] [[gnu::externally_visible]]
-#endif
- int
- dl_iterate_phdr(int (*callback)(dl_phdr_info* info, size_t size, void*
data), void* data) {
- auto* current_phdr_cache = phdr_cache.load();
- if (!current_phdr_cache) {
- // Cache is not yet populated, pass through to the original function.
- return getOriginalDLIteratePHDR()(callback, data);
+ size_t length = 0;
+ while (length + 1 < entry->name.size() && source[length] != '\0') {
+ entry->name[length] = source[length];
+ ++length;
+ }
+ entry->name[length] = '\0';
+ if (source[length] != '\0') {
+ snapshot->name_truncated = true;
+ }
+}
+
+int collectPHDRCacheEntry(dl_phdr_info* info, size_t /*size*/, void* data) {
+ auto* snapshot = reinterpret_cast<RawPHDRCacheSnapshot*>(data);
+ if (snapshot->size >= snapshot->entries.size()) {
+ snapshot->overflow = true;
+ return 0;
+ }
+
+ auto& entry = snapshot->entries[snapshot->size++];
+ entry.info = *info;
+ copy_object_name(info->dlpi_name, &entry, snapshot);
+
+ const size_t phdr_count = std::min<size_t>(info->dlpi_phnum,
entry.phdrs.size());
+ if (phdr_count < info->dlpi_phnum) {
+ snapshot->phdr_truncated = true;
+ }
+ entry.phdr_count = phdr_count;
+ entry.info.dlpi_phnum = static_cast<ElfW(Half)>(phdr_count);
+ entry.info.dlpi_name = nullptr;
+ entry.info.dlpi_phdr = nullptr;
+
+ if (info->dlpi_phdr == nullptr) {
+ return 0;
+ }
+ std::memcpy(entry.phdrs.data(), info->dlpi_phdr, phdr_count *
sizeof(ElfW(Phdr)));
+
+ for (size_t i = 0; i < phdr_count; ++i) {
+ const auto& phdr = entry.phdrs[i];
+ if (phdr.p_type != PT_LOAD || phdr.p_memsz == 0) {
+ continue;
+ }
+ const auto begin = static_cast<uintptr_t>(info->dlpi_addr +
phdr.p_vaddr);
+ const auto end = saturated_segment_end(begin,
static_cast<uintptr_t>(phdr.p_memsz));
+ entry.address_begin = std::min(entry.address_begin, begin);
+ entry.address_end = std::max(entry.address_end, end);
+ }
+ return 0;
+}
+
+PHDRCache* buildPHDRCache(const RawPHDRCacheSnapshot& snapshot) {
+ auto* cache = new PHDRCache;
+ cache->reserve(snapshot.size);
+ for (size_t i = 0; i < snapshot.size; ++i) {
+ const auto& raw_entry = snapshot.entries[i];
+ PHDRCacheEntry entry;
+ entry.info = raw_entry.info;
+ entry.phdrs.assign(raw_entry.phdrs.begin(), raw_entry.phdrs.begin() +
raw_entry.phdr_count);
+ entry.name = raw_entry.name.data();
+ entry.address_begin = raw_entry.address_begin;
+ entry.address_end = raw_entry.address_end;
+ cache->emplace_back(std::move(entry));
+ }
+ for (auto& entry : *cache) {
+ entry.info.dlpi_phdr = entry.phdrs.data();
+ entry.info.dlpi_name = entry.name.c_str();
+ }
+ return cache;
+}
+
+int iteratePHDRCache(int (*callback)(dl_phdr_info* info, size_t size, void*
data), void* data,
+ uintptr_t ip) {
+ auto* current_phdr_cache = phdr_cache.load(std::memory_order_acquire);
+ if (current_phdr_cache == nullptr) {
+ return 0;
Review Comment:
With the thirdparty patch, GNU libunwind calls `doris_unwind_iterate_phdr()`
whenever the weak hook is linked. Before the first `updatePHDRCache()`
publishes a snapshot, this branch returns `0`, so libunwind sees an empty PHDR
walk instead of the live loader list it used before. In `main()` the first
refresh is much later, after config parsing, storage-path checks, curl, and
optional JNI/Python initialization; those paths can still build
`Status`/`Exception` stack traces. That makes early stack captures
empty/truncated, and can feed fewer than the three frames that
`StackTrace::toString(int)` unconditionally subtracts. Please fall back to
`getOriginalDLIteratePHDR()` when the cache is null, or initialize the snapshot
before any stack-trace-producing code can run.
##########
be/test/service/http/be_thread_stack_action_test.cpp:
##########
@@ -249,11 +254,10 @@ TEST_F(BeThreadStackActionTest,
ThreadIdSelectorSupportsSingleAndMultipleIds) {
EXPECT_THAT(body, testing::HasSubstr("service_signal: "));
EXPECT_THAT(body, testing::HasSubstr("thread_count: 2\n"));
EXPECT_THAT(body, testing::HasSubstr("dwarf_location_info_mode:
disabled\n"));
- EXPECT_THAT(body,
- testing::HasSubstr(
- "signal_handler_unwinder: "
-
"frame_pointer_with_coordinator_signal_context_libunwind_fallback\n"));
- EXPECT_THAT(body, testing::HasSubstr("capture_method="));
+ EXPECT_THAT(body, testing::HasSubstr("signal_handler_unwinder:
signal_context_libunwind\n"));
Review Comment:
These tests still run on every Linux build, but the implementation now only
supports the asserted success path on Linux x86_64. On Linux aarch64,
`prepare_signal_capture()` skips the `__x86_64__` branch and returns
`"signal-context libunwind is unsupported on this architecture"`, so `handle()`
sends HTTP 500 before the new `signal_context_libunwind`/`phdr_cache`
assertions can pass. Please either keep/restore an aarch64-capable path, or
guard these success tests with the same architecture condition and add coverage
for the unsupported response.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
