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

Thu Sep 30 07:38:42 PDT 2021

labath added a comment.

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

> In D110571#3033078 <https://reviews.llvm.org/D110571#3033078>, @labath wrote:
>
>> Here's one more question. AIUI, lldb relies on the order of formal parameter declarations in dwarf to establish the the function signature (dwarf doesn't leave us much choice. This then affects how the function is printed in the backtrace, for instance. What will be the resulting order of arguments for these functions? I'm wondering if we don't need a two-pass algorithm, which first parses the arguments in the function declaration (to establish their order), and then do another pass over the concrete instance to fill in the missing information. (I'm sorry if you're doing this already, but I'm still too scared of the code to figure it out myself :P ).
>
> The code already does the merging. For DW_TAG_inlined_subroutine, it first collects the formal_parameter list from from its abstract_origin and then it will parse/insert all the missing one while parsing the concrete instance. This will preserve the order of formal parameters. Now that I think about this, it might add some formal parameters after local variables, but I hope this is not a real problem for LLDB. If this is a problem, we could perhaps flush the abstract formal parameters whenever we encounter DW_TAG_variable.

Cool. I see you're way ahead of me. If you're not careful you may end up as the dwarf maintainer. :P

>> In D110571#3032599 <https://reviews.llvm.org/D110571#3032599>, @jarin wrote:
>>
>>> From what you say, this is not the desired behavior? If we wanted two instances of the variable (one for each block), we could change the DIE-to-variable cache <https://github.com/llvm/llvm-project/blob/47d66355ef9039a15b7265945e3deb331d7f9e05/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h#L324> to be indexed by a pair <symbol-context-DIE, variable-DIE>.
>>
>> Given that was Greg's (and yours, kinda) reaction as well. I guess we should do something like that. Even if it does not cause problems now, it could certainly cause them in the future, if something starts relying on the symbol_context_scope link making sense.
>>
>> I am wondering about the best way to implement in though. Having a pair as a key seems very redundant to me. As we already know the block its going to end up in maybe we could somehow check if its already present there? Since blocks/functions don't generally have that many variables [citation needed], maybe even a simple iteration would suffice? (The situation is probably different for global variables, but those don't need the extra key.)
>
> Are you worried about code redundancy or memory redundancy?

A little bit of both, but I would mostly say its because "it doesn't feel right". However,

> I do not think a pair would be much extra code. If you are worried about memory, we could also have a separate map for the abstract parameters - we always know whether we are inserting an abstract parameter or a concrete one. (I did not quite understand the idea with block lookup/iteration.)
>
> An interesting question is whether the caching is needed at all in the context of functions - even without the cache, we should not parse block variables multiple times because the variables are already cached in their block's variable list. I actually verified that the cache never hits for function scoped variables on the LLDB test suite (with and without this patch). It does hit for global variables, but they take a different path now. So how would you feel about bypassing the cache when parsing in the function context? (I would basically move the caching code from SymbolFileDWARF::ParseVariableDIE to SymbolFileDWARF::ParseAndAppendGlobalVariable.)

this sounds like an excellent idea. Make the code correct by deleting it, and saving some memory in the process. :)

================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h:396
+      const lldb::addr_t func_low_pc, lldb_private::VariableList &variable_list,
+      DIEVector &abstract_formal_parameters, size_t &abstract_parameter_index);

----------------
The correct thing to do (despite the weird-looking name) is for the function to take a `SmallVectorImpl<DWARFDIE> &` argument.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110571