[PATCH] D96045: [llvm-dwarfdump][locstats] Unify handling of inlined vars with no loc

[Djordje Todorovic via Phabricator via llvm-commits]

djtodoro added a comment.

@jmorse Thanks for the review!

In D96045#2544740 <https://reviews.llvm.org/D96045#2544740>, @jmorse wrote:

> Thinking out loud, and motivated by memory pressure: for the per-inlining-site record, that doesn't need be be maintained before or after we visit the inlining site, right? We might be able to get away with keeping a "Current scopes inlined variables" vector on the stack and passing it down through collectStatsRecursive. That'll automagically release the vector when we're done with the scope.
>
> llvm-dwarfdump --statistics already takes a while to examine a stage2reldeb clang binary, it'd be best to avoid allocating more global data than we need.

It makes sense. I tried that approach, but I had encountered an issue, and suddenly just dropped that way of the implementation. I'll try to implement this that way now, since have a prototype working.



================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/locstats-for-inlined-vars.yaml:36-37
+##       DW_AT_call_column     (0x01)
+##       DW_TAG_formal_parameter
+##         DW_AT_abstract_origin       (0x00000018)
+##       DW_TAG_variable
----------------
jmorse wrote:
> Could I request a third inlining site with no DIE at all for this variable -- that'll ensure we test the behaviour that D94976 is trying to create.
Sure. I'll add it in the next revision.


================
Comment at: llvm/tools/llvm-dwarfdump/Statistics.cpp:439
   const bool IsInlinedFunction = Tag == dwarf::DW_TAG_inlined_subroutine;
+  std::string FnID;
   if (IsFunction || IsInlinedFunction || IsBlock) {
----------------
jmorse wrote:
> This doesn't need to be in the outer scope, does it? IMO better to keep the declaration where it was, as it's only used in that block.
It is being used at the end of the function again.


================
Comment at: llvm/tools/llvm-dwarfdump/Statistics.cpp:556-558
+    InlineSiteVars.erase(
+        std::remove(InlineSiteVars.begin(), InlineSiteVars.end(), Offset),
+        InlineSiteVars.end());
----------------
jmorse wrote:
> I get the feeling modifying this vector while iterating over it is vulnerable to re-ordering / re-allocating.
Right... It will be removed completely if we keep the vector on the stack.


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

https://reviews.llvm.org/D96045