[dwarf] Ignore DW_AT_linkage_name and DIEs with DW_AT_declaration #268

haberman · 2021-08-05T18:36:29Z

DWARF DIEs which have DW_AT_declaration are declarations, not definitions, and shouldn't be counted against the compileunit where they appear. This PR will cause us to completely ignore any DIE which has DW_AT_declaration set.

We also stop paying attention to DW_AT_linkage_name. We can always get the same information from either DW_AT_location or low/hi pc pairs.

This also improves the logic around deciding whether the high_pc value is a size or an absolute address. This logic should be based on the DWARF form.

Fixes: #236

dwblaikie

Couple of minor thoughts/tangents - though broader question: What does Bloaty do with the information if it isn't a declaration? The test case doesn't seem realistic to me - I wouldn't expect to see a declaration with a high/low_pc (a declaration with code seems like a contradiction) - so maybe something more realistic would help explain/motivate the feature?

src/dwarf.cc

haberman · 2021-08-05T22:07:32Z

What does Bloaty do with the information if it isn't a declaration?

For any DIE that is not a declaration (and is not stripped), Bloaty will look for any information it can find in the DIE that will associate a region of the binary with the current compileunit. That includes

DW_AT_linkage_name: We will look up the symbol and attribute its range to this CU
DW_AT_low_pc/DW_AT_high_pc: We will attribute the given address range to this CU
DW_AT_ranges/DW_AT_start_scope: We will attribute the ranges given in either kind of range list to this CU
DW_AT_location: We will try to parse the location expression and attribute the target to this CU

The test case doesn't seem realistic to me - I wouldn't expect to see a declaration with a high/low_pc (a declaration with code seems like a contradiction) - so maybe something more realistic would help explain/motivate the feature?

The actual case seen in the wild didn't have low_pc/high_pc, it's true, but it did have DW_AT_linkage_name, which Bloaty considers equivalent (Bloaty will look up the low/high_pc in the symbol table).

For the test case I decided to use low/high_pc instead to limit the test case to pure DWARF only, instead of exercising a DWARF/ELF interaction. That said, I could change it to use DW_AT_linkage_name instead, I agree it could be more realistic.

haberman · 2021-08-05T23:45:05Z

@dwblaikie back to you, I updated the test to use linkage_name instead of low_pc/high_pc.

dwblaikie · 2021-08-05T23:45:43Z

What does Bloaty do with the information if it isn't a declaration?

For any DIE that is not a declaration (and is not stripped), Bloaty will look for any information it can find in the DIE that will associate a region of the binary with the current compileunit. That includes

DW_AT_linkage_name: We will look up the symbol and attribute its range to this CU

DW_AT_low_pc/DW_AT_high_pc: We will attribute the given address range to this CU

DW_AT_ranges/DW_AT_start_scope: We will attribute the ranges given in either kind of range list to this CU

DW_AT_location: We will try to parse the location expression and attribute the target to this CU

Are the first and last cases needed, or could low/high/ranges be adequate? Then maybe you wouldn't need to check for declaration?

The test case doesn't seem realistic to me - I wouldn't expect to see a declaration with a high/low_pc (a declaration with code seems like a contradiction) - so maybe something more realistic would help explain/motivate the feature?

The actual case seen in the wild didn't have low_pc/high_pc, it's true, but it did have DW_AT_linkage_name, which Bloaty considers equivalent (Bloaty will look up the low/high_pc in the symbol table).

Ah, yeah, maybe the more suitable filter would be to only use the low/high/ranges data to attach a given subprogram to .text code?

haberman · 2021-08-06T01:00:39Z

Are the first and last cases needed, or could low/high/ranges be adequate? Then maybe you wouldn't need to check for declaration?

This is tricky; I added DW_AT_linkage_name after Bloaty already supported low_pc/high_pc, but I unfortunately didn't document the specific cases where I encountered DIEs with linkage_name but not low/high_pc.

Maybe I should try to determine whether there are any compilers today that would produce DIEs of this sort. Unfortunately it's hard to prove a negative.

haberman · 2021-08-08T00:24:04Z

Are the first and last cases needed, or could low/high/ranges be adequate? Then maybe you wouldn't need to check for declaration?

