[PATCH] D48680: Add missing visibility annotation for __base

[Manoj Gupta via Phabricator via cfe-commits]

manojgupta added a comment.

Thanks a lot!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D48680



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

[PATCH] D48680: Add missing visibility annotation for __base

[Louis Dionne via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG30ccc2e8d24b: [libc++] Add missing visibility annotation for 
__base (authored by yunlian, committed by ldionne).
Herald added a project: libc++.
Herald added a reviewer: libc++.

Changed prior to commit:
  https://reviews.llvm.org/D48680?vs=153198=251175#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D48680

Files:
  libcxx/include/functional


Index: libcxx/include/functional
===
--- libcxx/include/functional
+++ libcxx/include/functional
@@ -1618,7 +1618,7 @@
 
 // __base provides an abstract interface for copyable functors.
 
-template class __base;
+template class _LIBCPP_TEMPLATE_VIS __base;
 
 template
 class __base<_Rp(_ArgTypes...)>


Index: libcxx/include/functional
===
--- libcxx/include/functional
+++ libcxx/include/functional
@@ -1618,7 +1618,7 @@
 
 // __base provides an abstract interface for copyable functors.
 
-template class __base;
+template class _LIBCPP_TEMPLATE_VIS __base;
 
 template
 class __base<_Rp(_ArgTypes...)>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48680: Add missing visibility annotation for __base

[Manoj Gupta via Phabricator via cfe-commits]

manojgupta added a comment.

Friendly ping  @pcc and @ldionne . We have been carrying this patch I think for 
too long now.
Also, we have not discovered any LTO issues elsewhere so can't tell from our 
side if there are other places in libc++ that need visibility annotations.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D48680



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

[PATCH] D48680: Add missing visibility annotation for __base

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

In D48680#1587967 , @ldionne wrote:

> @pcc In your reproducer, what is `~/l3/ra-cmake/bin/clang++`?


That's just clang built from trunk at the time that I posted my comment.

> Are you able to reproduce without `-fuse-ld=lld`? I'm trying to reproduce 
> locally but I can't.

On which platform? On Linux lld (or gold) is needed in order to use LTO which 
is a requirement for CFI.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D48680



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

[PATCH] D48680: Add missing visibility annotation for __base

[Louis Dionne via Phabricator via cfe-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.
Herald added a subscriber: dexonsmith.

@pcc In your reproducer, what is `~/l3/ra-cmake/bin/clang++`? Are you able to 
reproduce without `-fuse-ld=lld`? I'm trying to reproduce locally but I can't.

I know this is a small change, but I'd like to understand why it's needed since 
I suspect it may uncover other places where visibility is wrong -- if the 
problem is really on libc++'s side.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D48680



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

[PATCH] D48680: Add missing visibility annotation for __base

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

I think it would be up to the libc++ maintainers to approve the patch.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D48680



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

[PATCH] D48680: Add missing visibility annotation for __base

[Manoj Gupta via Phabricator via cfe-commits]

manojgupta added a comment.

@pcc can you please submit this patch if there are no objections?


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D48680



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

[PATCH] D48680: Add missing visibility annotation for __base

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

The same problem occurs whether or not `exe.cpp` is compiled 
with`-fno-exceptions -fno-rtti`.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D48680



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

[PATCH] D48680: Add missing visibility annotation for __base

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added a comment.

You have an ODR violation. You can't compile one TU with `-fno-rtti 
-fno-exceptions` and another without. You give the definitions of `__base` 
different vtables.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D48680



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

[PATCH] D48680: Add missing visibility annotation for __base

[Manoj Gupta via Phabricator via cfe-commits]

manojgupta added a comment.

@ldionne Does Peter's example answer your questions?


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D48680



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

[PATCH] D48680: Add missing visibility annotation for __base

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

Here's a standalone reproducer for the problem:

  $ cat exe.cpp
  #include 
  
  std::function f();
  
  int main() {
f()();
  }
  $ cat dso.cpp
  #include 
  
  __attribute__((visibility("default"))) std::function f() {
return [](){};
  }
  $ ~/l3/ra-cmake/bin/clang++ -fsanitize=cfi -fvisibility=hidden dso.cpp 
-shared -flto -fuse-ld=lld -Wl,-soname,dso.so -o dso.so -stdlib=libc++ 
-fno-rtti -fno-exceptions
  $ ~/l3/ra-cmake/bin/clang++ -fsanitize=cfi -fvisibility=hidden exe.cpp  -flto 
-fuse-ld=lld dso.so -stdlib=libc++
  $ LD_LIBRARY_PATH=.:$HOME/l3/ra-cmake/lib ./a.out 
  Illegal instruction

With this patch:

  $ LD_LIBRARY_PATH=.:$HOME/l3/ra-cmake/lib ./a.out 
  [no output]


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D48680



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

[PATCH] D48680: Add missing visibility annotation for __base

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

More specifically, the following program always causes the vtable for 
`__base` to be internal:

  cat <
  
  int go(float) { return 0; }
  std::function foo() { return go; }
  
  int main() { foo()(3.3f); }
  EOF

That's true with or without this patch, and with or without 
`-fvisibility=hidden`. We always get:

  ...
   U vtable for __cxxabiv1::__si_class_type_info
  00013178 s vtable for std::__1::__function::__base
  000130f0 s vtable for std::__1::__function::__func, int (float)>
  00013218 s vtable for std::__1::bad_function_call
  ...


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D48680



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

[PATCH] D48680: Add missing visibility annotation for __base

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

What I don't understand is under which circumstances this changes anything, 
since we don't export `__base` from the dylib, and implicit instantiations of 
`__base` don't cause it to be exported. Can you please provide a sample program 
where this changes what's exported, or the LTO visibility you're mentioning?


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D48680



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

[PATCH] D48680: Add missing visibility annotation for __base

[Manoj Gupta via Phabricator via cfe-commits]

manojgupta added a comment.
Herald added a subscriber: libcxx-commits.

Hi Peter and Marshall,

Yunlian has moved to a different project. Can you let me know what is missing 
in this patch so that it can be submitted.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D48680



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

[PATCH] D48680: Add missing visibility annotation for __base

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

This change ensures that __func receives public LTO visibilty:
https://clang.llvm.org/docs/LTOVisibility.html
if a translation unit is compiled with `-fvisibility=hidden` and without 
`_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS` defined (i.e. dynamically linking 
against libc++).

What that means is that the control flow integrity and whole-program 
devirtualization features are disabled on the virtual calls on __func that are 
used to implement operator() on std::function. Enabling those features would be 
incorrect because libc++ is being linked dynamically and therefore the compiler 
does not have full visibility of all derived classes of __func and the calls 
cannot be devirtualized.


Repository:
  rCXX libc++

https://reviews.llvm.org/D48680



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

[PATCH] D48680: Add missing visibility annotation for __base

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

Sorry - what problem is this solving?


Repository:
  rCXX libc++

https://reviews.llvm.org/D48680



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

[PATCH] D48680: Add missing visibility annotation for __base

[Yunlian Jiang via Phabricator via cfe-commits]

yunlian created this revision.
Herald added subscribers: cfe-commits, ldionne.

This adds missing visibility annotation for __base.


Repository:
  rCXX libc++

https://reviews.llvm.org/D48680

Files:
  include/functional


Index: include/functional
===
--- include/functional
+++ include/functional
@@ -1458,7 +1458,7 @@
 
 namespace __function {
 
-template class __base;
+template class _LIBCPP_TEMPLATE_VIS __base;
 
 template
 class __base<_Rp(_ArgTypes...)>


Index: include/functional
===
--- include/functional
+++ include/functional
@@ -1458,7 +1458,7 @@
 
 namespace __function {
 
-template class __base;
+template class _LIBCPP_TEMPLATE_VIS __base;
 
 template
 class __base<_Rp(_ArgTypes...)>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits