jarin added a comment. Just a few replies below; I am hoping to put the relevant code changes together tomorrow.
In D110571#3027173 <https://reviews.llvm.org/D110571#3027173>, @labath wrote: > 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.. Right, I worked on a codebase where they used local classes for such things and in lldb I have seen lambdas. I actually do not have a strong preference, will rewrite this to use plain methods. >> - 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(?) Yes, they are parsed only once. This is because there is a DIE->Variable map (see SymbolFileDWARF::GetDIEToVariable) that makes sure no DIE gets parsed twice. Are you suggesting to index the map with a pair <concrete-function-die, variable-die>? That would indeed create healthier structure (even though I could not spot any problems even with my current somewhat flawed approach). >> - 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. Makes sense, I really like that approach. Let me try to get that working. > 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. I have actually use the attributes when experimenting with the patch - if you think this is useful, I can certainly provide those tests. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110571/new/ https://reviews.llvm.org/D110571 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