Here is an example I found where a variable was discoverable by DW_AT_linkage_name and DW_AT_location but not low/hi_pc (probably because it's a variable and not a function:

0x00053940: DW_TAG_compile_unit
              DW_AT_producer    ("clang version google3-trunk (0ad1d9fdf22dad41312e02b8bc990bf58ce1744c)")
              DW_AT_language    (DW_LANG_C_plus_plus_14)
              DW_AT_name        ("third_party/llvm/llvm-project/libcxx/src/chrono.cpp")
              DW_AT_str_offsets_base    (0x00006460)
              DW_AT_stmt_list   (0x00653598)
              DW_AT_comp_dir    ("/proc/self/cwd")
              DW_AT_GNU_pubnames        (true)
              DW_AT_low_pc      (0x0000000000000000)
              DW_AT_ranges      (indexed (0x2) rangelist = 0x0008c2e0
                 [0x00000000018658e0, 0x0000000001865940)
                 [0x0000000001865940, 0x000000000186595c)
                 [0x0000000001865960, 0x0000000001865968)
                 [0x0000000001865980, 0x00000000018659c7))
              DW_AT_addr_base   (0x0008ce60)
              DW_AT_rnglists_base       (0x0008c2c2)

0x0005395f:   DW_TAG_variable
                DW_AT_specification     (0x00053975 "is_steady")
                DW_AT_location  (DW_OP_addrx 0x0)
                DW_AT_linkage_name      ("_ZNSt3__u6chrono12system_clock9is_steadyE")

Here DW_AT_location and DW_AT_linkage_name seem to be giving true/reliable information that the is_steady variable belongs to the source file third_party/llvm/llvm-project/libcxx/src/chrono.cpp. This info doesn't seem to be available from any other attribute.

Here is another example where the DIE gives us DW_AT_location (but not DW_AT_linkage_name):

0x0000000b: DW_TAG_compile_unit
              DW_AT_stmt_list [DW_FORM_data4]   (0x00000000)
              DW_AT_low_pc [DW_FORM_addr]       (0x0000000000d78f80)
              DW_AT_high_pc [DW_FORM_addr]      (0x0000000000d78faa)
              DW_AT_name [DW_FORM_string]       ("../sysdeps/x86_64/start.S")
              DW_AT_comp_dir [DW_FORM_string]   ("/usr/grte/v4/debug-src/src/csu")
              DW_AT_producer [DW_FORM_string]   ("GNU AS 2.24")
              DW_AT_language [DW_FORM_data2]    (DW_LANG_Mips_Assembler)
0x00000067: Compile Unit: length = 0x0000007a, format = DWARF32, version = 0x0004, abbr_offset = 0x0014, addr_size = 0x08 (next unit at 0x000000e5)

[...]

0x000000ca:   DW_TAG_variable
                DW_AT_name [DW_FORM_strp]       ("_IO_stdin_used")
                DW_AT_decl_file [DW_FORM_data1] ("/usr/grte/v4/debug-src/src/csu/init.c")
                DW_AT_decl_line [DW_FORM_data1] (24)
                DW_AT_type [DW_FORM_ref4]       (0x000000df "const int")
                DW_AT_external [DW_FORM_flag_present]   (true)
                DW_AT_location [DW_FORM_exprloc]        (DW_OP_addr 0x21ffa0)

However I also discovered cases where Bloaty was being misled, and the DW_AT_linkage_name was making the results worse. Here is one example, which Bloaty should ignore because it is DW_AT_inline but is not ignoring:

0x00041aa4:       DW_TAG_subprogram
                    DW_AT_linkage_name  ("_ZNSt3__u7__sort3IRNS_6__lessIccEEPcEEjT0_S5_S5_T_")
                    DW_AT_name  ("__sort3<std::__less<char> &, char *>")
                    DW_AT_decl_file     ("/proc/self/cwd/third_party/llvm/llvm-project/libcxx/include/__algorithm/sort.h")
                    DW_AT_decl_line     (35)
                    DW_AT_type  (0x000529ad "unsigned int")
                    DW_AT_external      (true)
                    DW_AT_inline        (DW_INL_inlined)

After some experimentation, I'm inclined to think that:

Bloaty should pay attention to DW_AT_location, since this appears to provide useful and reliable information about the addresses of variables and which CU they belong to.
Bloaty should not pay attention to DW_AT_linkage_name. I could not find any examples where this provided useful information that was not also provided by DW_AT_location.

Does that sound reasonable?

dwblaikie · 2021-08-08T15:32:15Z

Are the first and last cases needed, or could low/high/ranges be adequate? Then maybe you wouldn't need to check for declaration?

Here is an example I found where a variable was discoverable by DW_AT_linkage_name and DW_AT_location but not low/hi_pc (probably because it's a variable and not a function:
0x00053940: DW_TAG_compile_unit
              DW_AT_producer    ("clang version google3-trunk (0ad1d9fdf22dad41312e02b8bc990bf58ce1744c)")
              DW_AT_language    (DW_LANG_C_plus_plus_14)
              DW_AT_name        ("third_party/llvm/llvm-project/libcxx/src/chrono.cpp")
              DW_AT_str_offsets_base    (0x00006460)
              DW_AT_stmt_list   (0x00653598)
              DW_AT_comp_dir    ("/proc/self/cwd")
              DW_AT_GNU_pubnames        (true)
              DW_AT_low_pc      (0x0000000000000000)
              DW_AT_ranges      (indexed (0x2) rangelist = 0x0008c2e0
                 [0x00000000018658e0, 0x0000000001865940)
                 [0x0000000001865940, 0x000000000186595c)
                 [0x0000000001865960, 0x0000000001865968)
                 [0x0000000001865980, 0x00000000018659c7))
              DW_AT_addr_base   (0x0008ce60)
              DW_AT_rnglists_base       (0x0008c2c2)

0x0005395f:   DW_TAG_variable
                DW_AT_specification     (0x00053975 "is_steady")
                DW_AT_location  (DW_OP_addrx 0x0)
                DW_AT_linkage_name      ("_ZNSt3__u6chrono12system_clock9is_steadyE")
Here DW_AT_location and DW_AT_linkage_name seem to be giving true/reliable information that the is_steady variable belongs to the source file third_party/llvm/llvm-project/libcxx/src/chrono.cpp. This info doesn't seem to be available from any other attribute.

Here is another example where the DIE gives us DW_AT_location (but not DW_AT_linkage_name):
0x0000000b: DW_TAG_compile_unit
              DW_AT_stmt_list [DW_FORM_data4]   (0x00000000)
              DW_AT_low_pc [DW_FORM_addr]       (0x0000000000d78f80)
              DW_AT_high_pc [DW_FORM_addr]      (0x0000000000d78faa)
              DW_AT_name [DW_FORM_string]       ("../sysdeps/x86_64/start.S")
              DW_AT_comp_dir [DW_FORM_string]   ("/usr/grte/v4/debug-src/src/csu")
              DW_AT_producer [DW_FORM_string]   ("GNU AS 2.24")
              DW_AT_language [DW_FORM_data2]    (DW_LANG_Mips_Assembler)
0x00000067: Compile Unit: length = 0x0000007a, format = DWARF32, version = 0x0004, abbr_offset = 0x0014, addr_size = 0x08 (next unit at 0x000000e5)

[...]

0x000000ca:   DW_TAG_variable
                DW_AT_name [DW_FORM_strp]       ("_IO_stdin_used")
                DW_AT_decl_file [DW_FORM_data1] ("/usr/grte/v4/debug-src/src/csu/init.c")
                DW_AT_decl_line [DW_FORM_data1] (24)
                DW_AT_type [DW_FORM_ref4]       (0x000000df "const int")
                DW_AT_external [DW_FORM_flag_present]   (true)
                DW_AT_location [DW_FORM_exprloc]        (DW_OP_addr 0x21ffa0)
However I also discovered cases where Bloaty was being misled, and the DW_AT_linkage_name was making the results worse. Here is one example, which Bloaty should ignore because it is DW_AT_inline but is not ignoring:
0x00041aa4:       DW_TAG_subprogram
                    DW_AT_linkage_name  ("_ZNSt3__u7__sort3IRNS_6__lessIccEEPcEEjT0_S5_S5_T_")
                    DW_AT_name  ("__sort3<std::__less<char> &, char *>")
                    DW_AT_decl_file     ("/proc/self/cwd/third_party/llvm/llvm-project/libcxx/include/__algorithm/sort.h")
                    DW_AT_decl_line     (35)
                    DW_AT_type  (0x000529ad "unsigned int")
                    DW_AT_external      (true)
                    DW_AT_inline        (DW_INL_inlined)
After some experimentation, I'm inclined to think that:

Bloaty should pay attention to DW_AT_location, since this appears to provide useful and reliable information about the addresses of variables and which CU they belong to.

Bloaty should not pay attention to DW_AT_linkage_name. I could not find any examples where this provided useful information that was not also provided by DW_AT_location.

Does that sound reasonable?

Hmm, there's a case I hadn't thought of that maybe demonstrates the need for the DW_AT_declaration checking: variables whose actual object definitions have been optimized away, but where the variable is still described.

This comes up for any variable, really - easily with static or inline variables (where the normal compilation stage can see all uses of this copy of the variable (ie: like an inline function, if all calls/uses are inlined then the definition can be dropped), but with LTO can happen to any variable):

static int i = 3;
int main() {
  return i;
}

Compiled with optimizations, LLVM generates this DWARF for 'i':

0x0000002a:   DW_TAG_variable
                DW_AT_name      ("i")
                DW_AT_type      (0x00000039 "int")
                DW_AT_decl_file ("/usr/local/google/home/blaikie/dev/scratch/def.cpp")
                DW_AT_decl_line (1)
                DW_AT_linkage_name      ("_ZL1i")

(unoptimized, this DW_TAG_variable DIE does include a DW_AT_location)

So, yeah, maybe the original direction of this patch is good - checking for DW_AT_declaration. I guess it depends what you're using this information for? What observations are built from this? (what does Bloaty conclude/say/report if it doesn't ignore a declaration DIE?)

haberman · 2021-08-08T16:49:03Z

I guess it depends what you're using this information for? What observations are built from this? (what does Bloaty conclude/say/report if it doesn't ignore a declaration DIE?)

