[PATCH] D159226: Emit line numbers in CodeView for trailing (after `ret`) blocks from inlined functions

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

Thanks for the patch, this is gnarly code. At the time I was probably thinking that flat vectors are good for performance, and that we'll have tons of line table entries, but as the inline location handling evolved, it's gotten quite complicated.



================
Comment at: llvm/lib/MC/MCCodeView.cpp:278
+  MCCVFunctionInfo *SiteInfo = getCVFunctionInfo(LineEntry.getFunctionId());
+  if (SiteInfo->isInlinedCallSite()) {
+    auto I = MCCVLineStartStop.insert(
----------------
This seems like it only handles one level of inlining. If this cv_loc is for an inline call site two levels deep, you aren't going to update the extent of the parent non-inline function to include this cv_loc entry.

I think this code in the inline line table emitter is meant to expand the range of cv_loc entries that we consider for inline line tables:
https://github.com/llvm/llvm-project/blob/main/llvm/lib/MC/MCCodeView.cpp#L468

Could we solve the problem by adding similar logic to `CodeViewContext::getFunctionLineEntries` before it's main loop?


================
Comment at: llvm/lib/MC/MCCodeView.cpp:290-292
   std::vector<MCCVLoc> FilteredLines;
   auto I = MCCVLineStartStop.find(FuncId);
   if (I != MCCVLineStartStop.end()) {
----------------
It would be LLVM-style to early return FilteredLines on map lookup failure and reduce nesting.

I also think this code is reimplementing getLineExtent, we could just call that, and then use names like Lo/Hi or L/R for the extent instead of `I->second.first/second`.

Then, I propose you increase the line extent using the same code from encodeinlinelinetable:
  // Include all child inline call sites in our .cv_loc extent.
  MCCVFunctionInfo *SiteInfo = getCVFunctionInfo(Frag.SiteFuncId);
  for (auto &KV : SiteInfo->InlinedAtMap) {
    unsigned ChildId = KV.first;
    auto Extent = getLineExtent(ChildId);
    LocBegin = std::min(LocBegin, Extent.first);
    LocEnd = std::max(LocEnd, Extent.second);
  }




================
Comment at: llvm/test/DebugInfo/COFF/trailing-inlined-function.ll:1
+; RUN: llc < %s -filetype=obj | llvm-readobj - --codeview --codeview-subsection-bytes | FileCheck %s
+
----------------
Can you make this a .s test? The fix is in the way MC handles .cv_loc directives, so it should reduce the scope of the test.

Does rustc have an equivalent of Clang -g1 / -gmlt, to only produce source locations, no variable locations? That has the potential to reduce variable debug info and make the test smaller.

It still makes sense to me to use llvm-readobj, since that keeps LLD and llvm-symbolizer out of the test.


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

https://reviews.llvm.org/D159226