Orphaned DWARF for static local of inlined func

[pogo59]

Bugzilla Link	30637
Version	3.9
OS	Linux
CC	@dwblaikie,@ellishg,@hfinkel,@wjristow,@yuanfang-chen

Extended Description

Debug info for this case has regressed in 3.9.

__attribute__((always_inline)) static int inlined(int i) {
  static int a = 0;
  a += i;
  return a;
}
int main() {
  return inlined(4);
}

In 3.8, we get this:

<1><2a>: Abbrev Number: 2 (DW_TAG_subprogram)
    <2b>   DW_AT_abstract_origin: <0x4c>
 <2><2f>: Abbrev Number: 3 (DW_TAG_variable)
    <30>   DW_AT_name        : (indirect string, offset: 0x5f): a
    <34>   DW_AT_type        : <0x45>
    <38>   DW_AT_decl_file   : 1
    <39>   DW_AT_decl_line   : 2
    <3a>   DW_AT_location    : 9 byte block: 3 0 0 0 0 0 0 0 0 	(DW_OP_addr: 0)
 
 <1><4c>: Abbrev Number: 5 (DW_TAG_subprogram)
    <4d>   DW_AT_linkage_name: (indirect string, offset: 0x65): _ZL7inlinedi
    <51>   DW_AT_name        : (indirect string, offset: 0x72): inlined
    <55>   DW_AT_decl_file   : 1
    <56>   DW_AT_decl_line   : 1
    <57>   DW_AT_type        : <0x45>
    <5b>   DW_AT_inline      : 1	(inlined)
 <2><5c>: Abbrev Number: 6 (DW_TAG_formal_parameter)
    <5d>   DW_AT_name        : (indirect string, offset: 0x7a): i
    <61>   DW_AT_decl_file   : 1
    <62>   DW_AT_decl_line   : 1
    <63>   DW_AT_type        : <0x45>

I'd rather see the static variable under the abstract origin DIE,
instead of separate, but at least the subprogram it's in points to the
abstract origin. If you work hard enough you can tie it all together.

But in 3.9, that abstract_origin link is gone:

 <1><2a>: Abbrev Number: 2 (DW_TAG_subprogram)
 <2><2b>: Abbrev Number: 3 (DW_TAG_variable)
    <2c>   DW_AT_name        : (indirect string, offset: 0x5f): a
    <30>   DW_AT_type        : <0x41>
    <34>   DW_AT_decl_file   : 1
    <35>   DW_AT_decl_line   : 2
    <36>   DW_AT_location    : 9 byte block: 3 0 0 0 0 0 0 0 0 	(DW_OP_addr: 0)

Now it's impossible to associate the static variable with the
subprogram where it belongs.

[dwblaikie]

