Page MenuHomePhabricator

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

Authored by jarin on Mon, Sep 27, 11:54 AM.

Details

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.

Diff Detail

Event Timeline

jarin created this revision.Mon, Sep 27, 11:54 AM
jarin requested review of this revision.Mon, Sep 27, 11:54 AM

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?

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

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
33–35

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

(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 added a comment.Tue, Sep 28, 5:51 AM

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

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

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.

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 updated this revision to Diff 375865.Wed, Sep 29, 6:46 AM

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.

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 updated this revision to Diff 375886.Wed, Sep 29, 7:47 AM

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

jarin added a comment.Wed, Sep 29, 7:50 AM

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 updated this revision to Diff 375901.Wed, Sep 29, 8:24 AM

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.

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.

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

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
3677–3678

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.

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:

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.

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:

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.

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
...
  • 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 updated this revision to Diff 376124.Thu, Sep 30, 12:50 AM
jarin marked an inline comment as done.

Improved the comment, as Greg suggested.

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

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

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 added a comment.Thu, Sep 30, 6:36 AM

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.

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

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

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
395

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

jarin updated this revision to Diff 376235.Thu, Sep 30, 8:34 AM
jarin marked an inline comment as done.

Cache only global variables.

clayborg accepted this revision.Thu, Sep 30, 1:53 PM

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.

This revision is now accepted and ready to land.Thu, Sep 30, 1:53 PM
jarin added a comment.Fri, Oct 1, 1:32 AM

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 added a comment.Fri, Oct 1, 2:09 AM

Just a couple of questions inline.

  • 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
3637–3638

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?

3649–3650

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
18

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 updated this revision to Diff 376501.Fri, Oct 1, 6:25 AM
jarin marked 5 inline comments as done.

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

jarin added a comment.Fri, Oct 1, 6:29 AM

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
3637–3638

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
18

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 accepted this revision.Mon, Oct 4, 4:49 AM

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
3544

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.

3587

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

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

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

400

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
18

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 updated this revision to Diff 377020.Mon, Oct 4, 1:18 PM
jarin marked 4 inline comments as done.

Addressed Pavel's comments.

jarin added inline comments.Mon, Oct 4, 1:19 PM
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
3544

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

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

Copy pasta, unfortunately.

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

I do not feel too strongly about this eiher.

labath accepted this revision.Mon, Oct 4, 11:46 PM

Let's ship this.

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

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.