Ultimately the whole goal of this exercise is to build a memory map for the binary. The memory map is trying to determine, for every byte of the file (or byte of the address space), what compileunit emitted it.

In essence, we are trying to build a linker map after-the-fact, using only the information that is still present in the final binary. For each DIE in a CU, we attribute whatever regions we can find to the enclosing CU.

Anything that was optimized out of the final binary we don't care about, because it doesn't have a footprint in the final binary. So it doesn't need to be part of the map.

Compiled with optimizations, LLVM generates this DWARF for 'i':
0x0000002a:   DW_TAG_variable
                DW_AT_name      ("i")
                DW_AT_type      (0x00000039 "int")
                DW_AT_decl_file ("/usr/local/google/home/blaikie/dev/scratch/def.cpp")
                DW_AT_decl_line (1)
                DW_AT_linkage_name      ("_ZL1i")
(unoptimized, this DW_TAG_variable DIE does include a DW_AT_location)

It seems like what I proposed would handle this case successfully?

Unoptimized, the DIE has DW_AT_location, so we'll find it if we are paying attention to DW_AT_location.
Optimized, the DIE does not have low/hi_pc, nor DW_AT_location, so we will ignore it. But that seems appropriate, since it has no footprint in the final binary.

haberman · 2021-08-08T17:10:10Z

