[PATCH] D33216: Generate ubsan shared libraries.

[Filipe Cabecinhas via Phabricator via llvm-commits]

filcab added inline comments.


================
Comment at: lib/ubsan/CMakeLists.txt:170
+      PARENT_TARGET ubsan)
+
     if (UNIX)
----------------
vsk wrote:
> aoli wrote:
> > vsk wrote:
> > > I'm not familiar with the differences between RTUbsan_cxx and its C variant, but for consistency with what's already here, should we have 'SHARED' versions of clang_rt.ubsan_standalone and clang_rt.ubsan_standalone_cxx?
> > > 
> > > Also: do you know whether the ubsan standalone runtimes are tested in a `check-ubsan` test run?
> > I've updated the CMakeList.txt which generates both clang_rt.ubsan_standalone and clang_rt.ubsan_standalone_cxx now. It seems that the ubsan standalone runtimes are tested in the test run. https://github.com/llvm-mirror/compiler-rt/blob/master/test/ubsan/CMakeLists.txt#L26 
> Hm, yes, but I've just now noticed that this doesn't appear to do the right thing, at least on my system.
> 
> For example, I can run a standalone ubsan test like this:
> `./bin/llvm-lit projects/compiler-rt/test/ubsan/Standalone-x86_64/TestCases/Misc/bool.cpp -a`
> 
> Inspecting the link command, I see:
> `"/Volumes/Builds/llvm.org-ubsan-arith-DA/lib/clang/5.0.0/lib/darwin/libclang_rt.ubsan_osx_dynamic.dylib"`
> 
> So it's not using the standalone lib at all? I filed a bug: https://bugs.llvm.org/show_bug.cgi?id=33059. I don't think this should hold up your patch, but missing test coverage is always spooky.
That looks like the standalone lib (versus the ASan/TSan/etc lib which include ubsan).
You're testing on macOS, though, which already has its own build of a standalone ubsan library, separate from this part of the makefile, which is for `!APPLE` (check lines 47 and 83).


================
Comment at: lib/ubsan/CMakeLists.txt:42
+append_list_if(COMPILER_RT_HAS_LIBM m UBSAN_DYNAMIC_LIBS)
+append_list_if(COMPILER_RT_HAS_LIBPTHREAD pthread UBSAN_DYNAMIC_LIBS)
+append_list_if(COMPILER_RT_HAS_LIBSTDCXX stdc++ UBSAN_DYNAMIC_LIBS)
----------------
Are all of these needed?


================
Comment at: lib/ubsan/CMakeLists.txt:43
+append_list_if(COMPILER_RT_HAS_LIBPTHREAD pthread UBSAN_DYNAMIC_LIBS)
+append_list_if(COMPILER_RT_HAS_LIBSTDCXX stdc++ UBSAN_DYNAMIC_LIBS)
+ 
----------------
I wouldn't expect the non-c++ version of the lib to bring libstdc++ with it.


https://reviews.llvm.org/D33216