This is an archive of the discontinued LLVM Phabricator instance.

Add missing visibility annotation for __base
ClosedPublic

Authored by yunlian on Jun 27 2018, 4:19 PM.

Details

Summary

This adds missing visibility annotation for __base.

Diff Detail

Event Timeline

yunlian created this revision.Jun 27 2018, 4:19 PM

Sorry - what problem is this solving?

pcc added a comment.EditedJun 27 2018, 4:46 PM

This change ensures that __base 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 __base 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 __base and the calls cannot be devirtualized.

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.

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?

More specifically, the following program always causes the vtable for __base<int(float)> to be internal:

cat <<EOF | clang++ -xc++ - -std=c++11 && nm --demangle a.out
#include <functional>

int go(float) { return 0; }
std::function<int(float)> 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
0000000100003178 s vtable for std::__1::__function::__base<int (float)>
00000001000030f0 s vtable for std::__1::__function::__func<int (*)(float), std::__1::allocator<int (*)(float)>, int (float)>
0000000100003218 s vtable for std::__1::bad_function_call
...
pcc added a comment.Jun 4 2019, 12:56 PM

Here's a standalone reproducer for the problem:

$ cat exe.cpp
#include <functional>

std::function<void()> f();

int main() {
  f()();
}
$ cat dso.cpp
#include <functional>

__attribute__((visibility("default"))) std::function<void()> 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]

@ldionne Does Peter's example answer your questions?

EricWF added a subscriber: EricWF.Jun 25 2019, 2:25 AM

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.

pcc added a comment.Jun 25 2019, 8:36 AM

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

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

pcc added a comment.Jul 16 2019, 10:51 AM

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

ldionne requested changes to this revision.Jul 16 2019, 10:55 AM

@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.

This revision now requires changes to proceed.Jul 16 2019, 10:55 AM
pcc added a comment.Jul 16 2019, 11:08 AM

@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.

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.

ldionne accepted this revision.Mar 18 2020, 1:49 PM
This revision is now accepted and ready to land.Mar 18 2020, 1:49 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 18 2020, 2:41 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Thanks a lot!