[PATCH] D33216: Generate ubsan shared libraries.

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
Comment at: lib/ubsan/CMakeLists.txt:174
+      ARCHS ${UBSAN_SUPPORTED_ARCH}
+      OBJECT_LIBS ${UBSAN_COMMON_RUNTIME_OBJECT_LIBS}
+      RTUbsan_cxx
----------------
aoli wrote:
> vsk wrote:
> > aoli wrote:
> > > vsk wrote:
> > > > Here, it seems like you'd just want 'RTInterception', and not all of '${UBSAN_COMMON_RUNTIME_OBJECT_LIBS}'.
> > > Oh, RTInterception was added by accident. I think I'm not using it either. Shall I keep share libraries' `OBJECT_LIBS` just the same as those for static libraries?
> > I think it's a good idea to keep the OBJECT_LIBS as close to possible as those for static libraries. Could you double-check that RTInterception isn't needed in just this specific case? I do see some Windows-specific code in lib/ubsan that sets up interceptors.
> I just checked again. To compile `clang_rt.ubsan_standalone_cxx` it also requires `RTUbsan`, `RTSanitizerCommon` and `RTSanitizerCommonLibc` but not `RTInterception`. I'd prefer to have only one `clang_rt.ubsan_standalone` which includes  `RTUbsan`, `RTUbsan_cxx ,`RTSanitizerCommon` and `RTSanitizerCommonLibc` now(which is the same as my first change). Because `clang_rt.ubsan_standalone_cxx` will include all libraries in `clang_rt.ubsan_standalone`.
> 
> To find the reference to `RTInterception`, I first compiled the shared library without `RTInterception` on my machine and it doesn't generate any error. Then I grepped several APIs shown in interception_win.h and interception_mac.h and it didn't return any result so I suppose the `RTInterception` is not being used in ubsan. The only thing I found was `sanitizer_win_weak_interception.h` which was actually included in `RTSanitizerCommon`. I'm new to LLVM and not quiet familiar with the source code and I'm not sure if I'm doing right.
> I just checked again. To compile clang_rt.ubsan_standalone_cxx it also requires RTUbsan, RTSanitizerCommon and RTSanitizerCommonLibc but not RTInterception.

This makes sense, looking at RTUbsan_cxx, it's just RTUbsan plus some source files which implement C++ specific checks.

> I'd prefer to have only one clang_rt.ubsan_standalone which includes  RTUbsan, RTUbsan_cxx ,RTSanitizerCommon` and RTSanitizerCommonLibc now (which is the same as my first change).

I don't think clang_rt.ubsan_standalone is meant to include any C++ specific checks. I think this could matter in practice because -fsanitize=vptr needs to find some typeinfo objects from libcxxabi, or at least something that looks like it.

> To find the reference to RTInterception, I first compiled the shared library without RTInterception on my machine and it doesn't generate any error. Then I grepped several APIs shown in interception_win.h and interception_mac.h and it didn't return any result so I suppose the RTInterception is not being used in ubsan. The only thing I found was sanitizer_win_weak_interception.h which was actually included in RTSanitizerCommon.

Perfect, thanks for checking. The version of your patch we've got right now LGTM, after we remove the RTInterception dep.

> I'm new to LLVM and not quiet familiar with the source code and I'm not sure if I'm doing right.

Welcome! You're doing it right.


https://reviews.llvm.org/D33216