This is an archive of the discontinued LLVM Phabricator instance.

[Lexer] Add udefined_behavior_sanitizer feature
ClosedPublic

Authored by leonardchan on Sep 21 2018, 5:49 PM.

Details

Summary

This can be used to detect whether the code is being built with UBSan using the __has_feature(undefined_behavior_sanitizer) predicate.

Diff Detail

Repository
rC Clang

Event Timeline

leonardchan created this revision.Sep 21 2018, 5:49 PM
phosek accepted this revision.Sep 21 2018, 5:54 PM

LGTM but you should probably a wait a day to see if sanitizer owners are fine with with this as well.

This revision is now accepted and ready to land.Sep 21 2018, 5:54 PM
vitalybuka accepted this revision.Sep 21 2018, 5:59 PM
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.
rsmith added a subscriber: rsmith.Oct 3 2018, 4:20 PM

I'm not at all convinced that this is a good thing. There isn't such a thing as "undefined behavior sanitizer". Rather, there are a whole bunch of different checks that fall under the same umbrella. This test seems on the surface to be about as meaningless as __has_feature(warnings) would be: it's useless to ask the question without knowing *which* warnings you're talking about. But perhaps there's some use case I've overlooked (and your description of the patch doesn't mention why you want this). What is the use case you're trying to address with this change?

If we want to change this, you'll also need to update getPPTransparentSanitizers to exclude all the UBSan checks, because it's no longer the case that those sanitizers only affect code generation.

I'm not at all convinced that this is a good thing. There isn't such a thing as "undefined behavior sanitizer". Rather, there are a whole bunch of different checks that fall under the same umbrella. This test seems on the surface to be about as meaningless as __has_feature(warnings) would be: it's useless to ask the question without knowing *which* warnings you're talking about. But perhaps there's some use case I've overlooked (and your description of the patch doesn't mention why you want this). What is the use case you're trying to address with this change?

This is part of enabling UBSan for Zircon (the Fuchsia kernel) (https://fuchsia-review.googlesource.com/c/zircon/+/197017). We enable UBSan when building musl libc, but libc is dynamically linked first before sanitizer runtimes, so we need to stub them out in the beginning before the UBSan runtime is linked. This __has_feature is there to check if we build with -fsanitize=undefined since we only want to define these stubs if libc is built with UBSan.

If we want to change this, you'll also need to update getPPTransparentSanitizers to exclude all the UBSan checks, because it's no longer the case that those sanitizers only affect code generation.

I can update this in another patch.

rsmith added a comment.Oct 3 2018, 5:03 PM

I'm not at all convinced that this is a good thing. There isn't such a thing as "undefined behavior sanitizer". Rather, there are a whole bunch of different checks that fall under the same umbrella. This test seems on the surface to be about as meaningless as __has_feature(warnings) would be: it's useless to ask the question without knowing *which* warnings you're talking about. But perhaps there's some use case I've overlooked (and your description of the patch doesn't mention why you want this). What is the use case you're trying to address with this change?

This is part of enabling UBSan for Zircon (the Fuchsia kernel) (https://fuchsia-review.googlesource.com/c/zircon/+/197017). We enable UBSan when building musl libc, but libc is dynamically linked first before sanitizer runtimes, so we need to stub them out in the beginning before the UBSan runtime is linked. This __has_feature is there to check if we build with -fsanitize=undefined since we only want to define these stubs if libc is built with UBSan.

This sounds like something that would be better handled in the build system rather than in source code. In particular, I think you don't actually want to detect "is this translation unit being compiled with ubsan enabled?", you instead want to detect "is the rest of musl libc being compiled with ubsan enabled?" -- you should not compile the stubs themselves with ubsan enabled, because ubsan might insert calls to its runtime at arbitrary places within that translation unit, which means you'll get infinite recursion when one of the ubsan callbacks ends up calling itself.

This sounds like something that would be better handled in the build system rather than in source code. In particular, I think you don't actually want to detect "is this translation unit being compiled with ubsan enabled?", you instead want to detect "is the rest of musl libc being compiled with ubsan enabled?" -- you should not compile the stubs themselves with ubsan enabled, because ubsan might insert calls to its runtime at arbitrary places within that translation unit, which means you'll get infinite recursion when one of the ubsan callbacks ends up calling itself.

UBSan doesn't actually instrument these stubs though. We have them initially defined to be weakly linked and just call __builtin_trap(), which is be replaced when the UBSan runtime eventually gets linked when compiling something that uses this libc.

I also imagine it would be convenient for there to be some __has_feature to detect if any of the UBSan checks are being used.

This is needed in the NetBSD kernel, more fine-grained checks would be acceptable too, but one global feature detection is what I need.

Herald added a project: Restricted Project. · View Herald TranscriptSep 21 2019, 4:03 AM
thakis added a subscriber: thakis.Feb 23 2021, 5:51 PM

Looks like this landed in b32d40417e25bad79e9b5bc0f0a29d7ba0222ad9 but the review feedback was never addressed.

With a more fine-grained thing, we could use this to add a compile-time error to libcxxabi for D97323.