[PATCH] D128223: [clang] Cached linkage assertion for static locals of static function

[David Tenty via Phabricator via cfe-commits]

daltenty accepted this revision.
daltenty added a comment.

New revision LGTM, VisibleNoLinkage is typical reserved for types from inline 
functions so it doesn't seem sensible to return. Looking at the C++ standard, 
I'm not even convinced this is guaranteed to be the same object if the function 
doesn't have external linkage, so I think the guard is sensible.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128223

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

[PATCH] D128223: [clang] Cached linkage assertion for static locals of static function

[Chris Bowler via Phabricator via cfe-commits]

cebowleratibm added a comment.

I believe the fix for a52d151f9dde7 inadvertently exposed a code path where by 
the linkage of a static local of a static function, which would otherwise 
return LinkageInfo::none() may now return VisibleNoLinkage depending on the 
incoming computation argument.

I don't think a static local of a function with no linkage should ever return 
VisibleNoLinkage so I've added the appropriate guard such that the linkage 
evaluation with return no linkage regardless of the computation argument.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128223

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

[PATCH] D128223: [clang] Cached linkage assertion for static locals of static function

[Chris Bowler via Phabricator via cfe-commits]

cebowleratibm updated this revision to Diff 473756.
cebowleratibm retitled this revision from "[clang] Linkage computation of 
static locals may require forcing visibility computation" to "[clang] Cached 
linkage assertion for static locals of static function".
cebowleratibm edited the summary of this revision.
This revision is now accepted and ready to land.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128223

Files:
  clang/lib/AST/Decl.cpp
  clang/test/CodeGenCXX/linkage-static-local-crash.cpp


Index: clang/test/CodeGenCXX/linkage-static-local-crash.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/linkage-static-local-crash.cpp
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 -triple powerpc-ibm-aix 
-mdefault-visibility-export-mapping=explicit -fvisibility-inlines-hidden 
-emit-llvm %s -o - | FileCheck %s
+
+struct C {
+  template  
+  __attribute__((__visibility__("hidden"))) 
+  static int& f() {
+static int g = 42;
+return g;
+  }
+};
+
+template
+void foo(T i) { C::f(); }
+
+void bar() {
+  foo([]{});
+}
+
+// CHECK: @"_ZZN1C1fIZ3barvE3$_0EERivE1g" = internal global i32 42, align 4
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -1368,7 +1368,8 @@
 // is not explicitly attributed as a hidden function,
 // we should not make static local variables in the function hidden.
 LV = getLVForDecl(FD, computation);
-if (isa(D) && useInlineVisibilityHidden(FD) &&
+if (isExternallyVisible(LV.getLinkage()) &&
+isa(D) && useInlineVisibilityHidden(FD) &&
 !LV.isVisibilityExplicit() &&
 !Context.getLangOpts().VisibilityInlinesHiddenStaticLocalVar) {
   assert(cast(D)->isStaticLocal());


Index: clang/test/CodeGenCXX/linkage-static-local-crash.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/linkage-static-local-crash.cpp
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 -triple powerpc-ibm-aix -mdefault-visibility-export-mapping=explicit -fvisibility-inlines-hidden -emit-llvm %s -o - | FileCheck %s
+
+struct C {
+  template  
+  __attribute__((__visibility__("hidden"))) 
+  static int& f() {
+static int g = 42;
+return g;
+  }
+};
+
+template
+void foo(T i) { C::f(); }
+
+void bar() {
+  foo([]{});
+}
+
+// CHECK: @"_ZZN1C1fIZ3barvE3$_0EERivE1g" = internal global i32 42, align 4
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -1368,7 +1368,8 @@
 // is not explicitly attributed as a hidden function,
 // we should not make static local variables in the function hidden.
 LV = getLVForDecl(FD, computation);
-if (isa(D) && useInlineVisibilityHidden(FD) &&
+if (isExternallyVisible(LV.getLinkage()) &&
+isa(D) && useInlineVisibilityHidden(FD) &&
 !LV.isVisibilityExplicit() &&
 !Context.getLangOpts().VisibilityInlinesHiddenStaticLocalVar) {
   assert(cast(D)->isStaticLocal());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits