[PATCH] D53153: [OpenCL] Mark kernel functions with default visibility

[Scott Linder via Phabricator via cfe-commits]

scott.linder added a comment.

In D53153#1355718 <https://reviews.llvm.org/D53153#1355718>, @rjmccall wrote:

> In D53153#1353256 <https://reviews.llvm.org/D53153#1353256>, @scott.linder wrote:
>
> > In D53153#1317977 <https://reviews.llvm.org/D53153#1317977>, @rjmccall wrote:
> >
> > > I think `-fvisibility=hidden` isn't good enough because you want to infer hidden visibility even for external symbol references, and that's just not how global visibility works.  But after this discussion, I'm prepared to accept that (1) we should have some sort of single-image compiler mode that implies that all symbols are defined within the image and (2) you can make your toolchain imply that together with `-fvisibility=hidden` and then have specific symbols opt in to non-hidden visibility if they need to be accessible to whatever loader / runtime you have.
> >
> >
> > It seems that explicit visibility attributes on external symbol references (e.g. `__attribute__((visibility("hidden"))) extern int foo;`) are respected in Clang, so I don't understand the rationale for global visibility controls not applying to them as well. Can you describe why this is the case?
>
>
> Well, one answer is that that's the behavior that GCC defined when they added global visibility controls, and it would be unreasonable for Clang to deviate in such a fundamental way.  However, we do deviate in some other ways in our interpretation of visibility attributes, so that's not a totally satisfactory answer.
>
> The stronger answer is that GCC has a very good reason for this behavior.  Global visibility controls apply to all code in the translation unit.  While programmers often mentally distinguish between "project" headers (which they control) and "library" headers (which they do not), that is not a difference that is known to the compiler.  Therefore, if global visibility controls applied to external symbols, the compiler would end up assuming that all external symbols not annotated as `default` are provided by the current image.  That assumption would have been incompatible with essentially all existing headers for dynamic libraries on GCC's core targets, including system headers; in other words, it would have caused ubiquitous miscompiles.*  It would be completely unreasonable to expect all those headers to be changed just for the benefit of a single new compiler feature.  Therefore, global visibility controls do not apply to external symbols because doing so would make it impossible to actually enable global visibility controls.
>
> - GCC's rule helps a great deal even in C++, which is generally more sensitive to global visibility: you can typically use an unannotated C++ header under `-fvisibility=hidden` and have no problems unless you start using its types as exceptions (and GCC tries to work around that, too) or comparing the addresses of inline functions or template instantiations.
>
>   John.


Thank you for the context, that makes sense.

In terms of implementation of the above, currently at the AST level the LinkageComputer seems to assign global visibility to extern declarations, then during CodeGen this is simply not copied to the IR level unless the visibility was set explicitly. At first I was confused that the visibility of e.g. a `clang::Decl` and its `llvm::GlobalValue` could differ. Is there a reason why the implementation of LinkageComputer does not calculate the correct visibility here from the start? If the logic for extern declarations can be moved into the AST would it then be reasonable to add a distinct global visibility for extern declarations (something like `-fextern-visibility=`)? We could pair this with the existing global value visibility to effectively get the single-image compilation mode.


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

https://reviews.llvm.org/D53153