I guess it depends what you're using this information for? What observations are built from this? (what does Bloaty conclude/say/report if it doesn't ignore a declaration DIE?)

For the second part of the question: I'll use the example from this PR's test case:

# CHECK: VM MAP:
# CHECK: 0000-1000              4096             [-- Nothing mapped --]
# CHECK: 1000-1010                16             foo.c
# CHECK: 1010-1020                16             bar.c

This will lead Bloaty to report something like:

     VM SIZE   
 --------------- 
 50.0%      16    foo.c
 50.0%      16    bar.c
100.0%      32    TOTAL

Whereas if it did not ignore the declaration, it would have reported:

     VM SIZE   
 --------------- 
 50.0%      32    foo.c
100.0%      32    TOTAL

Because now bar_func is getting attributed to foo.c instead of bar.c.

Since bar_func is actually coming from the bar.c compileunit, the first is correct and the second is incorrect. This is equivalent to the bug reported by a user here: #236

dwblaikie · 2021-08-08T18:02:21Z

I guess it depends what you're using this information for? What observations are built from this? (what does Bloaty conclude/say/report if it doesn't ignore a declaration DIE?)

For the second part of the question: I'll use the example from this PR's test case:
# CHECK: VM MAP:
# CHECK: 0000-1000              4096             [-- Nothing mapped --]
# CHECK: 1000-1010                16             foo.c
# CHECK: 1010-1020                16             bar.c
This will lead Bloaty to report something like:
     VM SIZE   
 --------------- 
 50.0%      16    foo.c
 50.0%      16    bar.c
