This is an archive of the discontinued LLVM Phabricator instance.

reland "[DebugInfo] Support to emit debugInfo for extern variables"
ClosedPublic

Authored by yonghong-song on Dec 22 2019, 2:46 PM.

Details

Summary

Commit d77ae1552fc21a9f3877f3ed7e13d631f517c825
("[DebugInfo] Support to emit debugInfo for extern variables")
added deebugInfo for extern variables for BPF target.
The commit is reverted by 891e25b02d760d0de18c7d46947913b3166047e7
as the committed tests using %clang instead of %clang_cc1 causing
test failed in certain scenarios as reported by Reid Kleckner.

This patch fixed the tests by using %clang_cc1.

Diff Detail

Event Timeline

yonghong-song created this revision.Dec 22 2019, 2:46 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptDec 22 2019, 2:46 PM
yonghong-song added a comment.EditedDec 22 2019, 2:50 PM

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 D70696 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 only at linux now.
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.

rnk added inline comments.Dec 22 2019, 2:52 PM
clang/test/CodeGen/debug-info-extern-basic.c
2

Clang does not require a registered target to emit LLVM IR for a target. Please remove this, so that these tests actually run and cover the code under test in most configurations.

yonghong-song marked an inline comment as done.Dec 22 2019, 2:55 PM
yonghong-song added inline comments.
clang/test/CodeGen/debug-info-extern-basic.c
2

Let me try.

Removed REQUIRES: bpf-registered-target. This is not needed any more if using %clang_cc1. Tested by build clang with and without BPF target.

ast accepted this revision.Dec 22 2019, 5:36 PM
This revision is now accepted and ready to land.Dec 22 2019, 5:36 PM
This revision was automatically updated to reflect the committed changes.
rnk added a comment.Dec 22 2019, 6:34 PM

I patched this in, and the tests don't pass for me locally, so we're back to where we started. Go ahead and land it, I guess. I'm kind of not happy that we had to poke another virtual method through the ASTConsumer interface to make this happen.

@rnk I somehow cannot reproduce the issue locally. If you can let me know how to exactly reproduce the issue, I might be able to also help with some kind of solution, although frankly speaking, I am not a clang expert.

+@rsmith @rjmccall
For more meaningful feedback on the code structure.

clang/include/clang/Sema/Sema.h
671–672

This name seems a bit vague. This is a member of Sema, essentially a global variable, for this very corner-case feature (BPF). I'm not convinced that you couldn't have done this without changing Sema.

rnk added a comment.Dec 22 2019, 6:55 PM

You forgot to update MultiplexConsumer:

diff --git a/clang/include/clang/Frontend/MultiplexConsumer.h b/clang/include/clang/Frontend/MultiplexConsumer.h
index ca6ed8310ae..3054e184281 100644
--- a/clang/include/clang/Frontend/MultiplexConsumer.h
+++ b/clang/include/clang/Frontend/MultiplexConsumer.h
@@ -65,6 +65,7 @@ public:
   void HandleTopLevelDeclInObjCContainer(DeclGroupRef D) override;
   void HandleImplicitImportDecl(ImportDecl *D) override;
   void CompleteTentativeDefinition(VarDecl *D) override;
+  void CompleteExternalDeclaration(VarDecl *D) override;
   void AssignInheritanceModel(CXXRecordDecl *RD) override;
   void HandleVTable(CXXRecordDecl *RD) override;
   ASTMutationListener *GetASTMutationListener() override;
diff --git a/clang/lib/Frontend/MultiplexConsumer.cpp b/clang/lib/Frontend/MultiplexConsumer.cpp
index 04e896296c9..5abbb3a235b 100644
--- a/clang/lib/Frontend/MultiplexConsumer.cpp
+++ b/clang/lib/Frontend/MultiplexConsumer.cpp
@@ -322,6 +322,11 @@ void MultiplexConsumer::CompleteTentativeDefinition(VarDecl *D) {
     Consumer->CompleteTentativeDefinition(D);
 }
 
+void MultiplexConsumer::CompleteExternalDeclaration(VarDecl *D) {
+  for (auto &Consumer : Consumers)
+    Consumer->CompleteExternalDeclaration(D);
+}
+
 void MultiplexConsumer::AssignInheritanceModel(CXXRecordDecl *RD) {
   for (auto &Consumer : Consumers)
     Consumer->AssignInheritanceModel(RD);

That seems to fix this. I'm curious why my build of clang uses this.

rnk added a comment.Dec 22 2019, 7:00 PM

Ah, I see, these tests will fail if you link in some clang plugins, which I do for Chrome. So, this only affects me. Anyway, I'll fix it.

@rnk Great to know the reason. Thanks!