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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

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

> First of all, thank you, Greg and Pavel, for all the great feedback and discussion. I have followed all your suggestions (use plain method, use `image lookup`, added the additional tests). I have also packaged the C test, but as Greg notes I am not convinced it will keep testing what it's supposed to.
>
> Now, let me answer the question regarding the context:
>
> In D110571#3030913 <https://reviews.llvm.org/D110571#3030913>, @clayborg wrote:
>
>> LLDB doesn't parse function definitions that have no low/high PC into lldb_private::Function with lldb_private::Block objects, it only does this for instances of functions with address ranges.
>>
>> I would expect that the SymbolContextScope (one pointer that can identify the SymbolContext) for each variable parsed by ParseVariableDIE to point to the DW_TAG_variable that is in the function with the address range. Are you saying that the symbol context points to the definition?
>
> With this change, the symbol context for unused parameters points to one of the concrete inlined function block (DW_TAG_inlined_subroutine). That concrete inlined function will not contain that formal parameter because after the clang change https://reviews.llvm.org/D95617, unused formal parameters are deleted from their concrete inlined functions (i.e., from their DW_TAG_inlined_subroutine).
>
> The point here is that if a function is inlined into two places, i.e., there are two corresponding DW_TAG_inlined_subroutines for the inlined function, we still create just one instance of Variable and its symbol context will be one randomly chosen DW_TAG_inlined_subroutine.
>
> As Pavel suggested, an alternative would be creating one Variable instance per DW_TAG_inlined_subroutine. That would require some changes to other data structures because the existing code assumes there is just one Variable for each DIE (see SymbolFileDWARF::GetDIEToVariable).
>
> For illustration:
>
>   0x100  DW_TAG_subprogram
>            DW_AT_name "inlined_function"
>            ... no DW_AT_low_pc here ...
>   0x110    DW_TAG_formal_parameter
>              DW_AT_name "unused"
>              ...
>          ...
>   0x200  DW_TAG_subprogram
>            DW_AT_name    ("top_level_function_with_address"
>            DW_AT_low_pc  (0x3000)
>            DW_AT_high_pc  (0x3100)
>            ...
>   0x210    DW_TAG_inlined_subroutine
>              DW_AT_abstract_origin (0x100 "inlined_function")
>              DW_AT_low_pc  (0x3010)
>              DW_AT_high_pc  (0x3020)
>              # Note the missing DW_TAG_formal_parameter here!
>            NULL
>          ...
>   0x400  DW_TAG_subprogram
>            DW_AT_name    ("another_top_level_function_with_address"
>            DW_AT_low_pc  (0x5000)
>            DW_AT_high_pc  (0x5100)
>            ...
>   0x410    DW_TAG_inlined_subroutine
>              DW_AT_abstract_origin (0x100 "inlined_function")
>              DW_AT_low_pc  (0x5030)
>              DW_AT_high_pc  (0x5040)
>              # Note the missing DW_TAG_formal_parameter here!
>            NULL
>          ...
>
> Here, we will create just one variable for the formal parameter "unused" (DIE offset 0x110). That variable's symbol context will be randomly one of the DW_TAG_inline subroutine blocks (either 0x210 or 0x410), and the variable will be inserted into two variable lists, one for the Block associated with the DIE at 0x210 and one for DIE associated with 0x410.

I hear what you are saying, but I am not sure this will be happening. Let me explain: for each concrete DW_TAG_subprogram (0x200 and 0x400 in your example above), we create a unique lldb_private::Function object whose UserID will be 0x200 for "top_level_function_with_address" and 0x400 for "another_top_level_function_with_address". Each of those functions might be asked for their lldb_private::Block objects at some point and we should create unique lldb_private::Block for each DW_TAG_lexical_block and DW_TAG_inlined_subroutine that is contained within these unique DIEs. Each of these should now have a variable within the block that is a parameter whose name is "unused" and whose symbol context should be 0x210 for the 0x200 DIE, and 0x410 for the 0x400 DIE. So it would be great to make sure this happens correctly. >From looking at the code, it seems like this should be happening correctly, but you might know better since you made these new modifications.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110571