100.0%      32    TOTAL
Whereas if it did not ignore the declaration, it would have reported:
     VM SIZE   
 --------------- 
 50.0%      32    foo.c
100.0%      32    TOTAL

Are these meant to be attributed by CU (ie: to know which source file ultimately used the entity and caused it to be pulled in), or by original source (ie: to know where in the source code the entity is written, to go and make changes to it for instance)? Because in general I'd expect the declaration to have source/line numbers on it - so despite bar.c not being built with debug info (I assume that's the scenario here - otherwise you'd get the entity from bar.c's CU, right?)

Because now bar_func is getting attributed to foo.c instead of bar.c.

Since bar_func is actually coming from the bar.c compileunit, the first is correct and the second is incorrect. This is equivalent to the bug reported by a user here: #236

Ah, fair enough. Maybe the bug/issue then is in using DW_AT_linkage_name to find symbols/attribute entities? If the DWARF producer knows the entity, it'll use high/low/ranges/location to describe it.

But also, yeah, attributing anything that's only a declaration to its CU would be wrong too.

haberman · 2021-08-08T19:54:41Z

Are these meant to be attributed by CU (ie: to know which source file ultimately used the entity and caused it to be pulled in), or by original source (ie: to know where in the source code the entity is written, to go and make changes to it for instance)?

The first. This is specifically for the compileunits data source.

Because in general I'd expect the declaration to have source/line numbers on it - so despite bar.c not being built with debug info (I assume that's the scenario here - otherwise you'd get the entity from bar.c's CU, right?)

They are both built with debug info (see the .test file in this PR, it has the full yaml2obj input of all the test data).

We do get the entity from bar.c's CU. But since it's also present in foo.c's CU, and because we are scanning foo.c's CU first, foo.c wins and the DIE from bar.c "loses the race", so to speak. (unless we ignore DW_AT_declaration).

Ah, fair enough. Maybe the bug/issue then is in using DW_AT_linkage_name to find symbols/attribute entities? If the DWARF producer knows the entity, it'll use high/low/ranges/location to describe it.

Yep, agreed. That is what I concluded above:

After some experimentation, I'm inclined to think that:

Bloaty should pay attention to DW_AT_location, since this appears to provide useful and reliable information about the addresses of variables and which CU they belong to.

Bloaty should not pay attention to DW_AT_linkage_name. I could not find any examples where this provided useful information that was not also provided by DW_AT_location.

It's a slight bummer that DW_AT_location doesn't also have size info. Right now Bloaty tries to determine the size by looking for a symbol in the symbol table with the same address, and will ignore the DW_AT_location if it can't find this. This seems to work reasonably well. Do you have any other ideas where we could get this information from?

dwblaikie · 2021-08-08T20:40:12Z

Are these meant to be attributed by CU (ie: to know which source file ultimately used the entity and caused it to be pulled in), or by original source (ie: to know where in the source code the entity is written, to go and make changes to it for instance)?

The first. This is specifically for the compileunits data source.

Because in general I'd expect the declaration to have source/line numbers on it - so despite bar.c not being built with debug info (I assume that's the scenario here - otherwise you'd get the entity from bar.c's CU, right?)

They are both built with debug info (see the .test file in this PR, it has the full yaml2obj input of all the test data).

We do get the entity from bar.c's CU. But since it's also present in foo.c's CU, and because we are scanning foo.c's CU first, foo.c wins and the DIE from bar.c "loses the race", so to speak. (unless we ignore DW_AT_declaration).

Ah, fair enough. Maybe the bug/issue then is in using DW_AT_linkage_name to find symbols/attribute entities? If the DWARF producer knows the entity, it'll use high/low/ranges/location to describe it.

Yep, agreed. That is what I concluded above:

After some experimentation, I'm inclined to think that:

