[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D70696#1795427 , @yonghong-song 
wrote:

> @dblaikie The root cause has been identified by @rnk . A clang plugin is used 
> which requires the following patch:
>
>   
> https://github.com/llvm/llvm-project/commit/5128026467cbc17bfc796d94bc8e40e52a9b0752
>
> which has been merged. The original extern variable debug info type has been 
> relanded. So we are good. Thanks!


Ah, OK - would be good to follow-up here and mention the commit where this was 
relanded for the history/records. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70696



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

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

@dblaikie The root cause has been identified by @rnk . A clang plugin is used 
which requires the following patch:

  
https://github.com/llvm/llvm-project/commit/5128026467cbc17bfc796d94bc8e40e52a9b0752

which has been merged. The original extern variable debug info type has been 
relanded. So we are good. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70696



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

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D70696#1795302 , @yonghong-song 
wrote:

> > Could you link to particular bots that have logs showing they ran this 
> > test? (perhaps the logs have been retired by now, though - since this patch 
> > was reverted :/ - but then at least seeing which bots run BPF tests would 
> > be helpful)
>
> @dblaikie Actually, I do not have direct proof that this test is running on 
> buildbot. I only knew BPF is enabled by a lot of bots. I tried to search 
> through emails when I got from buildbot failures like:
>
>   clang-cmake-x86_64-avx2-linux
>   llvm-clang-win-x-aarch64
>   llvm-clang-win-x-armv7l
>   clang-hexagon-elf
>   clang-with-thin-lto-ubuntu
>   clang-ppc64be-linux-lnt
>   sanitizer-x86_64-linux-bootstrap-msan
>   clang-s390x-linux
>   sanitizer-x86_64-linux-fuzzer
>   ..
>   
>
> The contents of buildbot failures are all gone and not accessible now. Will 
> definitely check buildbot BPF coverage next time when I saw any issues.


Hmm, yeah, I thought some of the buildbots print out verbose test logs, showing 
every test execution (even the passing ones) , but I guess I'm mistaken.

Probably worth waiting a few days (maybe until after the holidays/new year) to 
see if @rnk has some more info that'd help debug the issue before committing 
again.

(or, given it's the holidasy and not many people are around, some "testing on 
the bots" might be acceptable (though I guess we already know the bots aren't 
failing... or you'd have got email, etc... so maybe that doesn't add much 
value?))


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70696



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

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

> Could you link to particular bots that have logs showing they ran this test? 
> (perhaps the logs have been retired by now, though - since this patch was 
> reverted :/ - but then at least seeing which bots run BPF tests would be 
> helpful)

@dblaikie Actually, I do not have direct proof that this test is running on 
buildbot. I only knew BPF is enabled by a lot of bots. I tried to search 
through emails when I got from buildbot failures like:

  clang-cmake-x86_64-avx2-linux
  llvm-clang-win-x-aarch64
  llvm-clang-win-x-armv7l
  clang-hexagon-elf
  clang-with-thin-lto-ubuntu
  clang-ppc64be-linux-lnt
  sanitizer-x86_64-linux-bootstrap-msan
  clang-s390x-linux
  sanitizer-x86_64-linux-fuzzer
  ..

The contents of buildbot failures are all gone and not accessible now. Will 
definitely check buildbot BPF coverage next time when I saw any issues.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70696



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

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D70696#1794440 , @rnk wrote:

> Three of these tests have been failing for me locally since this patch landed:
>
>   Failing Tests (3):
>   Clang :: CodeGen/debug-info-extern-basic.c
>   Clang :: CodeGen/debug-info-extern-duplicate.c
>   Clang :: CodeGen/debug-info-extern-multi.c 
>
>
> I suspect bots do not see these failures because the BPF target is 
> experimental. I would recommend that you:
>
> - do not use %clang in CodeGen tests, use %clang_cc1
> - find a way to make the test run without the BPF target
>
>   I'll take a look at fixing forward.


Got any more details on the nature of the failure/repro steps?

In D70696#1794480 , @yonghong-song 
wrote:

> @rnk I just submitted a patch https://reviews.llvm.org/D71818 to use 
> `%clang_cc1` instead of `%clang` in the test. Could you check whether this 
> fixed your issue or not?
>
> I cannot remove BPF target requirement as only BPF target can trigger debug 
> info generation for extern variables. This is by design as discussed in this 
> patch review due to concerns it may blow up debug info binary size for 
> existing applications. This may be relaxed in the future through some flags. 
> BPF is used on linux platform only.


If needed/useful, we could add a (just a cc1 flag, not a full driver flag) to 
support expliictly enabling this feature regardless of target, for testing 
purposes - but I'm not sure that's needed as yet.

> As far as I know, BPF is enabled for a lot of bots, ubuntu, x86, ppc, arm, 
> etc. So the coverage of this feature should be fine.

Could you link to particular bots that have logs showing they ran this test? 
(perhaps the logs have been retired by now, though - since this patch was 
reverted :/ - but then at least seeing which bots run BPF tests would be 
helpful)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70696



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

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

@rnk I just submitted a patch https://reviews.llvm.org/D71818 to use 
`%clang_cc1` instead of `%clang` in the test. Could you check whether this 
fixed your issue or not?

I cannot remove BPF target requirement as only BPF target can trigger debug 
info generation for extern variables. This is by design as discussed in this 
patch review due to concerns it may blow up debug info binary size for existing 
applications. This may be relaxed in the future through some flags. BPF is used 
on linux platform only.
As far as I know, BPF is enabled for a lot of bots, ubuntu, x86, ppc, arm, etc. 
So the coverage of this feature should be fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70696



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

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

I wasn't able to fix forward because the functionality you added doesn't seem 
to work, at least not for me, so I went ahead and reverted the change. I 
considered XFAILing the test, but I am concerned that may cause it to pass 
unexpectedly on some bot somewhere. In any case, please address the two 
requests I made (use cc1, avoid requiring BPF target in test) and reland.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70696



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

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

Three of these tests have been failing for me locally since this patch landed:

  Failing Tests (3):
  Clang :: CodeGen/debug-info-extern-basic.c
  Clang :: CodeGen/debug-info-extern-duplicate.c
  Clang :: CodeGen/debug-info-extern-multi.c 

I suspect bots do not see these failures because the BPF target is 
experimental. I would recommend that you:

- do not use %clang in CodeGen tests, use %clang_cc1
- find a way to make the test run without the BPF target

I'll take a look at fixing forward.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70696



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

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[Yonghong Song via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGd77ae1552fc2: [DebugInfo] Support to emit debugInfo for 
extern variables (authored by yonghong-song).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70696

Files:
  clang/include/clang/AST/ASTConsumer.h
  clang/include/clang/Basic/TargetInfo.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Basic/Targets/BPF.h
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/CodeGen/ModuleBuilder.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CodeGen/debug-info-extern-basic.c
  clang/test/CodeGen/debug-info-extern-duplicate.c
  clang/test/CodeGen/debug-info-extern-multi.c
  clang/test/CodeGen/debug-info-extern-unused.c
  llvm/include/llvm/IR/DIBuilder.h
  llvm/lib/IR/DIBuilder.cpp
  llvm/lib/IR/DebugInfo.cpp
  llvm/unittests/Transforms/Utils/CloningTest.cpp

Index: llvm/unittests/Transforms/Utils/CloningTest.cpp
===
--- llvm/unittests/Transforms/Utils/CloningTest.cpp
+++ llvm/unittests/Transforms/Utils/CloningTest.cpp
@@ -764,7 +764,7 @@
 
 DBuilder.createGlobalVariableExpression(
 Subprogram, "unattached", "unattached", File, 1,
-DBuilder.createNullPtrType(), false, Expr);
+DBuilder.createNullPtrType(), false, true, Expr);
 
 auto *Entry = BasicBlock::Create(C, "", F);
 IBuilder.SetInsertPoint(Entry);
