[PATCH] D109703: [DebugInfo] Fix scope for local static variables

[Kristina Bessonova via Phabricator via cfe-commits]

krisb abandoned this revision.
krisb added a comment.

Abandon in favor of https://reviews.llvm.org/D113741.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109703

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109703: [DebugInfo] Fix scope for local static variables

[Kristina Bessonova via Phabricator via cfe-commits]

krisb added a comment.

@ellis I'd appreciate if you go ahead and fix the issues with inlined 
functions. Sorry for interrupting you in D108492 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109703

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109703: [DebugInfo] Fix scope for local static variables

[Ellis Hoag via Phabricator via cfe-commits]

ellis added a comment.

In D109703#3018935 , @krisb wrote:

> @dblaikie yeah, the problem(s) seemed easier and smaller :(
>
> Basically, we have two issues with local scopes here:
> (1) function-scoped entities like static variables, type 
> definitions/typedefs, etc have incorrect (empty) parent DIE if the function 
> containing them was inlined. We do not consider abstract SP as a possible 
> parent DIE and try to create a new DIE for a function that doesn't exist. 
> @ellis is working on this issue in [0] (for static vars) and [1] (for 
> imported declarations).
> (2) the same entities (static local vars, typedefs, etc) that should be 
> scoped within a lexical block have a subprogram scope (in debug metadata) and 
> parent DIE (in DWARF) instead. This is the issue I'm trying to fix in this 
> patch, but for static variables only.
>
> As a side effect, this patch fixes the issue with inlined functions for 
> static vars (1) as well. But it seems the issues are not related and can be 
> fixed separately.
> And as now I've realized that static locals is not the only problem, this 
> patch should implement a more generic solution to cover other entities. So, 
> please, consider it as a WIP.

Hey @krisb I was under the impression that this patch would fix the static 
variable bug. Should I wait to see what this patch fixes before working on [0] 
and [1]?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109703

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109703: [DebugInfo] Fix scope for local static variables

[Kristina Bessonova via Phabricator via cfe-commits]

krisb added a comment.

@dblaikie yeah, the problem(s) seemed easier and smaller :(

Basically, we have two issues with local scopes here:
(1) function-scoped entities like static variables, type definitions/typedefs, 
etc have incorrect (empty) parent DIE if the function containing them was 
inlined. We do not consider abstract SP as a possible parent DIE and try to 
create a new DIE for a function that doesn't exist. @ellis is working on this 
issue in [0] (for static vars) and [1] (for imported declarations).
(2) the same entities (static local vars, typedefs, etc) that should be scoped 
within a lexical block have a subprogram scope (in debug metadata) and parent 
DIE (in DWARF) instead. This is the issue I'm trying to fix in this patch, but 
for static variables only.

As a side effect, this patch fixes the issue with inlined functions for static 
vars (1) as well. But it seems the issues are not related and can be fixed 
separately.
And as now I've realized that static locals is not the only problem, this patch 
should implement a more generic solution to cover other entities. So, please, 
consider it as a WIP.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109703

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109703: [DebugInfo] Fix scope for local static variables

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

There have been a few different patches going around related to mis-scoped 
things - imported entities and types, maybe other things too? (static 
variables? Or has that already been fixed now) Perhaps some summary of the 
state of all these patches sent to llvm-dev/linked from the various reviews 
would be helpful? It'd be good to know if/how all these patches are hopefully 
aiming for some unified solution.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109703

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109703: [DebugInfo] Fix scope for local static variables

[Ellis Hoag via Phabricator via cfe-commits]

ellis added a comment.

In D109703#2998350 , @krisb wrote:

> But it seems imported declarations are broken not just for static locals, but 
> for all inlined entities, for example
>
>   namespace ns {
>   inline __attribute__((always_inline))
>   int foo() {
> int a = 42; 
> return a;
>   }
>   }
>   
>   int main() {
> using ns::foo;
> return foo();
>   }
>
> produces (with or w/o this patch) imported declaration for `foo()` that 
> refers to an empty subprogram:
>
>   x002a:   DW_TAG_namespace
>   DW_AT_name  ("ns")
>   
>   0x002f: DW_TAG_subprogram
> DW_AT_name  ("foo")
> DW_AT_inline  (DW_INL_inlined)
>   
>   0x003f:   DW_TAG_variable
>   DW_AT_name  ("a")
>   
>   0x004a:   NULL
>   
>   0x004b: DW_TAG_subprogram
>   
>   0x004c: NULL
>   
>   0x0054:   DW_TAG_subprogram
>   DW_AT_name  ("main")
>   
>   0x006d: DW_TAG_imported_declaration
> DW_AT_import  (0x004b)
>
> while it should point to `0x002f`.

I've looked into this and realized that clang correctly emits a 
`DW_TAG_inlined_subroutine` for foo so the variables here are actually ok. The 
`DW_TAG_imported_declaration` tag is incorrect though, and I have a fix in 
https://reviews.llvm.org/D110294.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109703

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109703: [DebugInfo] Fix scope for local static variables

[Ellis Hoag via Phabricator via cfe-commits]

ellis added a comment.

In D109703#2998350 , @krisb wrote:

> In D109703#2998155 , @ellis wrote:
>
>> I've added this to the added test case.
>>
>>   !18 = !DIImportedEntity(tag: DW_TAG_imported_declaration, scope: !4, 
>> entity: !19, line: 122)
>>   !19 = !DIGlobalVariable(name: "imported_static_var", scope: !6, file: !5, 
>> line: 3, type: !10, isLocal: false, isDefinition: true)
>
> Thank you for pointing to this! Just in case, do you have a C++ example, that 
> reproduces the issue with imported static local declaration? I'm asking, 
> cause in your example `DW_TAG_imported_declaration` has CU as a scope, while 
> `DIGlobalVariable` is function-scoped. This looks a bit strange. I mean, 
> `constructImportedEntityDIE()` called from `DwarfDebug::beginModule()` skips 
> all local-scoped entities, and unless I'm missing something static locals 
> shouldn't go here. Another call for  `constructImportedEntityDIE()` is from 
> `DwarfCompileUnit::createScopeChildrenDIE` where I'd not expect any issue 
> with parents.

No, I designed the test to hit that code path, but I should have found a C++ 
example to back it up. Thanks for looking into this!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109703

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109703: [DebugInfo] Fix scope for local static variables

[Kristina Bessonova via Phabricator via cfe-commits]

krisb added a comment.

In D109703#2998155 , @ellis wrote:

> I've added this to the added test case.
>
>   !18 = !DIImportedEntity(tag: DW_TAG_imported_declaration, scope: !4, 
> entity: !19, line: 122)
>   !19 = !DIGlobalVariable(name: "imported_static_var", scope: !6, file: !5, 
> line: 3, type: !10, isLocal: false, isDefinition: true)

Thank you for pointing to this! Just in case, do you have a C++ example, that 
reproduces the issue with imported static local declaration? I'm asking, cause 
in your example `DW_TAG_imported_declaration` has CU as a scope, while 
`DIGlobalVariable` is function-scoped. This looks a bit strange. I mean, 
`constructImportedEntityDIE()` called from `DwarfDebug::beginModule()` skips 
all local-scoped entities, and unless I'm missing something static locals 
shouldn't go here. Another call for  `constructImportedEntityDIE()` is from 
`DwarfCompileUnit::createScopeChildrenDIE` where I'd not expect any issue with 
parents.

But it seems imported declarations are broken not just for static locals, but 
for all inlined entities, for example

  namespace ns {
  inline __attribute__((always_inline))
  int foo() {
int a = 42; 
return a;
  }
  }
  
  int main() {
using ns::foo;
return foo();
  }

produces (with or w/o this patch) imported declaration for `foo()` that refers 
to an empty subprogram:

  x002a:   DW_TAG_namespace
  DW_AT_name  ("ns")
  
  0x002f: DW_TAG_subprogram
DW_AT_name  ("foo")
DW_AT_inline  (DW_INL_inlined)
  
  0x003f:   DW_TAG_variable
  DW_AT_name  ("a")
  
  0x004a:   NULL
  
  0x004b: DW_TAG_subprogram
  
  0x004c: NULL
  
  0x0054:   DW_TAG_subprogram
  DW_AT_name  ("main")
  
  0x006d: DW_TAG_imported_declaration
DW_AT_import  (0x004b)

while it should point to `0x002f`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109703

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109703: [DebugInfo] Fix scope for local static variables

[Ellis Hoag via Phabricator via cfe-commits]

ellis added a comment.

Thanks for working on this! `getOrCreateGlobalVariableDIE()` is also called in 
`constructImportedEntityDIE()` which means that there could be some globals 
without DIE parents. In D108492  I've added 
this to the added test case.

  !18 = !DIImportedEntity(tag: DW_TAG_imported_declaration, scope: !4, entity: 
!19, line: 122)
  !19 = !DIGlobalVariable(name: "imported_static_var", scope: !6, file: !5, 
line: 3, type: !10, isLocal: false, isDefinition: true)

But maybe we can fix this in another diff?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109703

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D109703: [DebugInfo] Fix scope for local static variables

[Kristina Bessonova via Phabricator via cfe-commits]

krisb created this revision.
krisb added reviewers: dblaikie, probinson.
Herald added a subscriber: hiraditya.
krisb requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

This fixes https://bugs.llvm.org/show_bug.cgi?id=44695 (and likely
https://bugs.llvm.org/show_bug.cgi?id=30637).

On clang side we need to place static locals defined within a bracketed
block into a correct scope. Since getContextDescriptor() does no access lexical
blocks, just pick the innermost lexical scope early.

On llvm side the patch proposes to delay emission of static locals until their
parent scopes are created (just like normal local variables).
In case of inlined function, static locals are created only for abstract
entities (as per suggestions from https://bugs.llvm.org/show_bug.cgi?id=30637).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D109703

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/test/CodeGen/debug-info-static.c
  llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
  llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.h
  llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
  llvm/test/DebugInfo/Generic/static-locals.ll
  llvm/test/DebugInfo/X86/gnu-public-names.ll

Index: llvm/test/DebugInfo/X86/gnu-public-names.ll
===
--- llvm/test/DebugInfo/X86/gnu-public-names.ll
+++ llvm/test/DebugInfo/X86/gnu-public-names.ll
@@ -137,19 +137,6 @@
 ; CHECK-NOT: {{DW_TAG|NULL}}
 ; CHECK: DW_AT_name {{.*}} "global_namespace_function"
 
-; CHECK: DW_TAG_subprogram
-; CHECK-NOT: {{DW_TAG|NULL}}
-; CHECK:   DW_AT_name {{.*}} "f3"
-; CHECK-NOT: {{DW_TAG|NULL}}
-; CHECK: [[F3_Z:.*]]:   DW_TAG_variable
-; CHECK-NOT: DW_TAG
-; CHECK: DW_AT_name {{.*}} "z"
-; CHECK-NOT: {{DW_TAG|NULL}}
-; CHECK: DW_AT_location
-; CHECK-NOT: {{DW_TAG|NULL}}
-; CHECK:   NULL
-; CHECK-NOT: {{DW_TAG|NULL}}
-
 ; CHECK: [[ANON:.*]]: DW_TAG_namespace
 ; CHECK-NOT:   DW_AT_name
 ; CHECK: [[ANON_I:.*]]: DW_TAG_variable
@@ -237,6 +224,19 @@
 ; CHECK: DW_AT_linkage_name
 ; CHECK-NOT: {{DW_TAG|NULL}}
 ; CHECK: DW_AT_name {{.*}} "global_function"
+; CHECK-NOT: {{DW_TAG|NULL}}
+
+; CHECK: DW_TAG_subprogram
+; CHECK-NOT: {{DW_TAG|NULL}}
+; CHECK:   DW_AT_name {{.*}} "f3"
+; CHECK-NOT: {{DW_TAG|NULL}}
+; CHECK: [[F3_Z:.*]]:   DW_TAG_variable
+; CHECK-NOT: DW_TAG
+; CHECK: DW_AT_name {{.*}} "z"
+; CHECK-NOT: {{DW_TAG|NULL}}
+; CHECK: DW_AT_location
+; CHECK-NOT: {{DW_TAG|NULL}}
+; CHECK:   NULL
 
 ; CHECK-LABEL: .debug_gnu_pubnames contents:
 ; CHECK-NEXT: length = {{.*}}, version = 0x0002, unit_offset = 0x, unit_size = {{.*}}
@@ -250,8 +250,8 @@
 ; comes out naturally from LLVM's implementation, so I'm OK with it for now. If
 ; it's demonstrated that this is a major size concern or degrades debug info
 ; consumer behavior, feel free to change it.
-; CHECK-NEXT:  [[F3_Z]] STATIC VARIABLE "f3::z"
 ; CHECK-NEXT:  [[ANON]] EXTERNAL TYPE "(anonymous namespace)"
+; CHECK-NEXT:  [[F3_Z]] STATIC VARIABLE "f3::z"
 ; CHECK-NEXT:  [[OUTER_ANON]] EXTERNAL TYPE "outer::(anonymous namespace)"
 ; CHECK-NEXT:  [[ANON_INNER_B]] STATIC VARIABLE "(anonymous namespace)::inner::b"
 ; CHECK-NEXT:  [[OUTER]] EXTERNAL TYPE "outer"
Index: llvm/test/DebugInfo/Generic/static-locals.ll
===
--- /dev/null
+++ llvm/test/DebugInfo/Generic/static-locals.ll
@@ -0,0 +1,204 @@
+; RUN: %llc_dwarf -O0 -filetype=obj < %s \
+; RUN:   | llvm-dwarfdump -debug-info -  \
+; RUN:   | FileCheck --implicit-check-not "{{DW_TAG|NULL}}" %s
+
+; Generated from:
+
+; static int global = 42;
+;
+; inline __attribute__((always_inline))
+; int inlined() {
+;   static int inlined_a = 1;
+;   if (global > 100) {
+; static int inlined_b = 2;
+; return inlined_b;
+;   }
+;   {
+; static int inlined_c = 3;
+; inlined_a = inlined_c;
+;   }
+;   return inlined_a;
+; }
+;
+; int foo() {
+;   static int a = 1;
+;   if (global < 100) {
+; static int b = 2;
+; return b;
+;   }
+;   {
+; static int c = 3;
+; a = c;
+;   }
+;   return a;
+; }
+;
+; int far() {
+;   return inlined();
+; }
+
+; CHECK: DW_TAG_compile_unit
+; CHECK: DW_TAG_variable
+; CHECK: DW_AT_name	("global")
+; CHECK: DW_TAG_base_type
+
+; foo().
+; CHECK: DW_TAG_subprogram
+; CHECK:   DW_AT_name	("foo")
+; CHECK:   DW_TAG_variable
+; CHECK: DW_AT_name	("a")
+; CHECK:   DW_TAG_lexical_block
+; CHECK: DW_TAG_variable
+; CHECK:   DW_AT_name	("b")
+; CHECK: NULL
+; CHECK:   DW_TAG_lexical_block
+; CHECK: DW_TAG_variable
+; CHECK:   DW_AT_name	("c")
+; CHECK:   NULL
+; CHECK: NULL
+
+; inlined().
+; CHECK: DW_TAG_subprogram
+; CHECK:   DW_AT_name  ("inlined")
+; CHECK:   DW_AT_inline  (DW_INL_inlined)
+; CHECK:   DW_TAG_variable
+; CHECK: DW_AT_name	("inlined_a")
+; CHECK:   DW_TAG_lexical_block
+; CHECK: DW_TAG_variable
+; CHECK:   DW_AT_name	("inlined_b")
+; CHECK: NULL
+;