[lldb] Add omitted abstract formal parameters in DWARF symbol files

[jarin]

Summary

This change fixes a problem introduced by clang change
described by https://reviews.llvm.org/D95617 and described by
https://bugs.llvm.org/show_bug.cgi?id=50076#c6, where inlined
functions omit the unused parameters both in the stack trace
and in frame var command. With this change, the parameters
are listed correctly in the stack trace and in frame var
command (the included test tests frame var).

This change adds parsing of formal parameters from the abstract
version of inlined functions and use those formal parameters if
they are missing from the concrete version.

[jarin]

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?
• 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).
• 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?

[labath]

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

In 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.

lldb/test/Shell/SymbolFile/DWARF/x86/unused-inlined-params.test

32–34

Including the actual message would make it clearer what is going on.

[labath]

(It would also make it easier to understand the code if you could paste some dwarfdump output which illustrates the kind of debug info we're dealing with.)

[jarin]

Just a few replies below; I am hoping to put the relevant code changes together tomorrow.

In 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, @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.

[labath]

In D110571#3027283, @jarin wrote:

In D110571#3027173, @labath wrote:

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).

I am not really suggesting anything (at least not yet). I'm just trying to map out the problem space. Having separate variables could be nice, but it could also be a needless complication. I've added some people to see what they make of this.

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.

If you have something ready, feel free to include it and we can see what to do then. If you don't have them, maybe wait and see how the other approach pans out first...

[jarin]

Rewrote the recursive parser to use a plain method.

Pruned and annotated the test.

Added other test cases:

• all parameters unused,
• inlining from two different functions,
• stack trace.

This still uses frame variable rather than image lookup, mostly because frame variable tests better the user experience and the cognitive overhead for making the code runnable does not seem to be too high.

[labath]

In D110571#3030192, @jarin wrote:

This still uses frame variable rather than image lookup, mostly because frame variable tests better the user experience and the cognitive overhead for making the code runnable does not seem to be too high.

This is not really about "cognitive overhead", but "who can run this test". With a running process the answer is "a person with a linux x86 machine". With image lookup it's "everyone". It's also easier to debug failures in the test, as less code gets run before you get to the interesting part.

[jarin]

Added a C test (I have also verified that the C test fails without the SymbolFileDWARF patch).

[jarin]

In D110571#3030222, @labath wrote:

In D110571#3030192, @jarin wrote:

This still uses frame variable rather than image lookup, mostly because frame variable tests better the user experience and the cognitive overhead for making the code runnable does not seem to be too high.

This is not really about "cognitive overhead", but "who can run this test". With a running process the answer is "a person with a linux x86 machine". With image lookup it's "everyone". It's also easier to debug failures in the test, as less code gets run before you get to the interesting part.

Good point. Would you prefer to recast the test in terms of image lookup and get rid of checking the stack trace?

[jarin]

Changed the test to avoid running the process and use image lookup instead of frame variable.

I think I would still slightly prefer frame variable, mostly because there seem to be differences between what image lookup and frame variable show (image lookup omit variables that have DW_AT_location disjoint from the inspected address). As opposed to image lookup, frame variable tests more directly what the users would actually use.

[clayborg]

In 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?

Real function is fine and preferable IMHO.

• 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).

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?

• 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.

Anything that tests what the compilers are emitting would be great to have if we can make them.

• 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?

It is really hard to make sure the compiler generates what you want for a test case as it will change over time and you might not end up testing what you think you are testing. The easiest way to avoid this is to emit the assembly from the compiler and then use that as the source for the test since that will guarantee the same output.

• 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?

The image lookup as Pavel suggested is a good way to test info for various addresses without having to run the process or run to multiple locations.

[clayborg]

This looks good to me. Pavel, are you ok with the testing strategy with the updated tests?

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

3654–3655

Maybe expand this comment a bit. If I understand the problem correctly it might read something like:

DW_TAG_inline_subroutine objects may omit DW_TAG_formal_parameter in instances of the function when they are unused or ... . The current DW_TAG_inline_subroutine may refer to another DW_TAG_inline_subroutine or DW_TAG_subprogram that might actually have the definitions of the parameters and we need to include these so they show up in the variables for this function.

[jarin]

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, @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.

[clayborg]

In 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, @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.

[jarin]

In D110571#3031846, @clayborg wrote:

In D110571#3031140, @jarin wrote:

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.

Hi Greg, thanks for the detailed description! What you say is indeed happening until the point "Each of these [blocks] 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.".

With my patch, LLDB creates only one variable here, its symbol context will be whichever block was parsed first and that variable will be inserted into the variable lists of blocks corresponding to 0x210 and 0x410. The reason why LLDB creates only one variable is that there is a cache of variables indexed by DIEs. When we call ParseVariableDIE first time for the variable "unused" (DIE 0x110) and symbol context 0x210, the variable gets created and inserted under the key 0x110. When we call ParseVariableDIE second time for "unused" (still 0x110) and symbol context 0x410, we will find and return the originally created variable (with symbol context 0x210!) and happily insert it into the block for 0x410.

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 to be indexed by a pair <symbol-context-DIE, variable-DIE>.

I have validated this with a simple example below, after adding printing of the variable address (var_sp.get()) at its creation point and printing of variable address (var_sp.get()) and list address (this) at variable list addition. Below is the code and the session log of lldb (with this patch applied).

Code (compiled with a recent clang with -O1 -g):

#include <stdio.h>

__attribute__((always_inline))
void f(int unused) {
  printf("Hello");
}

__attribute__((noinline))
void other() {
  f(1);
}

int main() {
  f(2);
  other();
  return 0;
}

The lldb session (some fluff replaced with ...):

