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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
