[Lldb-commits] [PATCH] D158785: [lldb] Add a "thread extrainfo" LC_NOTE for Mach-O corefiles, to store the thread IDs of the threads
Jonas Devlieghere via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
Fri Aug 25 14:51:27 PDT 2023
JDevlieghere added inline comments.
================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:5682-5693
+ for (uint32_t i = 0; i < m_header.ncmds; ++i) {
+ const uint32_t cmd_offset = offset;
+ llvm::MachO::load_command lc = {};
+ if (m_data.GetU32(&offset, &lc.cmd, 2) == nullptr)
+ break;
+ if (lc.cmd == LC_NOTE) {
+ char data_owner[17];
----------------
There's a *lot* of duplication between reading `LC_NOTE`s. At some point we should extract the common code instead of copy-pasting it every time.
================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:5723-5724
+ }
+ const uint32_t num_threads = threads->GetSize();
+ for (uint32_t i = 0; i < num_threads; i++) {
+ StructuredData::Dictionary *thread;
----------------
`StructuredData::Array::GetSize` returns a `size_t`. No point in turning this into a `uint32_t`.
================
Comment at: lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp:603
+
+ // populate used_tids
+ for (uint32_t i = 0; i < num_threads; i++) {
----------------
Capitalize + period.
================
Comment at: lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp:608-619
+ // If any threads have an unspecified thread id,
+ // find an unused number, use it instead.
+ tid_t current_unused_tid = 0;
+ for (uint32_t i = 0; i < num_threads; i++) {
+ if (tids[i] == LLDB_INVALID_THREAD_ID) {
+ while (used_tids.find(current_unused_tid) != used_tids.end()) {
+ current_unused_tid++;
----------------
This code is (presumably on purpose) trying to find gaps in the available tids: for example if I had `[1, 2, ? ,4, ?, 6]` this is going to turn that into `[1, 2, (3), 4, (5), 6]`. If that property matters, it probably should be part of the comment. If it doesn't, you could exploit the fact that the `set` is sorted and just take the last value (6 for the previous example) and return `[1, 2, 4, 6, 7, 8]`, but again, I assume that's what you're trying to avoid here.
================
Comment at: lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp:628-629
+ for (uint32_t i = 0; i < num_threads; i++) {
+ ThreadSP thread_sp(new ThreadMachCore(*this, tids[i], i));
new_thread_list.AddThread(thread_sp);
}
----------------
`make_shared` (avoid "naked new")
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D158785/new/
https://reviews.llvm.org/D158785
More information about the lldb-commits
mailing list