$ bin/lldb a.out
...
(lldb) b f
...
(lldb) r
...
Created var 'unused' 0x7f6c48004f60 from DIE 0x4f   ### 0x4f is the formal_parameter from the abstract |f|
Adding variable 0x7f6c48004f60 to the list 0x7f6c48004910  ### Inserting into the inlined block in |main|
Adding variable 0x7f6c48004f60 to the list 0x7f6c4f0a4a70  ### Ignore, this is the output list for formatting
Process ... stopped
* thread #1, name = 'a.out', stop reason = breakpoint 1.3
    frame #0: 0x0000000000401151 a.out`main [inlined] f(unused=<unavailable>) at a.cc:5:3
...
(lldb) c
...
Adding variable 0x7f6c48004f60 to the list 0x7f6c4804b250 ### Inserting the same var into the block for |other|
Adding variable 0x7f6c48004f60 to the list 0x7f6c4f0a4a70
Process ... stopped
* thread #1, name = 'a.out', stop reason = breakpoint 1.2
    frame #0: 0x0000000000401141 a.out`other() [inlined] f(unused=<unavailable>) at a.cc:5:3
...

[dblaikie]

• 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?

It is really hard to make sure the compiler generates what you want for a test case as it will change over time and you might not end up testing what you think you are testing. The easiest way to avoid this is to emit the assembly from the compiler and then use that as the source for the test since that will guarantee the same output.

If anyone ever needs a hand constructing a stable debug info test case using clang (or other compilers for that matter) - I'm totally happy to help. It's quite possible to constrain the compiler enough and give it easy enough things to inline to make it pretty reliable - for instance for this sort of issue, I'd expect something like this is what I'd use to demonstrate a missing parameter:

__attribute__((optnone)) __attribute__((nodebug)) void use(int*) { }
inline void f1(int a, int b) {
  use(&b);
}
int main() {
  f1(5,  6);
}

This compiled with some optimizations (-O1 or above should be adequate) should result in a single concrete subprogram for main, a single inlined subroutine with a single formal parameter in the inlined subroutine (for 'b') and the abstract origin will have both 'a' and 'b'.

[jarin]

Improved the comment, as Greg suggested.

[labath]

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 ).

In D110571#3031140, @jarin wrote:

I have also packaged the C test, but as Greg notes I am not convinced it will keep testing what it's supposed to.

Given that we have targeted asm tests, I am not particularly worried about that. In fact, one could consider that a feature, as it means we will be able to catch the cases where the compiler output for unused variables changes into something we do not support (that is one of the goals of API tests).

In 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 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.)

[jarin]

In 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.

In 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 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? 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.)

[labath]

In D110571#3033282, @jarin wrote:

In 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, @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 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. :)

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h

391

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

[jarin]

Cache only global variables.

[clayborg]

In D110571#3033685, @jarin wrote:

Cache only global variables.

I concur that we should only cache globals. So now we have unique variables for each block and they have the right symbol context? LGTM if so and if the test suite is happy. Thanks for digging into this.

[jarin]

In D110571#3033481, @labath wrote:

In D110571#3033282, @jarin wrote:

In 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

Pavel, it is unfortunately really the case that with the current patch, the parameters might get interleaved with locals:

#include <stdio.h>

void f(int used, int unused) {
  int local = 1 + used;
  printf("Hello %i", local); // break here
}

int main() {
  f(4, 3);
  return 0;
}

Here is the LLDB session:

$ bin/lldb a.out
...
(lldb) b f
Breakpoint 1: 2 locations.
(lldb) r
...
* thread #1, name = 'a.out', stop reason = breakpoint 1.2
    frame #0: 0x0000000000401151 a.out`main [inlined] f(used=4, unused=<unavailable>) at a.cc:5:3
(lldb) frame var
(int) used = 4
(int) local = 5      <--- HERE, a local variables got between the parameters because we append unused parameters at the end.
(int) unused = <no location, value may have been optimized out>

Let me try to rewrite the code so that the trailing unused parameters are inserted after the last concrete parameter (or at the beginning of variable list if there are no concrete parameters). Let me know if you think it is unnecessary.

[labath]

Just a couple of questions inline.

In D110571#3035814, @jarin wrote:

• thread #1, name = 'a.out', stop reason = breakpoint 1.2 frame #0: 0x0000000000401151 a.out`main [inlined] f(used=4, unused=<unavailable>) at a.cc:5:3

(lldb) frame var
(int) used = 4
(int) local = 5 <--- HERE, a local variables got between the parameters because we append unused parameters at the end.
(int) unused = <no location, value may have been optimized out>

Let me try to rewrite the code so that the trailing unused parameters are inserted after the last concrete parameter (or at the beginning of variable list if there are no concrete parameters). Let me know if you think it is unnecessary.

TBH, I have no idea. The function description comes out right, so it seems at least some parts of lldb are prepared to handle this. I suppose it would be nicer if they were grouped together, but if it makes the code significantly more complex, then I probably wouldn't bother.

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

3545–3546

I'm wondering if one could pass a single ArrayRef<DWARFDIE> & argument instead of the array+index pair. FWICS, you're processing the list in a left-to-right fashion, which seems ideal for ArrayRef::drop_front. WDYT?

3566–3567

All of these abstract origin loops make me uneasy. They make it very easy to hang (whether deliberately or not) the debugger with a bit of incorrect dwarf (self-referencing DIEs). Do we actually know of any abstract_origin chains?
It's not really clear to me what would be the right interpretation of that, so I can't even say whether this algorithm would be correct there.

Maybe just stick to a single "dereference" ?

lldb/test/API/functionalities/unused-inlined-parameters/TestUnusedInlinedParameters.py

17 ↗

