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

Tue Sep 28 05:03:41 PDT 2021

labath added a comment.

I haven't looked at the actual code yet, so I could be off, but here are some thoughts.

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

> Hi, could you take a look at this change?
>
> Some discussion points:
>
> - In the ParseVariablesInFunctionContext method, we are using a lambda for the recursive parser. We could also use a function-local class or inner class of SymbolFileDWARF. Would any of these be preferable?

Yeah, what's the deal with that? Why wouldn't a regular function be sufficient? You can just pass things in arguments instead of closures or classes..

> - The variables created by ParseVariableDIE on abstract formal parameters are fairly strange, especially if a function gets inlined into two different functions. If that happens, then the parsed variable will refer to a symbol context that does not contain the variable DIE and a block can contain a variable that is not in the DIE of tree of the block. Is that a big problem? (Quick testing of this situation did not reveal any strange stack traces or `frame var` anomalies.) Unfortunately, there is no good way to provide the correct block and the correct function because LLDB does not parse functions and blocks for the abstract functions (i.e., for the DW_TAG_subroutines that are referenced by DW_AT_abstract_origin of concrete functions).

Judging by your description, I take it you parse these variables only once, regardless of how many functions they are inlined in. Could we fix that my creating a fresh variable object for each inlined instance? Then it could maybe be correctly made to point to the actual block and function it is inlined into(?)

> - The provided test only tests the case of an inlined function where some parameters are unused/omitted. Would it make sense to also provide tests for other interesting cases or would that be too much bloat? The particularly interesting cases are:
>   - Inlined function with all its parameters unused/omitted,
>   - Inlined function that is called from different top-level functions.
>   - Test correctness of the stack trace in the cases above.
> - We could supply a test written in C, but it needs -O1 and is fairly sensitive to the meaning of -O1 (e.g., clang started inlining and omitting unsued inlined parameters only recently, so changes to -O1 could make a C test easily meaningless). Any concerns here?
> - The provided test is a bit verbose, mostly because we wanted to mostly preserve the structure of the C compiler output. We could still cut the size of the test down by removing the main function in favour of _start and by removing all the file/line info. Would any of that make sense?

I think you could get quite far by just testing the output of the "image lookup" command. That should give you list variables that are in scope for any particular address, and a bunch of details about each var, including the expression used to compute its value (not the value itself, obviously). The main advantage is that you wouldn't need a fully functional program, as you wouldn't be running anything. That would remove a lot of bloat, and also allow the test to run on non-x86-pc-linux hosts. Then, maybe it wouldn't be too messy to add the additional test cases you mention.

You can look at (e.g.) DW_AT_loclists_base.s for an example of a test case with image lookup and local variables.

After that, we could think about adding a c++ test case. Although tests with optimized code are tricky, it is often possible (with judicious use of noinline, always_inline and optnone attributes) to constrain the optimizer in a way that it has no choice but to do exactly what we want.

================
Comment at: lldb/test/Shell/SymbolFile/DWARF/x86/unused-inlined-params.test:32-34
+# CHECK: (int) unused2 = <
+# CHECK: (int) partial = <
+# CHECK: (int) unused3 = <
----------------
Including the actual message would make it clearer what is going on.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110571