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

[Djordje Todorovic via Phabricator via llvm-commits]

djtodoro added inline comments.


================
Comment at: llvm/tools/llvm-dwarfdump/Statistics.cpp:456-458
+        // This means that the DW_AT_inline fn copy is out of order,
+        // so this inlined instance will be processed later.
+        InlinedFnsToBeProcessed.push_back(Die.getOffset());
----------------
jmorse wrote:
> Should this be setting InlinedVarsPtr to nullptr? Inlined variables from a higher-up-the-stack inlining site might still be present and pointed to by the InlinedVarsPtr argument.
it makes sense


================
Comment at: llvm/tools/llvm-dwarfdump/Statistics.cpp:490-500
+      // Skip over abstract origins, but collect variables
+      // from it so it can be used for location statistics
+      // for inlined instancies.
+      if (Die.find(dwarf::DW_AT_inline)) {
+        DWARFDie Child = Die.getFirstChild();
+        while (Child) {
+          const dwarf::Tag ChildTag = Child.getTag();
----------------
This piece of code needs some changes in order to handle lexical scopes... 


================
Comment at: llvm/tools/llvm-dwarfdump/Statistics.cpp:653-655
+          InlinedVars.erase(std::remove(InlinedVars.begin(), InlinedVars.end(),
+                                        (*OffsetVar).getRawUValue()),
+                            InlinedVars.end());
----------------
dblaikie wrote:
> jmorse wrote:
> > Isn't this "removing" twice, once with std::remove, the other with the erase method?
> Ah the joys of C++. This code is correct and an application of the classic "erase remove" idiom ( https://en.wikipedia.org/wiki/Erase%E2%80%93remove_idiom ).
> 
> Though we do have an llmv wrapper that tidies this up a bit, llvm::erase_if and llvm::erase_value
Yeah, it is an idiom for eliminating elements from stl containers.

@dblaikie I think that using of the `std::` ones is recommended approach, right? Or do you proposing using of the `llvm::` ones?


================
Comment at: llvm/tools/llvm-dwarfdump/Statistics.cpp:658
+
+      Child = Child.getSibling();
+    }
----------------
jmorse wrote:
> Hmmmmm, this isn't going to collect variables that are nested within lexical_block's I guess, and fixing that probably requires more recursion. Ouch.
O yeah... inlined functions can also have lexical scopes... I will add dedicated test case and handle it.


================
Comment at: llvm/tools/llvm-dwarfdump/Statistics.cpp:713
   /// version.
   unsigned Version = 6;
   unsigned VarParamTotal = 0;
----------------
jmorse wrote:
> I reckon we'll need to bump the version number for this.
I thought that only in the case of new metrics we should update this...
Thanks! I will update it.


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

https://reviews.llvm.org/D96045