[PATCH] D143793: Add the ability to segment GSYM files.

[Greg Clayton via Phabricator via llvm-commits]

clayborg marked 6 inline comments as done.
clayborg added inline comments.


================
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();
----------------
yinghuitan wrote:
> Is the lock only needed during accessing `Funcs` field? Maybe scope locking it instead of locking the entire function.
This is correctly scoped right at the end of the function to only lock during Funcs modification and using the Funcs.back().


================
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;
----------------
yinghuitan wrote:
> Shouldn't this be an error with meaning message instead of simply break and return success? 
It isn't an error, createSegment returns NULL if there are no more functions which is ok.


================
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;
----------------
clayborg wrote:
> yinghuitan wrote:
> > Shouldn't this be an error with meaning message instead of simply break and return success? 
> It isn't an error, createSegment returns NULL if there are no more functions which is ok.
I now return an expected to catch the error case where segment size is too small to contain a single function info.


================
Comment at: llvm/lib/DebugInfo/GSYM/GsymCreator.cpp:555
+  for (; FuncIdx < NumFuncs; ++FuncIdx) {
+    const uint64_t HeaderAndTableSize = GC->calculateHeaderAndTableSize();
+    if (HeaderAndTableSize + SegmentFuncInfosSize >= SegmentSize)
----------------
yinghuitan wrote:
> This is not changing per function, right? If so move outside of loop.
It is changing per function, since each function info can add more strings and files to the GSYM file. calculateHeaderAndTableSize is fast to calculate. So this needs to stay here.


================
Comment at: llvm/lib/DebugInfo/GSYM/GsymCreator.cpp:556
+    const uint64_t HeaderAndTableSize = GC->calculateHeaderAndTableSize();
+    if (HeaderAndTableSize + SegmentFuncInfosSize >= SegmentSize)
+      break;
----------------
yinghuitan wrote:
> We should also detect outside loop at least `SegmentSize > SegmentFuncInfosSize` so that we are not creating dummy segment file with only header. 
I will move this if statement after the copyFunctionInfo to ensure we always create a GSYM with at least one entry.


================
Comment at: llvm/lib/DebugInfo/GSYM/GsymCreator.cpp:556
+    const uint64_t HeaderAndTableSize = GC->calculateHeaderAndTableSize();
+    if (HeaderAndTableSize + SegmentFuncInfosSize >= SegmentSize)
+      break;
----------------
clayborg wrote:
> yinghuitan wrote:
> > We should also detect outside loop at least `SegmentSize > SegmentFuncInfosSize` so that we are not creating dummy segment file with only header. 
> I will move this if statement after the copyFunctionInfo to ensure we always create a GSYM with at least one entry.
I modified the code to detect when no function infos have been emitted and return an error when I detect that. Also added a test.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143793/new/

https://reviews.llvm.org/D143793