[Lldb-commits] [PATCH] D110571: [lldb] Add omitted abstract formal parameters in DWARF symbol files

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Just a couple of questions inline.

In D110571#3035814 <https://reviews.llvm.org/D110571#3035814>, @jarin wrote:

> - thread #1, name = 'a.out', stop reason = breakpoint 1.2 frame #0: 0x0000000000401151 a.out`main [inlined] f(used=4, unused=<unavailable>) at a.cc:5:3
>
> (lldb) frame var
> (int) used = 4
> (int) local = 5      <--- HERE, a local variables got between the parameters because we append unused parameters at the end.
> (int) unused = <no location, value may have been optimized out>
>
>   Let me try to rewrite the code so that the trailing unused parameters are inserted after the last concrete parameter (or at the beginning of variable list if there are no concrete parameters). Let me know if you think it is unnecessary.

TBH, I have no idea. The function description comes out right, so it seems at least some parts of lldb are prepared to handle this. I suppose it would be nicer if they were grouped together, but if it makes the code significantly more complex, then I probably wouldn't bother.



================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3561-3562
+    const lldb::addr_t func_low_pc, VariableList &variable_list,
+    llvm::SmallVectorImpl<DWARFDIE> &abstract_formal_parameters,
+    size_t &abstract_parameter_index) {
   size_t vars_added = 0;
----------------
I'm wondering if one could pass a single `ArrayRef<DWARFDIE> &` argument instead of the array+index pair. FWICS, you're processing the list in a left-to-right fashion, which seems ideal for `ArrayRef::drop_front`. WDYT?


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3575-3576
+      bool found = false;
+      while (parameter) {
+        parameter = parameter.GetReferencedDIE(DW_AT_abstract_origin);
+        if (parameter == abstract_parameter) {
----------------
All of these abstract origin loops make me uneasy. They make it very easy to hang (whether deliberately or not) the debugger with a bit of incorrect dwarf (self-referencing DIEs). Do we actually know of any abstract_origin chains?
It's not really clear to me what would be the right interpretation of that, so I can't even say whether this algorithm would be correct there.

Maybe just stick to a single "dereference" ?


================
Comment at: lldb/test/API/functionalities/unused-inlined-parameters/TestUnusedInlinedParameters.py:17
+
+        # For the unused parameters, only check the types.
+        self.assertEqual("void *", self.frame().FindVariable("unused1").GetType().GetName())
----------------
Maybe we could check something else as well... Do `GetDescription` or `str(value)` return something reasonable here?


================
Comment at: lldb/test/Shell/SymbolFile/DWARF/x86/unused-inlined-params.test:1
+# UNSUPPORTED: system-darwin, system-windows
+
----------------
You should be able to drop this now.


================
Comment at: lldb/test/Shell/SymbolFile/DWARF/x86/unused-inlined-params.test:33
+# Note: image lookup does not show variables outside of their
+#       location, so |partial| is missing here.
+# CHECK: name = "unused3", type = "int", location = <empty>
----------------
You could add `CHECK-NOT: partial` here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110571