[PATCH] D50968: [AST] make a static local variable in a fuction hidden by -fvisibility-inlines-hidden visible

[Hiroshi Inoue via Phabricator via llvm-commits]

inouehrs added inline comments.


================
Comment at: lib/AST/Decl.cpp:1272
+      assert(cast<VarDecl>(D)->isStaticLocal());
+      return LinkageInfo(VisibleNoLinkage, DefaultVisibility, false);
+    }
----------------
rnk wrote:
> jsji wrote:
> > Is it overreaction here if we always return "DefaultVisibility"? Is it possible that the visibility of this static was already hidden without -fvisibility-inlines-hidden?
> > 
> > Maybe we should consider avoid calling mergeVisibility for local static only?
> I think this incorrectly gives `x` default visibility when the hidden visibility attribute is explicit and compiling with -fvisibility-inlines-hidden, as in the example below:
> ```
> inline int *__attribute__((visibility("hidden"))) f() {
>   static int x;
>   return &x;
> }
> ```
> 
> You should add this test and confirm that we behave as GCC does.
> 
> You should also consider cases where the attribute is explicitly put on the variable itself and make sure that is handled.
As long as I tested, the behavior with this patch is consistent with the behavior of gcc-8.1.


================
Comment at: lib/AST/Decl.cpp:1275
+
     LV = getLVForDecl(FD, computation);
   }
----------------
inouehrs wrote:
> rnk wrote:
> > I think the right way to do this is to compute the linkage & visibility of FD, as is done here, check if the visibility is explicit, and if not, under the same conditions that you have above, upgrade the visibility to default.
> I agree. I will try to fix this. Thanks.
I cannot use `LV.isVisibilityExplicit()` for this check because the hidden visibility by -fvisibility-inlines-hidden is considered as explicit. I use `getExplicitVisibility` instead.


https://reviews.llvm.org/D50968