As for a fix, looks like GCC always puts static variables in the abstract subprogram definition, not the concrete one - if Clang did that it would address this issue (because we wouldn't try to create the concrete subprogram just to put the variable there) & I think makes more sense DWARF-wise, a static variable's description is valid for all instantiations (inline and out of line) of the function so should be shared by placing it in the abstract subprogram definition.

(placing it only in the concrete definition could make name lookup difficult for the inline function - the DWARF consumer would have to navigate to the abstract definition, then search around for a matching concrete definition (there's no pointer/link to follow, only the re

[dwblaikie]

(previous comment inspired by llvm/llvm-bugzilla-archive#51545 )

[ellishg]

A test case
Here's a test case I was playing with for llvm/test/DebugInfo/Generic

[ellishg]

*** Bug llvm/llvm-bugzilla-archive#51545 has been marked as a duplicate of this bug. ***

[ellishg]

As for a fix, looks like GCC always puts static variables in the abstract
subprogram definition, not the concrete one - if Clang did that it would
address this issue (because we wouldn't try to create the concrete
subprogram just to put the variable there) & I think makes more sense
DWARF-wise, a static variable's description is valid for all instantiations
(inline and out of line) of the function so should be shared by placing it
in the abstract subprogram definition.

(placing it only in the concrete definition could make name lookup difficult
for the inline function - the DWARF consumer would have to navigate to the
abstract definition, then search around for a matching concrete definition
(there's no pointer/link to follow, only the re

The problem I was running into is that the global variable DIEs are all created before any abstract subprogram DIEs are created. I've considered delaying the creation of static global DIEs, but I'm not familiar enough to know if this has other unintended side effects.

[dwblaikie]

As for a fix, looks like GCC always puts static variables in the abstract
subprogram definition, not the concrete one - if Clang did that it would
address this issue (because we wouldn't try to create the concrete
subprogram just to put the variable there) & I think makes more sense
DWARF-wise, a static variable's description is valid for all instantiations
(inline and out of line) of the function so should be shared by placing it
in the abstract subprogram definition.

(placing it only in the concrete definition could make name lookup difficult
for the inline function - the DWARF consumer would have to navigate to the
abstract definition, then search around for a matching concrete definition
(there's no pointer/link to follow, only the re

The problem I was running into is that the global variable DIEs are all
created before any abstract subprogram DIEs are created. I've considered
delaying the creation of static global DIEs, but I'm not familiar enough to
know if this has other unintended side effects.

ah, fair. Yeah - currently the abstract definitions are probably created only once & everything is known up-front. So this would require maybe deferring creating static variable DIEs until after functions. Currently it's done in beginModule (the loop over CUNode->getGlobalVariables()) and would need to move to finalizeModuleInfo.

That'll change the order of DIEs in lots of test cases, but they should be benign. (*there's always the possibility of bugs, other knockon/unintended effects, etc)

[ellishg]

As for a fix, looks like GCC always puts static variables in the abstract
subprogram definition, not the concrete one - if Clang did that it would
address this issue (because we wouldn't try to create the concrete
subprogram just to put the variable there) & I think makes more sense
DWARF-wise, a static variable's description is valid for all instantiations
(inline and out of line) of the function so should be shared by placing it
in the abstract subprogram definition.

(placing it only in the concrete definition could make name lookup difficult
for the inline function - the DWARF consumer would have to navigate to the
abstract definition, then search around for a matching concrete definition
(there's no pointer/link to follow, only the re

The problem I was running into is that the global variable DIEs are all
created before any abstract subprogram DIEs are created. I've considered
delaying the creation of static global DIEs, but I'm not familiar enough to
know if this has other unintended side effects.

ah, fair. Yeah - currently the abstract definitions are probably created
only once & everything is known up-front. So this would require maybe
deferring creating static variable DIEs until after functions. Currently
it's done in beginModule (the loop over CUNode->getGlobalVariables()) and
would need to move to finalizeModuleInfo.

That'll change the order of DIEs in lots of test cases, but they should be
benign. (*there's always the possibility of bugs, other knockon/unintended
effects, etc)

Sounds good! I'll try to put together a diff.

[ellishg]

Fixed in https://reviews.llvm.org/D112337

[pogo59]

Fixed in https://reviews.llvm.org/D112337

Reverted by https://reviews.llvm.org/rGf0d997c4723214f5bc098b0acd2e61f3215d4a49

We had a local test break due to the patch; the test was verifying a local
hack to fix the problem, so it might have been just because of that, but I
see that the local test didn't match the test case I attached previously.
I'll get the other one attached soon.

[pogo59]

Second test case
I didn't get a chance to investigate why this failed with your patch,
it might have just been a bad interaction between yours and our hack;
but I think it's enough different from the other test case to be worth
providing. Not saying your patch has to pass this exact test, but the
use-case needs to have reasonable DWARF.

[ellishg]

Created attachment 25438 [details]
Second test case

I didn't get a chance to investigate why this failed with your patch,
it might have just been a bad interaction between yours and our hack;
but I think it's enough different from the other test case to be worth
providing. Not saying your patch has to pass this exact test, but the
use-case needs to have reasonable DWARF.

Hi Paul,
Thanks for the test case! I just published a new patch https://reviews.llvm.org/D113736 for review and I think it solves the problem a bit different than your test case. The function inlined() will be removed, so we shouldn't actually expect a concrete DIE for that function, we should expect only the abstract DIE. Rather than keeping the concrete DIE and adding the DW_AT_abstract_origin attribute, this patch places the static variable DIE in the abstract SP DIE. That way we can start at the DIE for main and follow the abstract origin to the abstract DIE for inlined() which contains the static variable DIE. Of course, there could be other ways to fix this problem so let me know what you think.

[ellishg]

mentioned in issue llvm/llvm-bugzilla-archive#51545

[ellishg]

This seems to be partially improved by 06a0ae6, but the issue still persists.