847350 - Add/Update MOZ_ASAN/TSAN_BLACKLIST macros to annotate functions

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Christian Holler (:decoder)]

In order to make TSan ignore functions during instrumentation (compile time), we currently have to use the sanitize blacklist (-fsanitize-blacklist="blacklist file"). However, we need a macro to annotate functions anyway for two reasons

1) Functions that are marked this way must not be inlined (MOZ_NEVER_INLINE)
2) Soon, blacklisting will be done using an attribute (like ASan currently uses __attribute__((no_address_safety_analysis)) right now) instead of a file.

Patch coming soon.

Comment 1

[Christian Holler (:decoder)]

Here's a link to the discussion for the sanitizer attributes on Clang.

Comment 2

[Christian Holler (:decoder)]

Attachment: Patch

This patch adds the macro itself, a necessary configure switch to control it and also a preliminary blacklist file (new file) that will be used to store all the blacklist entries until this can be replaced with the compiler attribute. Until that, the annotation has to be used together with -fsanitize-blacklist to work.

Comment 3

[(not currently active) Ted Mielczarek]

Comment on attachment 721759 [details] [diff] [review]
Patch

Review of attachment 721759 [details] [diff] [review]:
-----------------------------------------------------------------

You should get a MFBT peer to sign off on the MFBT change. The build stuff looks fine.

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 721759 [details] [diff] [review]
Patch

Review of attachment 721759 [details] [diff] [review]:
-----------------------------------------------------------------

Seems reasonable.

::: mfbt/Attributes.h
@@ +164,5 @@
> + * instrumentation shipped with Clang) to not instrument the annotated function.
> + * Right now, it only prevents the compiler from inlining the function (since
> + * inlining breaks the blacklisting mechanism) and blacklisting has to be done
> + * with an external file, but in the future, an attribute like AddressSanitizer
> + * has one will be used here.

For the last bit, do you mean "an attribute like __attribute__((no_thread_safety_analysis)) will be used as soon as Clang supports one" or something like that?  This doesn't parse right now.  :-)

Comment 5

[Christian Holler (:decoder)]

The attribute has changed since this patch was made (both for ASan and TSan, and the old ones are deprecated). I'll come up with a new patch that updates the flags. The patch will also use a new approach using __has_feature instead of requiring MOZ_TSAN through configure.in at all. Furthermore, the same can be done for MOZ_ASAN_BLACKLIST, so the ASan annotations will not be used if the compiler doesn't understand them (this is right now the case when building on Windows for ASan where ASan is a post-build step, --enable-address-sanitizer causes breakage there).

I will also not land a blacklist with this patch, because for the compile-time blacklisting, the annotations are sufficient now (at least on Clang trunk).

Comment 6

[Christian Holler (:decoder)]

Attachment: asan-tsan-attr.patch

Waldo, can you re-review this simple patch, since it changed quite a bit since the last patch? With all the configure.in bits and the blacklist gone, your review should be sufficient :) Thanks!

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 818306 [details] [diff] [review]
asan-tsan-attr.patch

Review of attachment 818306 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/Attributes.h
@@ +175,5 @@
> +# if __has_feature(address_sanitizer)
> +#  undef MOZ_ASAN_BLACKLIST
> +#  define MOZ_ASAN_BLACKLIST MOZ_NEVER_INLINE __attribute__((no_sanitize_address))
> +# endif
> +#endif

I'd rather see

#if defined(__has_feature)
#  if __has_feature(address_sanitizer)
#    define MOZ_ASAN_BLACKLIST MOZ_NEVER_INLINE __attribute__((no_sanitize_address))
#  else
#    define MOZ_ASAN_BLACKLIST /* nothing */
#  endif
#else
#  define MOZ_ASAN_BLACKLIST /* nothing */
#endif

so that people can read the if/else/etc. code to see how the value is defined, rather than having a default and then overriding it after.  (Also: note two-space indentation of nested preprocessor directives.

@@ +193,1 @@
>  #endif

Same rejiggering here, and two-space indents.

Comment 8

[Christian Holler (:decoder)]

Attachment: asan-tsan-attr.patch

Updated patch according to reviewer comments. Carrying r+.

Comment 9

[Christian Holler (:decoder)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/315b927c1934

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/315b927c1934