[PATCH] D143793: Add the ability to segment GSYM files.
jeffrey tan via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Feb 14 10:49:07 PST 2023
yinghuitan added inline comments.
================
Comment at: llvm/lib/DebugInfo/GSYM/GsymCreator.cpp:454
+ // Add size of address info offsets
+ size += NumFuncs * 4;
+ // Add file table size
----------------
Instead of magic value 4, maybe `sizeof(<type-of-offset-entry>)` in case it is changed in future?
================
Comment at: llvm/lib/DebugInfo/GSYM/GsymCreator.cpp:503-505
+ std::lock_guard<std::mutex> Guard(Mutex);
+ Funcs.push_back(DstFI);
+ return Funcs.back().cacheEncoding();
----------------
Is the lock only needed during accessing `Funcs` field? Maybe scope locking it instead of locking the entire function.
================
Comment at: llvm/lib/DebugInfo/GSYM/GsymCreator.cpp:517-518
+ std::unique_ptr<GsymCreator> GC = createSegment(SegmentSize, FuncIdx);
+ if (!GC)
+ break;
+ raw_null_ostream ErrorStrm;
----------------
Shouldn't this be an error with meaning message instead of simply break and return success?
================
Comment at: llvm/lib/DebugInfo/GSYM/GsymCreator.cpp:555
+ for (; FuncIdx < NumFuncs; ++FuncIdx) {
+ const uint64_t HeaderAndTableSize = GC->calculateHeaderAndTableSize();
+ if (HeaderAndTableSize + SegmentFuncInfosSize >= SegmentSize)
----------------
This is not changing per function, right? If so move outside of loop.
================
Comment at: llvm/lib/DebugInfo/GSYM/GsymCreator.cpp:556
+ const uint64_t HeaderAndTableSize = GC->calculateHeaderAndTableSize();
+ if (HeaderAndTableSize + SegmentFuncInfosSize >= SegmentSize)
+ break;
----------------
We should also detect outside loop at least `SegmentSize > SegmentFuncInfosSize` so that we are not creating dummy segment file with only header.
================
Comment at: llvm/tools/llvm-gsymutil/llvm-gsymutil.cpp:322
+
+ if (auto Err = Gsym.save(OutFile, Endian, SegmentSize > 0 ? std::optional(SegmentSize.getValue()) : std::nullopt))
return Err;
----------------
The line is very long, maybe run clang-format against it.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143793/new/
https://reviews.llvm.org/D143793
More information about the llvm-commits
mailing list