(On Diff #376235)

Maybe we could check something else as well... Do GetDescription or str(value) return something reasonable here?

lldb/test/Shell/SymbolFile/DWARF/x86/unused-inlined-params.test

2

You should be able to drop this now.

34

You could add CHECK-NOT: partial here

[jarin]

Addressed reviewer comments, separated merging of the abstract parameters into a function, prevented interleaving of parameters with locals.

[jarin]

Thank you for the great comments, Pavel. I took a stab at merging the parameters without interleaving them with the locals. Let me know what you think; I can certainly put this back to the original state if you think this is a change for the worse.

(I am sorry for the churn, but I feel the code is fairly subtle and would like to leave it in a state we are all happy with.)

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

3545–3546

I have replaced the in place-merging with a separate merging function, so this should not be relevant anymore.

lldb/test/API/functionalities/unused-inlined-parameters/TestUnusedInlinedParameters.py

17 ↗

(On Diff #376235)

Actually, the most important thing is the type here, so this was quite deliberate.

GetDescription returns (void *) unused1 = <no location, value may have been optimized out>\n\n, but I am not sure if we can safely require that all future versions/platforms optimize the parameter out.

[labath]

Looks good, apart from some stylistic comments inline. Thank you for taking the time to do this right.

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

3543

In llvm, we prefer static functions over anonymous namespaces. Theoretically, you could keep the anonymous namespace around the using declaration (per https://llvm.org/docs/CodingStandards.html#anonymous-namespaces, as those can't use static), though I would actually probably prefer DIEArray type defined in DIERef.h over a custom type.

3586

I would assume this is redundant, as an invalid DIE will never match abstract_child.

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h

395

When you're not mutating the vector, the usual argument type is ArrayRef<T>

396

We generally do not put a const qualifier on by-value arguments (it's pretty useless).

(I see it's present on other functions too, but I don't know if they were introduced by you or you're just propagating them.)

lldb/test/API/functionalities/unused-inlined-parameters/TestUnusedInlinedParameters.py

17 ↗

(On Diff #376235)

I don't feel strongly about it, but I would say that this function is so simple than any optimizer worthy of that name should be able to optimize those arguments away. I might replace printf with a noinline/optnone function though, to avoid any libc shenanigans.

[jarin]

Addressed Pavel's comments.

[jarin]

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

3543

Changed to static function, DIEArray (interestingly, this file actually starts with anonymous namespace, see line 121).

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h

396

Copy pasta, unfortunately.

lldb/test/API/functionalities/unused-inlined-parameters/TestUnusedInlinedParameters.py

17 ↗

(On Diff #376235)

I do not feel too strongly about this eiher.

[labath]

Let's ship this.

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

3543

We're not always very good at following llvm policies, although I would say that this particular namespace is mostly ok-ish -- it mostly contains type declarations (classes, enums), where static does not work.

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h

lldb_private::Type *ResolveTypeUID(const DWARFDIE &die,
                                      bool assert_not_being_parsed);
 
   lldb_private::Type *ResolveTypeUID(const DIERef &die_ref);
 
   lldb::VariableSP ParseVariableDIE(const lldb_private::SymbolContext &sc,
                                     const DWARFDIE &die,
                                     const lldb::addr_t func_low_pc);
 
-  void
-  ParseAndAppendGlobalVariable(const lldb_private::SymbolContext &sc,
-                               const DWARFDIE &die,
-                               lldb_private::VariableList &cc_variable_list);
+  void ParseAndAppendGlobalVariable(
+      const lldb_private::SymbolContext &sc, const DWARFDIE &die,
 
   size_t ParseVariablesInFunctionContext(const lldb_private::SymbolContext &sc,
                                          const DWARFDIE &die,
                                          const lldb::addr_t func_low_pc);
 
   bool ClassOrStructIsVirtual(const DWARFDIE &die);
 
   // Given a die_offset, figure out the symbol context representing that die.
   bool ResolveFunction(const DWARFDIE &die, bool include_inlines,
                        lldb_private::SymbolContextList &sc_list);
 
   /// Resolve functions and (possibly) blocks for the given file address and a
   /// compile unit. The compile unit comes from the sc argument and it must be
   /// set. The results of the lookup (if any) are written back to the symbol
   /// context.
   void ResolveFunctionAndBlock(lldb::addr_t file_vm_addr, bool lookup_block,
                                lldb_private::SymbolContext &sc);
 
   virtual lldb::TypeSP

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

void SymbolFileDWARF::ParseAndAppendGlobalVariable(
 
   // Check to see if we have already parsed this variable or constant?
   VariableSP var_sp = GetDIEToVariable()[die.GetDIE()];
   if (var_sp) {
     cc_variable_list.AddVariableIfUnique(var_sp);
     return;
   }
 
-  // We haven't already parsed it, lets do that now.
+  // We haven't parsed the variable yet, lets do that now. Also, let us include
+  // the variable in the relevant compilation unit's variable list, if it
+  // exists.
   VariableListSP variable_list_sp;
   DWARFDIE sc_parent_die = GetParentSymbolContextDIE(die);
   dw_tag_t parent_tag = sc_parent_die.Tag();
   switch (parent_tag) {
   case DW_TAG_compile_unit:
   case DW_TAG_partial_unit:
     if (sc.comp_unit != nullptr) {
       variable_list_sp = sc.comp_unit->GetVariableList(false);
void SymbolFileDWARF::ParseAndAppendGlobalVariable(
   if (variable_list_sp)
     variable_list_sp->AddVariableIfUnique(var_sp);
 }
 
 size_t SymbolFileDWARF::ParseVariablesInFunctionContext(
     const SymbolContext &sc, const DWARFDIE &die,
     const lldb::addr_t func_low_pc) {
   if (!die || !sc.function)
     return 0;
 
-  const std::function<size_t(const DWARFDIE &, VariableListSP)>
+  using DIEVector = llvm::SmallVector<DWARFDIE, 2>;
+
+  const std::function<size_t(const DWARFDIE &, VariableListSP, DIEVector &,
+                             size_t &)>
       parse_recursive = [this, func_low_pc, &parse_recursive,
                          &sc](const DWARFDIE &die,
-                              VariableListSP variable_list_sp) -> size_t {
+                              VariableListSP variable_list_sp,
+                              DIEVector &abstract_formal_parameters,
+                              size_t &abstract_parameter_index) -> size_t {
     size_t vars_added = 0;
     dw_tag_t tag = die.Tag();
 
+    // If we are parsing a formal parameter, let us make sure that we insert
+    // preceding abstract parameters if they were omitted from the concrete
+    // function instance.
+    if (tag == DW_TAG_formal_parameter) {
+      while (abstract_parameter_index <
+             abstract_formal_parameters.size()) {
+        const DWARFDIE &abstract_parameter =
+            abstract_formal_parameters[abstract_parameter_index++];
+        DWARFDIE parameter = die;
+        bool found = false;
+        while (parameter) {
+          parameter = parameter.GetReferencedDIE(DW_AT_abstract_origin);
+          if (parameter == abstract_parameter) {
+            found = true;
+            break;
+          }
+        }
+        if (found)
+          break;
+        VariableSP var_sp(ParseVariableDIE(sc, abstract_parameter, func_low_pc));
+        if (var_sp) {
+          variable_list_sp->AddVariableIfUnique(var_sp);
+          ++vars_added;
+        }
+      }
+    }
+
+    // If |die| is a variable, parse it and insert it.
     if ((tag == DW_TAG_variable) || (tag == DW_TAG_constant) ||
         (tag == DW_TAG_formal_parameter)) {
       VariableSP var_sp(ParseVariableDIE(sc, die, func_low_pc));
       if (var_sp) {
         variable_list_sp->AddVariableIfUnique(var_sp);
         ++vars_added;
       }
     }
 
     switch (tag) {
     case DW_TAG_subprogram:
     case DW_TAG_inlined_subroutine:
     case DW_TAG_lexical_block: {
-      // If we start a new block, compute a new block context and recurse.
+      // If we are starting a new block, compute a new block context and recurse
+      // in that context.
       Block *block = sc.function->GetBlock(true).FindBlockByID(die.GetID());
       if (block == nullptr) {
         // This must be a specification or abstract origin with a
         // concrete block counterpart in the current function. We need
         // to find the concrete block so we can correctly add the
         // variable to it
         const DWARFDIE concrete_block_die = FindBlockContainingSpecification(
             GetDIE(sc.function->GetID()), die.GetOffset());
         if (concrete_block_die)
           block = sc.function->GetBlock(true).FindBlockByID(
               concrete_block_die.GetID());
       }
 
       if (block == nullptr)
         return 0;
 
       const bool can_create = false;
       VariableListSP block_variable_list_sp =
           block->GetBlockVariableList(can_create);
       if (block_variable_list_sp.get() == nullptr) {
         block_variable_list_sp = std::make_shared<VariableList>();
         block->SetVariableList(block_variable_list_sp);
       }
+
+      DIEVector block_abstract_formal_parameters;
+      if (tag == DW_TAG_inlined_subroutine) {
+        // Walk abstract origins until we find DW_TAG_subprogram and extract
+        // its formal parameters.
+        DWARFDIE abs_die = die;
+        while (abs_die && abs_die.Tag() != DW_TAG_subprogram) {
+          abs_die = abs_die.GetReferencedDIE(DW_AT_abstract_origin);
+        }
+        if (abs_die && abs_die.HasChildren()) {
+          for (DWARFDIE child = abs_die.GetFirstChild(); child;
+               child = child.GetSibling()) {
+            if (child.Tag() == DW_TAG_formal_parameter) {
+              block_abstract_formal_parameters.push_back(child);
+            }
+          }
+        }
+      }
+
+      size_t block_parameter_index = 0;
+      // Recursively parse children in the context of the block.
       for (DWARFDIE child = die.GetFirstChild(); child;
            child = child.GetSibling()) {
-        vars_added += parse_recursive(child, block_variable_list_sp);
+        vars_added += parse_recursive(child, block_variable_list_sp,
+                                      block_abstract_formal_parameters,
+                                      block_parameter_index);
       }
 
+      // If there any remaining abstract formal parameters, parse and insert
+      // them as well.
+      while (block_parameter_index < block_abstract_formal_parameters.size()) {
+        const DWARFDIE &abstract_parameter =
+            block_abstract_formal_parameters[block_parameter_index++];
+        VariableSP var_sp(
+            ParseVariableDIE(sc, abstract_parameter, func_low_pc));
+        if (var_sp) {
+          block_variable_list_sp->AddVariableIfUnique(var_sp);
+          ++vars_added;
+        }
+      }
       break;
     }
 
     default:
       // Recurse to children with the same block context.
       for (DWARFDIE child = die.GetFirstChild(); child;
            child = child.GetSibling()) {
-        vars_added += parse_recursive(child, variable_list_sp);
+        vars_added +=
+            parse_recursive(child, variable_list_sp, abstract_formal_parameters,
+                            abstract_parameter_index);
       }
-
       break;
     }
 
     return vars_added;
   };
 
   Block *block = sc.function->GetBlock(true).FindBlockByID(die.GetID());
   const bool can_create = false;
   VariableListSP variable_list_sp = block->GetBlockVariableList(can_create);
-  return parse_recursive(die, variable_list_sp);
+  DIEVector block_abstract_formal_parameters;
+  size_t block_parameter_index = 0;
+  return parse_recursive(die, variable_list_sp,
+                         block_abstract_formal_parameters,
+                         block_parameter_index);
 }
 
 /// Collect call site parameters in a DW_TAG_call_site DIE.
 static CallSiteParameterArray
 CollectCallSiteParameters(ModuleSP module, DWARFDIE call_site_die) {
   CallSiteParameterArray parameters;
   for (DWARFDIE child : call_site_die.children()) {
     if (child.Tag() != DW_TAG_call_site_parameter &&

lldb/test/Shell/SymbolFile/DWARF/x86/Inputs/unused-inlined-params.s

+# Generated from the following program (with some pruning):
+#
+# 1   #include <stdio.h>
+# 2
+# 3   __attribute__((always_inline))
+# 4   void f(void* unused1, int used, int unused2, int partial, int unused3) {
+# 5     used += partial;
+# 6     printf("%i", partial);
+# 7     printf("%i", used);
+# 8   }
+# 9
+# 10  int main(int argc, char** argv) {
+# 11    f(argv, 42, 1, argc, 2);
+# 12    return 0;
+# 13  }
+  .text
+  .file "unused-inlined-params.c"
+  .globl  main                            # -- Begin function main
+  .p2align  4, 0x90
+  .type main,@function
+main:                                   # @main
+.Lfunc_begin1:
+  .file 1 "/example" "unused-inlined-params.c"
+  .loc  1 10 0                          # unused-inlined-params.c:10:0
+  .cfi_startproc
+# %bb.0:
+  #DEBUG_VALUE: main:argc <- $edi
+  #DEBUG_VALUE: main:argv <- $rsi
+  #DEBUG_VALUE: f:partial <- $edi
+  pushq %rbx
+  .cfi_def_cfa_offset 16
+  .cfi_offset %rbx, -16
+  movl  %edi, %esi
+.Lline5:
+  #DEBUG_VALUE: main:argv <- [DW_OP_LLVM_entry_value 1] $rsi
+  #DEBUG_VALUE: f:unused3 <- undef
+  #DEBUG_VALUE: f:unused2 <- undef
+  #DEBUG_VALUE: f:used <- 42
+  #DEBUG_VALUE: f:unused1 <- undef
+  #DEBUG_VALUE: f:partial <- $esi
+  #DEBUG_VALUE: main:argc <- $esi
+  .loc  1 5 8 prologue_end              # unused-inlined-params.c:5:8
+  leal  42(%rsi), %ebx
+.Lline6:
+  #DEBUG_VALUE: f:used <- $ebx
+  .loc  1 6 3                           # unused-inlined-params.c:6:3
+  movl  $.L.str, %edi
+                                        # kill: def $esi killed $esi killed $rsi
+  xorl  %eax, %eax
+  callq printf
+.Lline7:
+  .loc  1 7 3                           # unused-inlined-params.c:7:3
+  movl  $.L.str, %edi
+  movl  %ebx, %esi
+  xorl  %eax, %eax
+  callq printf
+.Lline12:
+  .loc  1 12 3                          # unused-inlined-params.c:12:3
+  xorl  %eax, %eax
+  popq  %rbx
+.Lreturn1:
+  .cfi_def_cfa_offset 8
+  retq
+.Lfunc_end1:
+  .size main, .Lfunc_end1-main
+  .cfi_endproc
+                                                # -- End function
+# Dummy printf implementation.
+printf:
+        retq
+
+# Simple implementation of _start - set up argc, argv and tail-call main.
+  .globl  _start
+  .p2align  4, 0x90
+  .type _start,@function
+_start:
+        movl    (%rsp), %edi
+        leaq    4(%rsp), %rsi
+        jmp     main
+
+  .type .L.str,@object                  # @.str
+  .section  .rodata.str1.1,"aMS",@progbits,1
+.L.str:
+  .asciz  "%i"
+  .size .L.str, 3
+
+  .section  .debug_loc,"",@progbits
+.Ldebug_loc_argc:
+  .quad .Lfunc_begin1-.Lfunc_begin1
+  .quad .Lline5-.Lfunc_begin1
+  .short  1                               # Loc expr size
+  .byte 85                              # super-register DW_OP_reg5
+  .quad .Lline5-.Lfunc_begin1
+  .quad .Lline7-.Lfunc_begin1
+  .short  1                               # Loc expr size
+  .byte 84                              # super-register DW_OP_reg4
+  .quad 0
+  .quad 0
+.Ldebug_loc3:
+  .quad .Lfunc_begin1-.Lfunc_begin1
+  .quad .Lline5-.Lfunc_begin1
+  .short  1                               # Loc expr size
+  .byte 84                              # DW_OP_reg4
+  .quad .Lline5-.Lfunc_begin1
+  .quad .Lfunc_end1-.Lfunc_begin1
+  .short  4                               # Loc expr size
+  .byte 243                             # DW_OP_GNU_entry_value
+  .byte 1                               # 1
+  .byte 84                              # DW_OP_reg4
+  .byte 159                             # DW_OP_stack_value
+  .quad 0
+  .quad 0
+.Ldebug_loc4:
+  .quad .Lfunc_begin1-.Lfunc_begin1
+  .quad .Lline5-.Lfunc_begin1
+  .short  1                               # Loc expr size
+  .byte 85                              # super-register DW_OP_reg5
+  .quad .Lline5-.Lfunc_begin1
+  .quad .Lline7-.Lfunc_begin1
+  .short  1                               # Loc expr size
+  .byte 84                              # super-register DW_OP_reg4
+  .quad 0
+  .quad 0
+.Ldebug_loc5:
+  .quad .Lline5-.Lfunc_begin1
+  .quad .Lline6-.Lfunc_begin1
+  .short  3                               # Loc expr size
+  .byte 17                              # DW_OP_consts
+  .byte 42                              # 42
+  .byte 159                             # DW_OP_stack_value
+  .quad .Lline6-.Lfunc_begin1
+  .quad .Lreturn1-.Lfunc_begin1
+  .short  1                               # Loc expr size
+  .byte 83                              # super-register DW_OP_reg3
+  .quad 0
+  .quad 0
+  .section  .debug_abbrev,"",@progbits
+  .byte 1                               # Abbreviation Code
+  .byte 17                              # DW_TAG_compile_unit
+  .byte 1                               # DW_CHILDREN_yes
+  .byte 19                              # DW_AT_language
+  .byte 5                               # DW_FORM_data2
+  .byte 3                               # DW_AT_name
+  .byte 14                              # DW_FORM_strp
+  .byte 16                              # DW_AT_stmt_list
+  .byte 23                              # DW_FORM_sec_offset
+  .byte 27                              # DW_AT_comp_dir
+  .byte 14                              # DW_FORM_strp
+  .byte 17                              # DW_AT_low_pc
+  .byte 1                               # DW_FORM_addr
+  .byte 18                              # DW_AT_high_pc
+  .byte 6                               # DW_FORM_data4
+  .byte 0                               # EOM(1)
+  .byte 0                               # EOM(2)
+  .byte 4                               # Abbreviation Code
+  .byte 5                               # DW_TAG_formal_parameter
+  .byte 0                               # DW_CHILDREN_no
+  .byte 2                               # DW_AT_location
+  .byte 23                              # DW_FORM_sec_offset
+  .byte 49                              # DW_AT_abstract_origin
+  .byte 19                              # DW_FORM_ref4
+  .byte 0                               # EOM(1)
+  .byte 0                               # EOM(2)
+  .byte 5                               # Abbreviation Code
+  .byte 46                              # DW_TAG_subprogram
+  .byte 1                               # DW_CHILDREN_yes
+  .byte 3                               # DW_AT_name
+  .byte 14                              # DW_FORM_strp
+  .byte 58                              # DW_AT_decl_file
+  .byte 11                              # DW_FORM_data1
+  .byte 59                              # DW_AT_decl_line
+  .byte 11                              # DW_FORM_data1
+  .byte 39                              # DW_AT_prototyped
+  .byte 25                              # DW_FORM_flag_present
+  .byte 63                              # DW_AT_external
+  .byte 25                              # DW_FORM_flag_present
+  .byte 32                              # DW_AT_inline
+  .byte 11                              # DW_FORM_data1
+  .byte 0                               # EOM(1)
+  .byte 0                               # EOM(2)
+  .byte 6                               # Abbreviation Code
+  .byte 5                               # DW_TAG_formal_parameter
+  .byte 0                               # DW_CHILDREN_no
+  .byte 3                               # DW_AT_name
+  .byte 14                              # DW_FORM_strp
+  .byte 58                              # DW_AT_decl_file
+  .byte 11                              # DW_FORM_data1
+  .byte 59                              # DW_AT_decl_line
+  .byte 11                              # DW_FORM_data1
+  .byte 73                              # DW_AT_type
+  .byte 19                              # DW_FORM_ref4
+  .byte 0                               # EOM(1)
+  .byte 0                               # EOM(2)
+  .byte 7                               # Abbreviation Code
+  .byte 15                              # DW_TAG_pointer_type
+  .byte 0                               # DW_CHILDREN_no
+  .byte 0                               # EOM(1)
+  .byte 0                               # EOM(2)
+  .byte 8                               # Abbreviation Code
+  .byte 36                              # DW_TAG_base_type
+  .byte 0                               # DW_CHILDREN_no
+  .byte 3                               # DW_AT_name
+  .byte 14                              # DW_FORM_strp
+  .byte 62                              # DW_AT_encoding
+  .byte 11                              # DW_FORM_data1
+  .byte 11                              # DW_AT_byte_size
+  .byte 11                              # DW_FORM_data1
+  .byte 0                               # EOM(1)
+  .byte 0                               # EOM(2)
+  .byte 9                               # Abbreviation Code
+  .byte 46                              # DW_TAG_subprogram
+  .byte 1                               # DW_CHILDREN_yes
+  .byte 17                              # DW_AT_low_pc
+  .byte 1                               # DW_FORM_addr
+  .byte 18                              # DW_AT_high_pc
+  .byte 6                               # DW_FORM_data4
+  .byte 64                              # DW_AT_frame_base
+  .byte 24                              # DW_FORM_exprloc
+  .ascii  "\227B"                         # DW_AT_GNU_all_call_sites
+  .byte 25                              # DW_FORM_flag_present
+  .byte 3                               # DW_AT_name
+  .byte 14                              # DW_FORM_strp
+  .byte 58                              # DW_AT_decl_file
+  .byte 11                              # DW_FORM_data1
+  .byte 59                              # DW_AT_decl_line
+  .byte 11                              # DW_FORM_data1
+  .byte 39                              # DW_AT_prototyped
+  .byte 25                              # DW_FORM_flag_present
+  .byte 73                              # DW_AT_type
+  .byte 19                              # DW_FORM_ref4
+  .byte 63                              # DW_AT_external
+  .byte 25                              # DW_FORM_flag_present
+  .byte 0                               # EOM(1)
+  .byte 0                               # EOM(2)
+  .byte 10                              # Abbreviation Code
+  .byte 5                               # DW_TAG_formal_parameter
+  .byte 0                               # DW_CHILDREN_no
+  .byte 2                               # DW_AT_location
+  .byte 23                              # DW_FORM_sec_offset
+  .byte 3                               # DW_AT_name
+  .byte 14                              # DW_FORM_strp
+  .byte 58                              # DW_AT_decl_file
+  .byte 11                              # DW_FORM_data1
+  .byte 59                              # DW_AT_decl_line
+  .byte 11                              # DW_FORM_data1
+  .byte 73                              # DW_AT_type
+  .byte 19                              # DW_FORM_ref4
+  .byte 0                               # EOM(1)
+  .byte 0                               # EOM(2)
+  .byte 11                              # Abbreviation Code
+  .byte 29                              # DW_TAG_inlined_subroutine
+  .byte 1                               # DW_CHILDREN_yes
+  .byte 49                              # DW_AT_abstract_origin
+  .byte 19                              # DW_FORM_ref4
+  .byte 17                              # DW_AT_low_pc
+  .byte 1                               # DW_FORM_addr
+  .byte 18                              # DW_AT_high_pc
+  .byte 6                               # DW_FORM_data4
+  .byte 88                              # DW_AT_call_file
+  .byte 11                              # DW_FORM_data1
+  .byte 89                              # DW_AT_call_line
+  .byte 11                              # DW_FORM_data1
+  .byte 87                              # DW_AT_call_column
+  .byte 11                              # DW_FORM_data1
+  .byte 0                               # EOM(1)
+  .byte 0                               # EOM(2)
+  .byte 12                              # Abbreviation Code
+  .byte 15                              # DW_TAG_pointer_type
+  .byte 0                               # DW_CHILDREN_no
+  .byte 73                              # DW_AT_type
+  .byte 19                              # DW_FORM_ref4
+  .byte 0                               # EOM(1)
+  .byte 0                               # EOM(2)
+  .byte 0                               # EOM(3)
+  .section  .debug_info,"",@progbits
+.Lcu_begin0:
+  .long .Ldebug_info_end0-.Ldebug_info_start0 # Length of Unit
+.Ldebug_info_start0:
+  .short  4                               # DWARF version number
+  .long .debug_abbrev                   # Offset Into Abbrev. Section
+  .byte 8                               # Address Size (in bytes)
+  .byte 1                               # Abbrev [1] 0xb:0x11c DW_TAG_compile_unit
+  .short  12                              # DW_AT_language
+  .long .Linfo_string1                  # DW_AT_name
+  .long .Lline_table_start0             # DW_AT_stmt_list
+  .long .Linfo_string2                  # DW_AT_comp_dir
+  .quad .Lfunc_begin1                   # DW_AT_low_pc
+  .long .Lfunc_end1-.Lfunc_begin1       # DW_AT_high_pc
+.Ldebug_info_f:
+  .byte 5                               # Abbrev [5] 0x5f:0x40 DW_TAG_subprogram
+  .long .Linfo_string3                  # DW_AT_name
+  .byte 1                               # DW_AT_decl_file
+  .byte 4                               # DW_AT_decl_line
+                                        # DW_AT_prototyped
+                                        # DW_AT_external
+  .byte 1                               # DW_AT_inline
+.Ldebug_info_param1:
+  .byte 6                               # Abbrev [6] 0x67:0xb DW_TAG_formal_parameter
+  .long .Linfo_string4                  # DW_AT_name
+  .byte 1                               # DW_AT_decl_file
+  .byte 4                               # DW_AT_decl_line
+  .long .Ldebug_info_void_ptr-.Lcu_begin0
+                                          # DW_AT_type
+.Ldebug_info_param2:
+  .byte 6                               # Abbrev [6] 0x72:0xb DW_TAG_formal_parameter
+  .long .Linfo_string5                  # DW_AT_name
+  .byte 1                               # DW_AT_decl_file
+  .byte 4                               # DW_AT_decl_line
+  .long .Ldebug_info_int-.Lcu_begin0    # DW_AT_type
+.Ldebug_info_param3:
+  .byte 6                               # Abbrev [6] 0x7d:0xb DW_TAG_formal_parameter
+  .long .Linfo_string7                  # DW_AT_name
+  .byte 1                               # DW_AT_decl_file
+  .byte 4                               # DW_AT_decl_line
+  .long .Ldebug_info_int-.Lcu_begin0    # DW_AT_type
+.Ldebug_info_param4:
+  .byte 6                               # Abbrev [6] 0x88:0xb DW_TAG_formal_parameter
+  .long .Linfo_string8                  # DW_AT_name
+  .byte 1                               # DW_AT_decl_file
+  .byte 4                               # DW_AT_decl_line
+  .long .Ldebug_info_int-.Lcu_begin0    # DW_AT_type
+.Ldebug_info_param5:
+  .byte 6                               # Abbrev [6] 0x93:0xb DW_TAG_formal_parameter
+  .long .Linfo_string9                  # DW_AT_name
+  .byte 1                               # DW_AT_decl_file
+  .byte 4                               # DW_AT_decl_line
+  .long .Ldebug_info_int-.Lcu_begin0    # DW_AT_type
+  .byte 0                               # End Of Children Mark
+.Ldebug_info_void_ptr:
+  .byte 7                               # Abbrev [7] 0x9f:0x1 DW_TAG_pointer_type
+.Ldebug_info_int:
+  .byte 8                               # Abbrev [8] 0xa0:0x7 DW_TAG_base_type
+  .long .Linfo_string6                  # DW_AT_name
+  .byte 5                               # DW_AT_encoding
+  .byte 4                               # DW_AT_byte_size
+  .byte 9                               # Abbrev [9] 0xa7:0x6e DW_TAG_subprogram
+  .quad .Lfunc_begin1                   # DW_AT_low_pc
+  .long .Lfunc_end1-.Lfunc_begin1       # DW_AT_high_pc
+  .byte 1                               # DW_AT_frame_base
+  .byte 87
+                                        # DW_AT_GNU_all_call_sites
+  .long .Linfo_string10                 # DW_AT_name
+  .byte 1                               # DW_AT_decl_file
+  .byte 10                              # DW_AT_decl_line
+                                        # DW_AT_prototyped
+  .long .Ldebug_info_int-.Lcu_begin0    # DW_AT_type
+                                        # DW_AT_external
+  .byte 10                              # Abbrev [10] 0xc0:0xf DW_TAG_formal_parameter
+  .long .Ldebug_loc_argc                # DW_AT_location
+  .long .Linfo_string11                 # DW_AT_name
+  .byte 1                               # DW_AT_decl_file
+  .byte 10                              # DW_AT_decl_line
+  .long .Ldebug_info_int-.Lcu_begin0    # DW_AT_type
+  .byte 10                              # Abbrev [10] 0xcf:0xf DW_TAG_formal_parameter
+  .long .Ldebug_loc3                    # DW_AT_location
+  .long .Linfo_string12                 # DW_AT_name
+  .byte 1                               # DW_AT_decl_file
+  .byte 10                              # DW_AT_decl_line
+  .long .Ldebug_info_char_ptr_ptr-.Lcu_begin0
+                                          # DW_AT_type
+  .byte 11                              # Abbrev [11] 0xde:0x36 DW_TAG_inlined_subroutine
+  .long .Ldebug_info_f-.Lcu_begin0
+                                          # DW_AT_abstract_origin
+  .quad .Lline5                         # DW_AT_low_pc
+  .long .Lline12-.Lline5                # DW_AT_high_pc
+  .byte 1                               # DW_AT_call_file
+  .byte 11                              # DW_AT_call_line
+  .byte 3                               # DW_AT_call_column
+  .byte 4                               # Abbrev [4] 0xf7:0x9 DW_TAG_formal_parameter
+  .long .Ldebug_loc5                    # DW_AT_location
+  .long .Ldebug_info_param2-.Lcu_begin0
+                                          # DW_AT_abstract_origin
+  .byte 4                               # Abbrev [4] 0x105:0x9 DW_TAG_formal_parameter
+  .long .Ldebug_loc4                    # DW_AT_location
+  .long .Ldebug_info_param4-.Lcu_begin0
+                                          # DW_AT_abstract_origin
+  .byte 0                               # End Of Children Mark
+  .byte 0                               # End Of Children Mark
+.Ldebug_info_char_ptr_ptr:
+  .byte 12                              # Abbrev [12] 0x115:0x5 DW_TAG_pointer_type
+  .long .Ldebug_info_char_ptr-.Lcu_begin0
+                                          # DW_AT_type
+.Ldebug_info_char_ptr:
+  .byte 12                              # Abbrev [12] 0x11a:0x5 DW_TAG_pointer_type
+  .long .Ldebug_info_char-.Lcu_begin0   # DW_AT_type
+.Ldebug_info_char:
+  .byte 8                               # Abbrev [8] 0x11f:0x7 DW_TAG_base_type
+  .long .Linfo_string13                 # DW_AT_name
+  .byte 6                               # DW_AT_encoding
+  .byte 1                               # DW_AT_byte_size
+  .byte 0                               # End Of Children Mark
+.Ldebug_info_end0:
+  .section  .debug_str,"MS",@progbits,1
+.Linfo_string1:
+  .asciz  "unused-inlined-params.c"       # string offset=30
+.Linfo_string2:
+  .asciz  "/usr/local/google/home/jarin/projects/emsample/ccpp" # string offset=54
+.Linfo_string3:
+  .asciz  "f"                             # string offset=106
+.Linfo_string4:
+  .asciz  "unused1"                       # string offset=108
+.Linfo_string5:
+  .asciz  "used"                          # string offset=116
+.Linfo_string6:
+  .asciz  "int"                           # string offset=121
+.Linfo_string7:
+  .asciz  "unused2"                       # string offset=125
+.Linfo_string8:
+  .asciz  "partial"                       # string offset=133
+.Linfo_string9:
+  .asciz  "unused3"                       # string offset=141
+.Linfo_string10:
+  .asciz  "main"                          # string offset=149
+.Linfo_string11:
+  .asciz  "argc"                          # string offset=154
+.Linfo_string12:
+  .asciz  "argv"                          # string offset=159
+.Linfo_string13:
+  .asciz  "char"                          # string offset=164
+  .section  ".note.GNU-stack","",@progbits
+  .addrsig
+  .section  .debug_line,"",@progbits
+.Lline_table_start0:

lldb/test/Shell/SymbolFile/DWARF/x86/unused-inlined-params.test

+# UNSUPPORTED: system-darwin, system-windows
+
+# REQUIRES: lld
+
+# RUN: llvm-mc -filetype=obj %S/Inputs/unused-inlined-params.s \
+# RUN:         -triple x86_64-pc-linux -o %t.o
+# RUN: ld.lld %t.o -o %t
+# RUN: %lldb %t -s %s -o exit | FileCheck %s
+
+breakpoint set -n f
+process launch
+
+# CHECK: Process {{.*}} stopped
+
+# Let us check that unused parameters (of an inlined function) are listed.
+frame variable
+# CHECK-LABEL: frame variable
+# CHECK: (void *) unused1 = <
+# CHECK: (int) used = 42
+# CHECK: (int) unused2 = <
+# CHECK: (int) partial = 1
+# CHECK: (int) unused3 = <
+
+# Step out of the live range of the 'partial' parameter.
+next
+next
+# Check the variable contents.
+frame variable
+# CHECK-LABEL: frame variable
+# CHECK: (void *) unused1 = <
+# CHECK: (int) used = 43
+# CHECK: (int) unused2 = <
+# CHECK: (int) partial = <
+# CHECK: (int) unused3 = <