Index: llvm/lib/IR/DebugInfo.cpp
===
--- llvm/lib/IR/DebugInfo.cpp
+++ llvm/lib/IR/DebugInfo.cpp
@@ -1289,7 +1289,7 @@
   return wrap(unwrap(Builder)->createGlobalVariableExpression(
   unwrapDI(Scope), {Name, NameLen}, {Linkage, LinkLen},
   unwrapDI(File), LineNo, unwrapDI(Ty), LocalToUnit,
-  unwrap(Expr), unwrapDI(Decl),
+  true, unwrap(Expr), unwrapDI(Decl),
   nullptr, AlignInBits));
 }
 
Index: llvm/lib/IR/DIBuilder.cpp
===
--- llvm/lib/IR/DIBuilder.cpp
+++ llvm/lib/IR/DIBuilder.cpp
@@ -640,13 +640,14 @@
 
 DIGlobalVariableExpression *DIBuilder::createGlobalVariableExpression(
 DIScope *Context, StringRef Name, StringRef LinkageName, DIFile *F,
-unsigned LineNumber, DIType *Ty, bool isLocalToUnit, DIExpression *Expr,
+unsigned LineNumber, DIType *Ty, bool isLocalToUnit,
+bool isDefined, DIExpression *Expr,
 MDNode *Decl, MDTuple *templateParams, uint32_t AlignInBits) {
   checkGlobalVariableScope(Context);
 
   auto *GV = DIGlobalVariable::getDistinct(
   VMContext, cast_or_null(Context), Name, LinkageName, F,
-  LineNumber, Ty, isLocalToUnit, true, cast_or_null(Decl),
+  LineNumber, Ty, isLocalToUnit, isDefined, cast_or_null(Decl),
   templateParams, AlignInBits);
   if (!Expr)
 Expr = createExpression();
Index: llvm/include/llvm/IR/DIBuilder.h
===
--- llvm/include/llvm/IR/DIBuilder.h
+++ llvm/include/llvm/IR/DIBuilder.h
@@ -583,7 +583,7 @@
 ///specified)
 DIGlobalVariableExpression *createGlobalVariableExpression(
 DIScope *Context, StringRef Name, StringRef LinkageName, DIFile *File,
-unsigned LineNo, DIType *Ty, bool isLocalToUnit,
+unsigned LineNo, DIType *Ty, bool isLocalToUnit, bool isDefined = true,
 DIExpression *Expr = nullptr, MDNode *Decl = nullptr,
 MDTuple *templateParams = nullptr, uint32_t AlignInBits = 0);
 
Index: clang/test/CodeGen/debug-info-extern-unused.c
===
--- /dev/null
+++ clang/test/CodeGen/debug-info-extern-unused.c
@@ -0,0 +1,27 @@
+// REQUIRES: bpf-registered-target
+// RUN: %clang -target bpf -emit-llvm -S -g %s -o - | FileCheck %s
+
+extern char ch;
+int test() {
+  return 0;
+}
+
+int test2() {
+  extern char ch2;
+  return 0;
+}
+
+extern int (*foo)(int);
+int test3() {
+  return 0;
+}
+
+int test4() {
+  extern int (*foo2)(int);
+  return 0;
+}
+
+// CHECK-NOT: distinct !DIGlobalVariable(name: "ch"
+// CHECK-NOT: distinct !DIGlobalVariable(name: "ch2"
+// CHECK-NOT: distinct !DIGlobalVariable(name: "foo"
+// CHECK-NOT: distinct !DIGlobalVariable(name: "foo2"
Index: clang/test/CodeGen/debug-info-extern-multi.c
===
--- /dev/null
+++ clang/test/CodeGen/debug-info-extern-multi.c
@@ -0,0 +1,23 @@
+// REQUIRES: bpf-registered-target
+// RUN: %clang -target bpf -emit-llvm -S -g %s -o - | FileCheck %s
+
+extern char ch;
+int test() {
+  extern short sh;
+  return ch + sh;
+}
+
+extern char (*foo)(char);
+int test2() {
+  return foo(0) + ch;
+}
+
+// CHECK: distinct !DIGlobalVariable(name: "ch",{{.*}} type: 

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/test/CodeGen/debug-info-extern-basic.c:14-23
+extern int (*foo)(int);
+int test3() {
+  return foo(0);
+}
+
+int test4() {
+  extern int (*foo2)(int);

yonghong-song wrote:
> dblaikie wrote:
> > What do these tests add? What sort of bugs would be caught by these global 
> > function pointer tests that wouldn't be caught by the char tests above them?
> These two additional tests to test extern function pointers. One of bpf 
> program use cases specifically need extern function debuginfo type so I added 
> a bunch. I do agree that this may be too much unnecessary and variable tests 
> should cover this.
> 
> I will remove all these extern function pointer tests including below one, 
> except one like the above test3() which should be enough.
Just to explain my thinking a bit more clearly here:

Testing this feature shouldn't be related to how you want to use the feature, 
but how the feature's designed/what likely places it could have bugs.

If the feature creates types in the same way for every type, and in a way 
that's already well tested for other types - then I don't think it's useful to 
test more than one type here.

(imagine, say, testing code generation for function calls - function calls are 
independent of the expressions that generate their arguments - so it doesn't 
make sense to test "foo(3)" and "foo(1 + 2)", etc - and of course there are 
limits to this sort of "orthogonality" analysis, taken too far you don't get 
enough test coverage, etc - but generally that's what I'm thinking of when I'm 
thinking about test case reduction, is semi-white-box "could I introduce a bug 
that would make one of these tests fail and teh other pass" - if it's pretty 
clear that the two systems are independent, and each independently well tested, 
I'll only test one or two paths through both of the systems)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70696



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

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song updated this revision to Diff 232980.
yonghong-song added a comment.

remove some redundant test cases


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70696

Files:
  clang/include/clang/AST/ASTConsumer.h
  clang/include/clang/Basic/TargetInfo.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Basic/Targets/BPF.h
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/CodeGen/ModuleBuilder.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CodeGen/debug-info-extern-basic.c
  clang/test/CodeGen/debug-info-extern-duplicate.c
  clang/test/CodeGen/debug-info-extern-multi.c
  clang/test/CodeGen/debug-info-extern-unused.c
  llvm/include/llvm/IR/DIBuilder.h
  llvm/lib/IR/DIBuilder.cpp
  llvm/lib/IR/DebugInfo.cpp
  llvm/unittests/Transforms/Utils/CloningTest.cpp

Index: llvm/unittests/Transforms/Utils/CloningTest.cpp
===
--- llvm/unittests/Transforms/Utils/CloningTest.cpp
+++ llvm/unittests/Transforms/Utils/CloningTest.cpp
@@ -764,7 +764,7 @@
 
 DBuilder.createGlobalVariableExpression(
 Subprogram, "unattached", "unattached", File, 1,
-DBuilder.createNullPtrType(), false, Expr);
+DBuilder.createNullPtrType(), false, true, Expr);
 
 auto *Entry = BasicBlock::Create(C, "", F);
 IBuilder.SetInsertPoint(Entry);
Index: llvm/lib/IR/DebugInfo.cpp
===
--- llvm/lib/IR/DebugInfo.cpp
+++ llvm/lib/IR/DebugInfo.cpp
@@ -1289,7 +1289,7 @@
   return wrap(unwrap(Builder)->createGlobalVariableExpression(
   unwrapDI(Scope), {Name, NameLen}, {Linkage, LinkLen},
   unwrapDI(File), LineNo, unwrapDI(Ty), LocalToUnit,
-  unwrap(Expr), unwrapDI(Decl),
+  true, unwrap(Expr), unwrapDI(Decl),
   nullptr, AlignInBits));
 }
 
Index: llvm/lib/IR/DIBuilder.cpp
===
--- llvm/lib/IR/DIBuilder.cpp
+++ llvm/lib/IR/DIBuilder.cpp
@@ -640,13 +640,14 @@
 
 DIGlobalVariableExpression *DIBuilder::createGlobalVariableExpression(
 DIScope *Context, StringRef Name, StringRef LinkageName, DIFile *F,
-unsigned LineNumber, DIType *Ty, bool isLocalToUnit, DIExpression *Expr,
+unsigned LineNumber, DIType *Ty, bool isLocalToUnit,
+bool isDefined, DIExpression *Expr,
 MDNode *Decl, MDTuple *templateParams, uint32_t AlignInBits) {
   checkGlobalVariableScope(Context);
 
   auto *GV = DIGlobalVariable::getDistinct(
   VMContext, cast_or_null(Context), Name, LinkageName, F,
-  LineNumber, Ty, isLocalToUnit, true, cast_or_null(Decl),
+  LineNumber, Ty, isLocalToUnit, isDefined, cast_or_null(Decl),
   templateParams, AlignInBits);
   if (!Expr)
 Expr = createExpression();
Index: llvm/include/llvm/IR/DIBuilder.h
===
--- llvm/include/llvm/IR/DIBuilder.h
+++ llvm/include/llvm/IR/DIBuilder.h
@@ -583,7 +583,7 @@
 ///specified)
 DIGlobalVariableExpression *createGlobalVariableExpression(
 DIScope *Context, StringRef Name, StringRef LinkageName, DIFile *File,
-unsigned LineNo, DIType *Ty, bool isLocalToUnit,
+unsigned LineNo, DIType *Ty, bool isLocalToUnit, bool isDefined = true,
 DIExpression *Expr = nullptr, MDNode *Decl = nullptr,
 MDTuple *templateParams = nullptr, uint32_t AlignInBits = 0);
 
Index: clang/test/CodeGen/debug-info-extern-unused.c
===
--- /dev/null
+++ clang/test/CodeGen/debug-info-extern-unused.c
@@ -0,0 +1,27 @@
+// REQUIRES: bpf-registered-target
+// RUN: %clang -target bpf -emit-llvm -S -g %s -o - | FileCheck %s
+
+extern char ch;
+int test() {
+  return 0;
+}
+
+int test2() {
+  extern char ch2;
+  return 0;
+}
+
+extern int (*foo)(int);
+int test3() {
+  return 0;
+}
+
+int test4() {
+  extern int (*foo2)(int);
+  return 0;
+}
+
+// CHECK-NOT: distinct !DIGlobalVariable(name: "ch"
+// CHECK-NOT: distinct !DIGlobalVariable(name: "ch2"
+// CHECK-NOT: distinct !DIGlobalVariable(name: "foo"
+// CHECK-NOT: distinct !DIGlobalVariable(name: "foo2"
Index: clang/test/CodeGen/debug-info-extern-multi.c
===
--- /dev/null
+++ clang/test/CodeGen/debug-info-extern-multi.c
@@ -0,0 +1,23 @@
+// REQUIRES: bpf-registered-target
+// RUN: %clang -target bpf -emit-llvm -S -g %s -o - | FileCheck %s
+
+extern char ch;
+int test() {
+  extern short sh;
+  return ch + sh;
+}
+
+extern char (*foo)(char);
+int test2() {
+  return foo(0) + ch;
+}
+
+// CHECK: distinct !DIGlobalVariable(name: "ch",{{.*}} type: ![[Tch:[0-9]+]], isLocal: false, isDefinition: false
+// CHECK: distinct 

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song marked an inline comment as done.
yonghong-song added inline comments.



Comment at: clang/test/CodeGen/debug-info-extern-basic.c:14-23
+extern int (*foo)(int);
+int test3() {
+  return foo(0);
+}
+
+int test4() {
+  extern int (*foo2)(int);

dblaikie wrote:
> What do these tests add? What sort of bugs would be caught by these global 
> function pointer tests that wouldn't be caught by the char tests above them?
These two additional tests to test extern function pointers. One of bpf program 
use cases specifically need extern function debuginfo type so I added a bunch. 
I do agree that this may be too much unnecessary and variable tests should 
cover this.

I will remove all these extern function pointer tests including below one, 
except one like the above test3() which should be enough.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70696



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

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[David Blaikie via Phabricator via cfe-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

In D70696#1774931 , @Jac1494 wrote:

> For some real life case like below we need debuginfo for declaration of 
> global extern variable .
>
> $cat shlib.c
>  int var;
>  int test()
>  { return var++; }
>
> $cat test.c
>  extern int test();
>  extern int var;
>  int main()
>  { var++; printf("%d\n",test()); }
>
> If we debug above case with gdb it is not giving types of variable var.
>  Because of no variable DIE is there in executable.
>
> (gdb) b main
>  Breakpoint 1 at 0x40063c: file test.c, line 4.
>  (gdb) pt var
>  type = 


Sounds like this is if you build shlib without debug info?

If you build shlib with debug info, it seems to be fine:

  $ clang-tot -shared -o libshlib.so shlib.c -fpic -g
  $ clang-tot test.c -lshlib -g -L.
  $ LD_LIBRARY_PATH=. gdb ./a.out
  (gdb) start
  Temporary breakpoint 1, main () at test.c:4
  4   { var++; printf("%d\n",test()); }
  (gdb) pt var
  type = int
  (gdb) 

This is the same as if the code were compiled statically & one test.c was 
compiled with debug info but shlib.c was compiled without debug info - the 
shared-library-ness doesn't change this situation.

-fstandalone-debug is the flag to use if parts of the program are built without 
debug info. Yes, that could be used to add global variable declaration to the 
DWARF & currently isn't - but I think that's a sufficiently nuanced/separate 
feature built on top of the work in this patch that it should be done 
separately, rather than trying to have all the design discussion for that in 
this code review.

This looks fine to me - I think a lot of the testing probably isn't testing 
"interesting" cases, but up to you.




Comment at: clang/test/CodeGen/debug-info-extern-basic.c:14-23
+extern int (*foo)(int);
+int test3() {
+  return foo(0);
+}
+
+int test4() {
+  extern int (*foo2)(int);

What do these tests add? What sort of bugs would be caught by these global 
function pointer tests that wouldn't be caught by the char tests above them?



Comment at: clang/test/CodeGen/debug-info-extern-duplicate.c:4-38
+extern char ch;
+extern char ch;
+int test() {
+  return ch;
+}
+
+extern char ch2;

Similarly - are these covering novel/distinct codepaths from the other tests? 
Are there codepaths that are different for multiple declarations?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70696



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

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

@Jac1494 Based on early discussion, we cannot just issue debuginfo for externs 
when -g is specified as this may cause debuginfo (and final binary) size bloat. 
One possible way is to enable it when `-fstandalone-debug` is specified. 
@dblaikie can you comment on this patch and general possible forward path for 
extern variable debug info support?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70696



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

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[Jaydeep Chauhan via Phabricator via cfe-commits]

Jac1494 added a comment.

For some real life case like below we need debuginfo for declaration of global 
extern variable .

$cat shlib.c
int var;
int test()
{ return var++; }

$cat test
extern int test();
extern int var;
int main()
{ var++; printf("%d\n",test()); }

If we debug above case with gdb it is not giving types of variable var.
Because of no variable DIE is there in executable.

(gdb) b main
Breakpoint 1 at 0x40063c: file test.c, line 5.
(gdb) pt var
type = 

To add variable debuginfo we need to merge below patch in code.
diff --git a/clang/include/clang/Basic/TargetInfo.h 
b/clang/include/clang/Basic/TargetInfo.h
index 9a3bb98..df79d46 100644

- a/clang/include/clang/Basic/TargetInfo.h

+++ b/clang/include/clang/Basic/TargetInfo.h
@@ -1371,6 +1371,9 @@ public:

  virtual void setAuxTarget(const TargetInfo *Aux) {}

+  /// Whether target allows debuginfo types for decl only variables.
+  virtual bool allowDebugInfoForExternalVar() const { return true; }
+
protected:

  /// Copy type and layout related info.
  void copyAuxTarget(const TargetInfo *Aux);

diff --git a/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp 
b/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
index a61c98e..92245c0 100644

- a/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

+++ b/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
@@ -158,7 +158,11 @@ DIE *DwarfCompileUnit::getOrCreateGlobalVariableDIE(

  if (!GV->isDefinition())
addFlag(*VariableDIE, dwarf::DW_AT_declaration);
  else

+  {
+/*Added location */
+addLocationAttribute(VariableDIE, GV, GlobalExprs);

  addGlobalName(GV->getName(), *VariableDIE, DeclContext);

+  }

  if (uint32_t AlignInBytes = GV->getAlignInBytes())
addUInt(*VariableDIE, dwarf::DW_AT_alignment, dwarf::DW_FORM_udata,

@@ -167,9 +171,6 @@ DIE *DwarfCompileUnit::getOrCreateGlobalVariableDIE(

  if (MDTuple *TP = GV->getTemplateParams())
addTemplateParams(*VariableDIE, DINodeArray(TP));

- // Add location.
- addLocationAttribute(VariableDIE, GV, GlobalExprs); - return VariableDIE; }




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70696



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

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

@dblaikie I consolidated test cases as you suggested. Along the way, I also 
added some more tests. I already answered some of your questions earlier. 
Please let me know what you think.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70696



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

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song updated this revision to Diff 232426.
yonghong-song added a comment.

consolidated into less test files, added more test cases along the way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70696

Files:
  clang/include/clang/AST/ASTConsumer.h
  clang/include/clang/Basic/TargetInfo.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Basic/Targets/BPF.h
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/CodeGen/ModuleBuilder.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CodeGen/debug-info-extern-basic.c
  clang/test/CodeGen/debug-info-extern-duplicate.c
  clang/test/CodeGen/debug-info-extern-multi.c
  clang/test/CodeGen/debug-info-extern-unused.c
  llvm/include/llvm/IR/DIBuilder.h
  llvm/lib/IR/DIBuilder.cpp
  llvm/lib/IR/DebugInfo.cpp
  llvm/unittests/Transforms/Utils/CloningTest.cpp

Index: llvm/unittests/Transforms/Utils/CloningTest.cpp
===
--- llvm/unittests/Transforms/Utils/CloningTest.cpp
+++ llvm/unittests/Transforms/Utils/CloningTest.cpp
@@ -764,7 +764,7 @@
 
 DBuilder.createGlobalVariableExpression(
 Subprogram, "unattached", "unattached", File, 1,
-DBuilder.createNullPtrType(), false, Expr);
+DBuilder.createNullPtrType(), false, true, Expr);
 
 auto *Entry = BasicBlock::Create(C, "", F);
 IBuilder.SetInsertPoint(Entry);
Index: llvm/lib/IR/DebugInfo.cpp
===
--- llvm/lib/IR/DebugInfo.cpp
+++ llvm/lib/IR/DebugInfo.cpp
@@ -1289,7 +1289,7 @@
   return wrap(unwrap(Builder)->createGlobalVariableExpression(
   unwrapDI(Scope), {Name, NameLen}, {Linkage, LinkLen},
   unwrapDI(File), LineNo, unwrapDI(Ty), LocalToUnit,
-  unwrap(Expr), unwrapDI(Decl),
+  true, unwrap(Expr), unwrapDI(Decl),
   nullptr, AlignInBits));
 }
 
Index: llvm/lib/IR/DIBuilder.cpp
===
--- llvm/lib/IR/DIBuilder.cpp
+++ llvm/lib/IR/DIBuilder.cpp
@@ -640,13 +640,14 @@
 
 DIGlobalVariableExpression *DIBuilder::createGlobalVariableExpression(
 DIScope *Context, StringRef Name, StringRef LinkageName, DIFile *F,
-unsigned LineNumber, DIType *Ty, bool isLocalToUnit, DIExpression *Expr,
+unsigned LineNumber, DIType *Ty, bool isLocalToUnit,
+bool isDefined, DIExpression *Expr,
 MDNode *Decl, MDTuple *templateParams, uint32_t AlignInBits) {
   checkGlobalVariableScope(Context);
 
   auto *GV = DIGlobalVariable::getDistinct(
   VMContext, cast_or_null(Context), Name, LinkageName, F,
-  LineNumber, Ty, isLocalToUnit, true, cast_or_null(Decl),
+  LineNumber, Ty, isLocalToUnit, isDefined, cast_or_null(Decl),
   templateParams, AlignInBits);
   if (!Expr)
 Expr = createExpression();
Index: llvm/include/llvm/IR/DIBuilder.h
===
--- llvm/include/llvm/IR/DIBuilder.h
+++ llvm/include/llvm/IR/DIBuilder.h
@@ -583,7 +583,7 @@
 ///specified)
 DIGlobalVariableExpression *createGlobalVariableExpression(
 DIScope *Context, StringRef Name, StringRef LinkageName, DIFile *File,
-unsigned LineNo, DIType *Ty, bool isLocalToUnit,
+unsigned LineNo, DIType *Ty, bool isLocalToUnit, bool isDefined = true,
 DIExpression *Expr = nullptr, MDNode *Decl = nullptr,
 MDTuple *templateParams = nullptr, uint32_t AlignInBits = 0);
 
Index: clang/test/CodeGen/debug-info-extern-unused.c
===
--- /dev/null
+++ clang/test/CodeGen/debug-info-extern-unused.c
@@ -0,0 +1,27 @@
+// REQUIRES: bpf-registered-target
+// RUN: %clang -target bpf -emit-llvm -S -g %s -o - | FileCheck %s
+
+extern char ch;
+int test() {
+  return 0;
+}
+
+int test2() {
+  extern char ch2;
+  return 0;
+}
+
+extern int (*foo)(int);
+int test3() {
+  return 0;
+}
+
+int test4() {
+  extern int (*foo2)(int);
+  return 0;
+}
+
+// CHECK-NOT: distinct !DIGlobalVariable(name: "ch"
+// CHECK-NOT: distinct !DIGlobalVariable(name: "ch2"
+// CHECK-NOT: distinct !DIGlobalVariable(name: "foo"
+// CHECK-NOT: distinct !DIGlobalVariable(name: "foo2"
Index: clang/test/CodeGen/debug-info-extern-multi.c
===
--- /dev/null
+++ clang/test/CodeGen/debug-info-extern-multi.c
@@ -0,0 +1,23 @@
+// REQUIRES: bpf-registered-target
+// RUN: %clang -target bpf -emit-llvm -S -g %s -o - | FileCheck %s
+
+extern char ch;
+int test() {
+  extern short sh;
+  return ch + sh;
+}
+
+extern char (*foo)(char);
+int test2() {
+  return foo(0) + ch;
+}
+
+// CHECK: distinct !DIGlobalVariable(name: "ch",{{.*}} type: ![[Tch:[0-9]+]], isLocal: false, 

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

In D70696#1767616 , @dblaikie wrote:

> Many of the test cases could be collapsed into one file - using different 
> variables that are used, unused, locally or globally declared, etc.


Okay. Will try to consolidate into one or fewer files. Originally, I am using 
different files to avoid cases where in the future clang may generate different 
ordering w.r.t. different global variables.

> Is this new code only active for C compilations? (does clang reject requests 
> for the bpf target when the input is C++?) I ask due to the concerns around 
> globals used in inline functions where the inline function is unused - though 
> C has inline functions too, so I guess the question stands: Is that a 
> problem? What happens?

Currently, yes. my implementation only active for C compilation.
In the kernel documentation 
(https://www.kernel.org/doc/Documentation/networking/filter.txt), we have:

  The new instruction set was originally designed with the possible goal in
  mind to write programs in "restricted C" and compile into eBPF with a optional
  GCC/LLVM backend, so that it can just-in-time map to modern 64-bit CPUs with
  minimal performance overhead over two steps, that is, C -> eBPF -> native 
code.

For LLVM itself, people can compile a C++ program into BPF target. But 
"officially" we do not
support this. That is why I restricted to C only. For C++ programs, we don't 
get much usage/tests
from users.

Do you have a concrete example for this? I tried the following:

  -bash-4.4$ cat t.h
  inline int foo() { extern int g; return g; }
  -bash-4.4$ cat t.c
  int bar() { return 0; }
  -bash-4.4$ clang -target bpf -g -O0 -S -emit-llvm t.c

`foo` is not used, clang seems smart enough to deduce `g` is not used, so no 
debuginfo is emitted in this case.

In general, if an inline function is not used but an external variable is used 
inside that inline function, the worst case is extra debuginfo for that 
external variable. Since it is not used, it won't impact bpf loader.

> Should this be driven by a lower level of code generation - ie: is it OK to 
> only produce debug info descriptions for variables that are referenced in the 
> resulting LLVM IR? (compile time constants wouldn't be described then, for 
> instance - since they won't be code generated, loaded from memory, etc)

Yes, it is OK to only produce debug info only for variables that are referenced 
in the resulting LLVM IR. But we are discussing extern variables and no compile 
time constants here. Maybe I miss something?

> Is there somewhere I should be reading about the design requirement for these 
> global variable descriptions to understand the justification for them & the 
> ramifications if there are bugs that cause them not to be emitted?

We do not have design documents yet. The following are two links and I can 
explain more:

1. 
https://lore.kernel.org/bpf/CAEf4BzYCNo5GeVGMhp3fhysQ=_axAf=23ptwazs-yayafmx...@mail.gmail.com/T/#t

The typical config is at /boot/config-<...> in a linux machine. The config 
entry typically look like:

  CONFIG_CC_IS_GCC=y
  CONFIG_GCC_VERSION=40805
  CONFIG_INITRAMFS_SOURCE=""

Suppose a bpf program wants to check config value and based on its value to do 
something, user can write:

  extern bool CONFIG_CC_IS_GCC;
  extern int CONFIG_GCC_VERSION;
  extern char CONFIG_INITRAMFS_SOURCE[20];
  ...
  if (CONFIG_CC_IS_GCC) ...
  map_val = CONFIG_GCC_VERSION; 
  __builtin_memcpy(map_value, 8, CONFIG_INITRAMFS_SOURCE);

bpfloader will create a data section store all the above info and patch the 
correct address to the code.
Without extern var type info, it becomes a guess game what type/size the user 
is using.
Based on precise type information, bpf loader is able to do relocation much 
easily.

2. 
https://lore.kernel.org/bpf/87eez4odqp@toke.dk/T/#m8d5c3e87ffe7f2764e02d722cb0d8cbc136880ed

This is for bpf program verification.
For example,
bpf_prog1:

  foo(...) {
... x ... y ...
z =  bar(x /*struct t * */, y /* int */);
...
  }

and there is no bar body available yet.
The kernel verifier still able to verify program "foo"
and makes sure type leading to bar for all parameters
are correct.

Later, if there is a program
prog2(struct t *a, int b)
which is verified independently.

The in kernel, prog1 can call prog2 if there parameter types
and return types match. This is the BPF-way dynamic linking.
The types for external used functions can help cut down
verification cost at linking time.

If there is no debug information for these extern variables, the current
proposal is to fail the bpf loader and verifier. User can always workaround
such issues to create bpf maps for the first use case (which is more expensive 
and not user friendly) and do static
linking before loading into the kernel for the second use case.


Repository:
  rG LLVM Github Monorepo

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

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Many of the test cases could be collapsed into one file - using different 
variables that are used, unused, locally or globally declared, etc.

Is this new code only active for C compilations? (does clang reject requests 
for the bpf target when the input is C++?) I ask due to the concerns around 
globals used in inline functions where the inline function is unused - though C 
has inline functions too, so I guess the question stands: Is that a problem? 
What happens?

Should this be driven by a lower level of code generation - ie: is it OK to 
only produce debug info descriptions for variables that are referenced in the 
resulting LLVM IR? (compile time constants wouldn't be described then, for 
instance - since they won't be code generated, loaded from memory, etc)

Is there somewhere I should be reading about the design requirement for these 
global variable descriptions to understand the justification for them & the 
ramifications if there are bugs that cause them not to be emitted?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70696



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

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song updated this revision to Diff 231786.
yonghong-song edited the summary of this revision.
yonghong-song added a comment.

clang-format change (from @aprantl) , add a little details into summary related 
to recent discussion,
move tests from CodeGenCXX to CodeGen as they are all C tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70696

Files:
  clang/include/clang/AST/ASTConsumer.h
  clang/include/clang/Basic/TargetInfo.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Basic/Targets/BPF.h
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/CodeGen/ModuleBuilder.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CodeGen/debug-info-extern-duplicate-2.c
  clang/test/CodeGen/debug-info-extern-duplicate.c
  clang/test/CodeGen/debug-info-extern-unused.c
  clang/test/CodeGen/debug-info-extern-var-char-2.c
  clang/test/CodeGen/debug-info-extern-var-char.c
  clang/test/CodeGen/debug-info-extern-var-func.c
  clang/test/CodeGen/debug-info-extern-var-multi.c
  llvm/include/llvm/IR/DIBuilder.h
  llvm/lib/IR/DIBuilder.cpp
  llvm/lib/IR/DebugInfo.cpp
  llvm/unittests/Transforms/Utils/CloningTest.cpp

Index: llvm/unittests/Transforms/Utils/CloningTest.cpp
===
--- llvm/unittests/Transforms/Utils/CloningTest.cpp
+++ llvm/unittests/Transforms/Utils/CloningTest.cpp
@@ -764,7 +764,7 @@
 
 DBuilder.createGlobalVariableExpression(
 Subprogram, "unattached", "unattached", File, 1,
-DBuilder.createNullPtrType(), false, Expr);
+DBuilder.createNullPtrType(), false, true, Expr);
 
 auto *Entry = BasicBlock::Create(C, "", F);
 IBuilder.SetInsertPoint(Entry);
Index: llvm/lib/IR/DebugInfo.cpp
===
--- llvm/lib/IR/DebugInfo.cpp
+++ llvm/lib/IR/DebugInfo.cpp
@@ -1290,7 +1290,7 @@
   return wrap(unwrap(Builder)->createGlobalVariableExpression(
   unwrapDI(Scope), {Name, NameLen}, {Linkage, LinkLen},
   unwrapDI(File), LineNo, unwrapDI(Ty), LocalToUnit,
-  unwrap(Expr), unwrapDI(Decl),
+  true, unwrap(Expr), unwrapDI(Decl),
   nullptr, AlignInBits));
 }
 
Index: llvm/lib/IR/DIBuilder.cpp
===
--- llvm/lib/IR/DIBuilder.cpp
+++ llvm/lib/IR/DIBuilder.cpp
@@ -639,13 +639,14 @@
 
 DIGlobalVariableExpression *DIBuilder::createGlobalVariableExpression(
 DIScope *Context, StringRef Name, StringRef LinkageName, DIFile *F,
-unsigned LineNumber, DIType *Ty, bool isLocalToUnit, DIExpression *Expr,
+unsigned LineNumber, DIType *Ty, bool isLocalToUnit,
+bool isDefined, DIExpression *Expr,
 MDNode *Decl, MDTuple *templateParams, uint32_t AlignInBits) {
   checkGlobalVariableScope(Context);
 
   auto *GV = DIGlobalVariable::getDistinct(
   VMContext, cast_or_null(Context), Name, LinkageName, F,
-  LineNumber, Ty, isLocalToUnit, true, cast_or_null(Decl),
+  LineNumber, Ty, isLocalToUnit, isDefined, cast_or_null(Decl),
   templateParams, AlignInBits);
   if (!Expr)
 Expr = createExpression();
Index: llvm/include/llvm/IR/DIBuilder.h
===
--- llvm/include/llvm/IR/DIBuilder.h
+++ llvm/include/llvm/IR/DIBuilder.h
@@ -581,7 +581,7 @@
 ///specified)
 DIGlobalVariableExpression *createGlobalVariableExpression(
 DIScope *Context, StringRef Name, StringRef LinkageName, DIFile *File,
-unsigned LineNo, DIType *Ty, bool isLocalToUnit,
+unsigned LineNo, DIType *Ty, bool isLocalToUnit, bool isDefined = true,
 DIExpression *Expr = nullptr, MDNode *Decl = nullptr,
 MDTuple *templateParams = nullptr, uint32_t AlignInBits = 0);
 
Index: clang/test/CodeGen/debug-info-extern-var-multi.c
===
--- /dev/null
+++ clang/test/CodeGen/debug-info-extern-var-multi.c
@@ -0,0 +1,13 @@
+// REQUIRES: bpf-registered-target
+// RUN: %clang -target bpf -emit-llvm -S -g %s -o - | FileCheck %s
+
+extern char ch;
+int test() {
+  extern short sh;
+  return ch + sh;
+}
+
+// CHECK: distinct !DIGlobalVariable(name: "ch",{{.*}} type: ![[Tch:[0-9]+]], isLocal: false, isDefinition: false
+// CHECK: distinct !DIGlobalVariable(name: "sh",{{.*}} type: ![[Tsh:[0-9]+]], isLocal: false, isDefinition: false
+// CHECK: ![[Tsh]] = !DIBasicType(name: "short", size: 16, encoding: DW_ATE_signed)
+// CHECK: ![[Tch]] = !DIBasicType(name: "char", size: 8, encoding: DW_ATE_signed_char)
Index: clang/test/CodeGen/debug-info-extern-var-func.c
===
--- /dev/null
+++ clang/test/CodeGen/debug-info-extern-var-func.c
@@ -0,0 +1,13 

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

@dblaikie  The concern for that users using -fstandalone-debug may suddenly see 
a bloat of external var debug info is totally valid. Let me just stick to BPF 
use case for now. If somebody else ever wants this information with 
`-fstandalone-debug`, the restriction can be evaluated and relaxed then.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70696



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

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D70696#1765823 , @SouraVX wrote:

> @dblaikie  , If you try switching to C compiler. then the DW_TAG_variable is 
> their. for C++ . It's not generated.  Please check godbolt again. -- though 
> strange


Hmm - I don't seem to see it in C either: https://godbolt.org/z/EUufYE (it's 
present in 9.2 as it is with C++, but not present in GCC trunk)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70696



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

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[Sourabh Singh Tomar via Phabricator via cfe-commits]

SouraVX added a comment.

In D70696#1765843 , @dblaikie wrote:

> In D70696#1765823 , @SouraVX wrote:
>
> > @dblaikie  , If you try switching to C compiler. then the DW_TAG_variable 
> > is their. for C++ . It's not generated.  Please check godbolt again. -- 
> > though strange
>
>
> Hmm - I don't seem to see it in C either: https://godbolt.org/z/EUufYE (it's 
> present in 9.2 as it is with C++, but not present in GCC trunk)


Ahh,, missed the trunk -- Sorry!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70696



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

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[Sourabh Singh Tomar via Phabricator via cfe-commits]

SouraVX added a comment.

@dblaikie  , If you try switching to C compiler. then the DW_TAG_variable is 
their. for C++ . It's not generated.  Please check godbolt again. -- though 
strange


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70696



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

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D70696#1765789 , @dblaikie wrote:

> In D70696#1765784 , @probinson wrote:
>
> > In D70696#1765671 , @dblaikie 
> > wrote:
> >
> > > In D70696#1765637 , @probinson 
> > > wrote:
> > >
> > > > For the case:
> > > >
> > > >   cat ref.c
> > > >   extern int global_var;
> > > >   int main() { return global_var; }
> > > >
> > > >
> > > > I *do* expect to see debug info for the declaration of global_var.
> > >
> > >
> > > FWIW I'd only expect it there with -fstandalone-debug - with 
> > > -fno-standalone-debug I'd expect this code to rely on the assumption that 
> > > def.c is also compiled with debug info.
> >
> >
> > When global_var is defined in a separate .so, you might not have the symbol 
> > at all.  It's helpful to be able to report "that symbol is defined outside 
> > this executable" (and IIRC, gdb can look it up by mangled name and actually 
> > show it to you anyway).  Without even the declaration, you get "that symbol 
> > that you can see right there in your source? It doesn't exist, haha."  So, 
> > I would definitely rather see a declaration for a referenced global.
> >  Even if we currently don't do that.
>
>
> Same would be true of all functions as well though, right? Neither LLVM nor 
> GCC emit declarations for all called functions (& I expect that'd produce 
> significant size growth - I'm worried enough about global variable 
> declarations that are referenced by unused inline functions or other similar 
> things, and pull in complex type hierarchies, etc - let alone all functions 
> called too).


(but in general, that's what -fstandalone-debug is for: If you want this file's 
debug info to be complete (for some value of complete) even if other parts of 
the program (shared libraries, static libraries, however they're done) are 
compiled without debug info - though I still suspect emitting debug info for 
all variables and functions called from this file would be a pretty big size 
cost)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70696



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

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D70696#1765784 , @probinson wrote:

> In D70696#1765671 , @dblaikie wrote:
>
> > In D70696#1765637 , @probinson 
> > wrote:
> >
> > > For the case:
> > >
> > >   cat ref.c
> > >   extern int global_var;
> > >   int main() { return global_var; }
> > >
> > >
> > > I *do* expect to see debug info for the declaration of global_var.
> >
> >
> > FWIW I'd only expect it there with -fstandalone-debug - with 
> > -fno-standalone-debug I'd expect this code to rely on the assumption that 
> > def.c is also compiled with debug info.
>
>
> When global_var is defined in a separate .so, you might not have the symbol 
> at all.  It's helpful to be able to report "that symbol is defined outside 
> this executable" (and IIRC, gdb can look it up by mangled name and actually 
> show it to you anyway).  Without even the declaration, you get "that symbol 
> that you can see right there in your source? It doesn't exist, haha."  So, I 
> would definitely rather see a declaration for a referenced global.
>  Even if we currently don't do that.


Same would be true of all functions as well though, right? Neither LLVM nor GCC 
emit declarations for all called functions (& I expect that'd produce 
significant size growth - I'm worried enough about global variable declarations 
that are referenced by unused inline functions or other similar things, and 
pull in complex type hierarchies, etc - let alone all functions called too).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70696



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

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

In D70696#1765671 , @dblaikie wrote:

> In D70696#1765637 , @probinson wrote:
>
> > For the case:
> >
> >   cat ref.c
> >   extern int global_var;
> >   int main() { return global_var; }
> >
> >
> > I *do* expect to see debug info for the declaration of global_var.
>
>
> FWIW I'd only expect it there with -fstandalone-debug - with 
> -fno-standalone-debug I'd expect this code to rely on the assumption that 
> def.c is also compiled with debug info.


When global_var is defined in a separate .so, you might not have the symbol at 
all.  It's helpful to be able to report "that symbol is defined outside this 
executable" (and IIRC, gdb can look it up by mangled name and actually show it 
to you anyway).  Without even the declaration, you get "that symbol that you 
can see right there in your source? It doesn't exist, haha."  So, I would 
definitely rather see a declaration for a referenced global.
Even if we currently don't do that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70696



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

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D70696#1765675 , @yonghong-song 
wrote:

> @probinson for the question,
>
> > Does bpf require debug info for the declaration of global_var in noref.c ?
>
> No, bpf only cares the referenced external global variables. So my current 
> implementation does not emit debug info
>  for external global_var in noref.c.
>
> It is just strange that gcc 7.3.1 (did not test, but maybe later gcc versions 
> as well) emits the debuginfo (encoded in dwarf)
>  even if the external variable is not used in the current compilation unit. 
> Not sure what is the rationale behind it.


FWIW, looks like GCC trunk doesn't do this anymore: 
https://godbolt.org/z/EP5mWF (try it with GCC trunk V GCC 9.2 - 9.2 mentions 
"glbl_var" and trunk does not).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70696



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

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D70696#1765714 , @yonghong-song 
wrote:

> @dblaikie Good points. I will guard external variable debug info generation 
> under `-fstandalone-debug` flag.


Oh, I figured given your needs this'd be guarded behind the target being BPF? 
While I don't mind it being also enabled by -fstandalone-debug (or, if you 
like, I guess maybe BPF is a "standalone-debug by default" target (like 
MacOS/Darwin, I think - due to some LLDB limitations at the moment) & maybe 
that captures all the BPF quirks in this regard?) - though I wouldn't leap to 
it unless someone else is already interested in that feature in 
-fstandalone-debug. (I'd be a bit worried about debug info growth and how "is 
this global variable referenced" is computed - eg: it could be referenced from 
a dead (uncalled) inline function)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70696



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

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[Sourabh Singh Tomar via Phabricator via cfe-commits]

SouraVX added a comment.

In D70696#1765675 , @yonghong-song 
wrote:

> @probinson for the question,
>
> > Does bpf require debug info for the declaration of global_var in noref.c ?
>
> No, bpf only cares the referenced external global variables. So my current 
> implementation does not emit debug info
>  for external global_var in noref.c.
>
> It is just strange that gcc 7.3.1 (did not test, but maybe later gcc versions 
> as well) emits the debuginfo (encoded in dwarf)
>  even if the external variable is not used in the current compilation unit. 
> Not sure what is the rationale behind it.


I'm using gcc 9.2.0 -- it's generating, for `noref.c`{Not used in compilation 
unit} case also.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70696



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

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

In D70696#1765675 , @yonghong-song 
wrote:

> It is just strange that gcc 7.3.1 (did not test, but maybe later gcc versions 
> as well) emits the debuginfo (encoded in dwarf)
>  even if the external variable is not used in the current compilation unit. 
> Not sure what is the rationale behind it.


If it were me, it would be because the declaration is available in the current 
CU, therefore if the debugger is stopped somewhere in the CU, the symbol should 
be available to the debugger.  Having the external declaration means you don't 
need to go rooting around in other CUs for the symbol.

This is just one of those trade-offs that producers and consumers make, 
regarding completeness of information versus bloat of unnecessary information.  
There is no "right" answer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70696



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

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

@dblaikie Good points. I will guard external variable debug info generation 
under `-fstandalone-debug` flag.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70696



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

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

@probinson for the question,

> Does bpf require debug info for the declaration of global_var in noref.c ?

No, bpf only cares the referenced external global variables. So my current 
implementation does not emit debug info
for external global_var in noref.c.

It is just strange that gcc 7.3.1 (did not test, but maybe later gcc versions 
as well) emits the debuginfo (encoded in dwarf)
even if the external variable is not used in the current compilation unit. Not 
sure what is the rationale behind it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70696



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

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D70696#1765637 , @probinson wrote:

> For the case:
>
>   cat def.c
>   int global_var = 2;
>
>
> def.o should have debug info for the definition of global_var.
>  For the case:
>
>   cat noref.c
>   extern int global_var;
>   int main() {}
>
>
> I would not expect to see debug info for the declaration of global_var.
>  For the case:
>
>   cat ref.c
>   extern int global_var;
>   int main() { return global_var; }
>
>
> I *do* expect to see debug info for the declaration of global_var.


FWIW I'd only expect it there with -fstandalone-debug - with 
-fno-standalone-debug I'd expect this code to rely on the assumption that def.c 
is also compiled with debug info.

(as it stands today, Clang/LLVM never produces debug info for global_var in 
ref.c, even with -fstandalone-debug & I'm not too fussed about that, but would 
be OK if someone wanted to fix/improve that)

> Does bpf require debug info for the declaration of global_var in `noref.c` ?

Yeah, +1, I'm still curious to know more/trying to understand this ^ 
requirement.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70696



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

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song marked an inline comment as done.
yonghong-song added inline comments.



Comment at: clang/lib/Sema/Sema.cpp:1140
 
+  for (auto D: ExternalDeclarations) {
+if (!D || D->isInvalidDecl() || D->getPreviousDecl() || !D->isUsed())

aprantl wrote:
> clang-format
Thanks for the review! Will update the patch today after running clang-format.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70696



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

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

For the case:

  cat def.c
  int global_var = 2;

def.o should have debug info for the definition of global_var.
For the case:

  cat noref.c
  extern int global_var;
  int main() {}

I would not expect to see debug info for the declaration of global_var.
For the case:

  cat ref.c
  extern int global_var;
  int main() { return global_var; }

I *do* expect to see debug info for the declaration of global_var.

Does bpf require debug info for the declaration of global_var in `noref.c` ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70696



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

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[Adrian Prantl via Phabricator via cfe-commits]

aprantl added inline comments.



Comment at: clang/lib/Sema/Sema.cpp:1140
 
+  for (auto D: ExternalDeclarations) {
+if (!D || D->isInvalidDecl() || D->getPreviousDecl() || !D->isUsed())

clang-format


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70696



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

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[Sourabh Singh Tomar via Phabricator via cfe-commits]

SouraVX added a comment.

In D70696#1764306 , @umesh.kalappa0 
wrote:

> @SouraVX  and @yonghong-song
>
> cat extern.c 
>  int global_var = 2;
>
> compile as an extern.c shared a library ,then final executable a.out doesn't 
> have debugInfo for global_var.


Perfect! Now we have a better use case and motivation that we should generate 
declaration debug entry. for a variable declared as extern.  
that will also solves @umesh.kalappa0 above case. `a.out` will have debuginfo 
for `global_var`, since we generated it's declaration debuginfo in primary 
object `main.c`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70696



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

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[Umesh Kalappa via Phabricator via cfe-commits]

umesh.kalappa0 added a comment.

@SouraVX  and @yonghong-song

cat extern.c 
int global_var = 2;

compile as an extern.c shared a library ,then final executable a.out doesn't 
have debugInfo for global_var.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70696



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

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[Sourabh Singh Tomar via Phabricator via cfe-commits]

SouraVX added a comment.

In D70696#1764043 , @yonghong-song 
wrote:

> If I remove `!D->isUsed()`, I should be able to generate the debuginfo for 
> `global_var` and eventually in dwarf in your above case.
>  What exactly your use case here for these unused external variables?


I simplified that test case, too much. Obiviously, for a real situation, that 
global variable must be defined possibly in some different file, which 
eventually got linked to final executable. Consider this for a moment -
`cat extern.c`

  int global_var = 2;

`cat main.c`

  extern int global_var;
  int main(){
   int local = global_var;
  }

Now for above, clang produce debug info for `gloabal_var` in `extern.c` fine, 
Not in `main.c` for Declaration of `gloabl_var`. Eventually when producing 
final binary{a.out}, linker will merge debug_info section of {extern.c and 
main.c} and we end up with debugging entry in `a.out` , consequently debugger 
is also seems to be happy with it.

But I think, it would be nice to have a declaration debug entry for 
`global_var` in file{main.o) debugging information contribution. Because it's 
using this global variable{declared extern}. like in present case!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70696



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

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

@SouraVX Currently, I filtered all external globals if they are not used. See

  for (auto D: ExternalDeclarations) {
if (!D || D->isInvalidDecl() || D->getPreviousDecl() || !D->isUsed())
  continue;
  
Consumer.CompleteExternalDeclaration(D);
  }

If I remove `!D->isUsed()`, I should be able to generate the debuginfo for 
`global_var` and eventually in dwarf in your above case.

I tested with gcc 7.3.1, indeed gcc generates dwarf info for `global_var` as 
you stated in the above.

What exactly your use case here for these unused external variables?

I would like to hear comments from llvm/clang debuginfo experts before making 
the change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70696



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

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[Sourabh Singh Tomar via Phabricator via cfe-commits]

SouraVX added a comment.

Hi @yonghong-song ,  Thanks for doing this. 
I have a doubt, regarding clang behavior, is this relevant to this.

Consider the following test case --

  extern global_var;
  int main(){}

Now when compiled with clang -g test.c, In file ELF[DWARF] file
their is no, DebugInfo for `global_var`. 
While gcc compiled binary gcc -g test.c. has DebugInfo[DWARF]

  DW_TAG_variable
   DW_AT_name  ("global_var")
   DW_AT_decl_file ("/test.c)
   DW_AT_decl_line (1)
   DW_AT_decl_column   (0x0c)
   DW_AT_type  (0x0039 "int")
   DW_AT_external  (true)
   DW_AT_declaration   (true)

Is this relevant ? Does this patch, adds support for this in clang ? I mean 
`DIGlobalVariable` related stuff in clang generated Debug Metadata ? seems to, 
but still confirming.
Maybe I can use this, to build DWARF related DebugInfo on top of this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70696



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

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

The corresponding BPF patch to consume the debuginfo 
https://reviews.llvm.org/D70697.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70696



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

[PATCH] D70696: [DebugInfo] Support to emit debugInfo for extern variables

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song created this revision.
yonghong-song added reviewers: dblaikie, aprantl, RKSimon.
yonghong-song added a project: debug-info.
Herald added subscribers: llvm-commits, cfe-commits, hiraditya.
Herald added projects: clang, LLVM.

Extern variable usage in BPF is different from traditional
pure user space application. Recent discussion in linux bpf 
mailing list has two use cases where debug info types are 
required to use extern variables:

- extern types are required to have a suitable interface in libbpf (bpf loader) 
to provide kernel config parameters to bpf programs. 
https://lore.kernel.org/bpf/CAEf4BzYCNo5GeVGMhp3fhysQ=_axAf=23ptwazs-yayafmx...@mail.gmail.com/T/#t
- extern types are required so kernel bpf verifier can verify program which 
uses external functions more precisely. This will make later link with actual 
external function no need to reverify. 
https://lore.kernel.org/bpf/87eez4odqp@toke.dk/T/#m8d5c3e87ffe7f2764e02d722cb0d8cbc136880ed

This patch added clang support to emit debuginfo for extern variables
with a TargetInfo hook to enable it. Currently, only BPF target enables
to generate debug info for extern variables. The emission of
such debuginfo is disabled for C++ at this moment since BPF only
supports a subset of C language. Emission with C++ can be enabled
later if an appropriate use case is identified.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70696

Files:
  clang/include/clang/AST/ASTConsumer.h
  clang/include/clang/Basic/TargetInfo.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Basic/Targets/BPF.h
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/CodeGen/ModuleBuilder.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/test/CodeGenCXX/debug-info-extern-duplicate-2.c
  clang/test/CodeGenCXX/debug-info-extern-duplicate.c
  clang/test/CodeGenCXX/debug-info-extern-unused.c
  clang/test/CodeGenCXX/debug-info-extern-var-char-2.c
  clang/test/CodeGenCXX/debug-info-extern-var-char.c
  clang/test/CodeGenCXX/debug-info-extern-var-func.c
  clang/test/CodeGenCXX/debug-info-extern-var-multi.c
  llvm/include/llvm/IR/DIBuilder.h
  llvm/lib/IR/DIBuilder.cpp
  llvm/lib/IR/DebugInfo.cpp
  llvm/unittests/Transforms/Utils/CloningTest.cpp

Index: llvm/unittests/Transforms/Utils/CloningTest.cpp
===
--- llvm/unittests/Transforms/Utils/CloningTest.cpp
+++ llvm/unittests/Transforms/Utils/CloningTest.cpp
@@ -764,7 +764,7 @@
 
 DBuilder.createGlobalVariableExpression(
 Subprogram, "unattached", "unattached", File, 1,
-DBuilder.createNullPtrType(), false, Expr);
+DBuilder.createNullPtrType(), false, true, Expr);
 
 auto *Entry = BasicBlock::Create(C, "", F);
 IBuilder.SetInsertPoint(Entry);
Index: llvm/lib/IR/DebugInfo.cpp
===
--- llvm/lib/IR/DebugInfo.cpp
+++ llvm/lib/IR/DebugInfo.cpp
@@ -1290,7 +1290,7 @@
   return wrap(unwrap(Builder)->createGlobalVariableExpression(
   unwrapDI(Scope), {Name, NameLen}, {Linkage, LinkLen},
   unwrapDI(File), LineNo, unwrapDI(Ty), LocalToUnit,
-  unwrap(Expr), unwrapDI(Decl),
+  true, unwrap(Expr), unwrapDI(Decl),
   nullptr, AlignInBits));
 }
 
Index: llvm/lib/IR/DIBuilder.cpp
===
--- llvm/lib/IR/DIBuilder.cpp
+++ llvm/lib/IR/DIBuilder.cpp
@@ -639,13 +639,14 @@
 
 DIGlobalVariableExpression *DIBuilder::createGlobalVariableExpression(
 DIScope *Context, StringRef Name, StringRef LinkageName, DIFile *F,
-unsigned LineNumber, DIType *Ty, bool isLocalToUnit, DIExpression *Expr,
+unsigned LineNumber, DIType *Ty, bool isLocalToUnit,
+bool isDefined, DIExpression *Expr,
 MDNode *Decl, MDTuple *templateParams, uint32_t AlignInBits) {
   checkGlobalVariableScope(Context);
 
   auto *GV = DIGlobalVariable::getDistinct(
   VMContext, cast_or_null(Context), Name, LinkageName, F,
-  LineNumber, Ty, isLocalToUnit, true, cast_or_null(Decl),
+  LineNumber, Ty, isLocalToUnit, isDefined, cast_or_null(Decl),
   templateParams, AlignInBits);
   if (!Expr)
 Expr = createExpression();
Index: llvm/include/llvm/IR/DIBuilder.h
===
--- llvm/include/llvm/IR/DIBuilder.h
+++ llvm/include/llvm/IR/DIBuilder.h
@@ -581,7 +581,7 @@
 ///specified)
 DIGlobalVariableExpression *createGlobalVariableExpression(
 DIScope *Context, StringRef Name, StringRef LinkageName, DIFile *File,
-unsigned LineNo, DIType *Ty, bool isLocalToUnit,
+unsigned LineNo, DIType *Ty, bool isLocalToUnit, bool isDefined = true,
 DIExpression *Expr = nullptr, MDNode *Decl = nullptr,
 MDTuple *templateParams = nullptr,