Bloaty should pay attention to DW_AT_location, since this appears to provide useful and reliable information about the addresses of variables and which CU they belong to.

Bloaty should not pay attention to DW_AT_linkage_name. I could not find any examples where this provided useful information that was not also provided by DW_AT_location.

It's a slight bummer that DW_AT_location doesn't also have size info. Right now Bloaty tries to determine the size by looking for a symbol in the symbol table with the same address, and will ignore the DW_AT_location if it can't find this. This seems to work reasonably well. Do you have any other ideas where we could get this information from?

You could still use the linkage_name only on entities that already have a location to find the size - but the DWARF way would be to look at the DW_AT_type of the DW_TAG_variable and then look at the type's DW_AT_byte_size. (though that doesn't cover all cases - full generality requires computing the size in some cases, for instance an array type doesn't specify its size - you'd have to look at the dimensions of the array, and the size of the underlying type and then compute the size from those values (& for a pointer type I don't think the size is in the DWARF DIEs, you'd have to get the address size of the target (addrSize in the DWARF unit header I /think/ should be enough for that))

haberman · 2021-08-08T23:41:55Z

Ok PTAL, I changed the PR to both:

Ignore DIEs that are declarations
Stop respecting linkage_name

There is still work to do, but this seems like a good stopping point for now. It should be enough to fix the issue #236 which motivated this PR to begin with.

As follow-up work I want to:

Add tests for low/high_pc that use DW_FORM_addrx*
Add tests for DW_AT_location, DW_AT_ranges, and more.
Implement support for DW_OP_addrx and add tests for it (right now we only support DW_OP_addr)
Possibly implement support for getting size information from DW_AT_type instead of the symbol table.

[dwarf] Ignore DIEs where DW_AT_declaration is set.

f70649c

haberman mentioned this pull request Aug 5, 2021

DWARF > 3 seems to misattribute symbols to wrong compile units #236

Closed

dwblaikie reviewed Aug 5, 2021

View reviewed changes

src/dwarf.cc Outdated Show resolved Hide resolved

src/dwarf.cc Show resolved Hide resolved

haberman added 2 commits August 5, 2021 15:31

Checked DIEs for valid low_pc up front.

07ae17e

Change the test case to use linkage_name instead of high/low_pc.

c952309

haberman added 2 commits August 8, 2021 16:25

Merge branch 'master' into ignore-declarations

58b1038

Added a few more tests for pcpair.

529516f

haberman changed the title ~~[dwarf] Ignore DIEs where DW_AT_declaration is set.~~ [dwarf] Ignore DW_AT_linkage_name and DIEs with DW_AT_declaration Aug 8, 2021

haberman merged commit e72c26d into google:master Aug 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[dwarf] Ignore DW_AT_linkage_name and DIEs with DW_AT_declaration #268

[dwarf] Ignore DW_AT_linkage_name and DIEs with DW_AT_declaration #268

haberman commented Aug 5, 2021 •

edited

dwblaikie left a comment

haberman commented Aug 5, 2021

haberman commented Aug 5, 2021

dwblaikie commented Aug 5, 2021

haberman commented Aug 6, 2021

haberman commented Aug 8, 2021

dwblaikie commented Aug 8, 2021

haberman commented Aug 8, 2021

haberman commented Aug 8, 2021

dwblaikie commented Aug 8, 2021

haberman commented Aug 8, 2021

dwblaikie commented Aug 8, 2021

haberman commented Aug 8, 2021

[dwarf] Ignore DW_AT_linkage_name and DIEs with DW_AT_declaration #268

[dwarf] Ignore DW_AT_linkage_name and DIEs with DW_AT_declaration #268

Conversation

haberman commented Aug 5, 2021 • edited

dwblaikie left a comment

Choose a reason for hiding this comment

haberman commented Aug 5, 2021

haberman commented Aug 5, 2021

dwblaikie commented Aug 5, 2021

haberman commented Aug 6, 2021

haberman commented Aug 8, 2021

dwblaikie commented Aug 8, 2021

haberman commented Aug 8, 2021

haberman commented Aug 8, 2021

dwblaikie commented Aug 8, 2021

haberman commented Aug 8, 2021

dwblaikie commented Aug 8, 2021

haberman commented Aug 8, 2021

haberman commented Aug 5, 2021 •

edited