[PATCH][PR sanitizer/64354] Define __SANITIZE_THREAD__ and __SANITIZE_UNDEFINED__ macros if corresponding switches are enabled.

[PATCH][PR sanitizer/64354] Define __SANITIZE_THREAD__ and __SANITIZE_UNDEFINED__ macros if corresponding switches are enabled.

[Maxim Ostapenko]

[-- Attachment #1: Type: text/plain, Size: 552 bytes --]

Hi,

when compiling with -fsanitize=address we define __SANITIZE_ADDRESS__ 
macros, but we don't do this for -fsanitize=thread and 
-fsanitize=undefined. Perhaps we should be more symmetric here and 
define corresponding __SANITIZE_THREAD__ and __SANITIZE_UNDEFINED__ 
macros respectively?

I added two simple test cases to c-c++-common/{ub, t}san/ directories 
that just verify if __SANITIZE_THREAD__ (__SANITIZE_UNDEFINED__) is 
defined. Is that a proper way how we check that the macros defined 
correctly? Does this patch looks reasonable?

-Maxim

[-- Attachment #2: pr64354.diff --]
[-- Type: text/x-diff, Size: 2897 bytes --]

gcc/ChangeLog:

2016-05-19  Maxim Ostapenko  <m.ostapenko@samsung.com>

	PR sanitizer/64354
	* cppbuiltin.c (define_builtin_macros_for_compilation_flags): Add new
	builtin __SANITIZE_THREAD__ and __SANITIZE_UNDEFINED__ macros for
	-fsanitize=thread and -fsanitize=undefined switches respectively.
	* doc/cpp.texi: Document new macros.

gcc/testsuite/ChangeLog:

2016-05-19  Maxim Ostapenko  <m.ostapenko@samsung.com>

	PR sanitizer/64354
	* c-c++-common/tsan/sanitize-thread-macro.c: New test.
	* c-c++-common/ubsan/sanitize-undefined-macro.c: Likewise.

diff --git a/gcc/cppbuiltin.c b/gcc/cppbuiltin.c
index 6d494ad..8fd0723 100644
--- a/gcc/cppbuiltin.c
+++ b/gcc/cppbuiltin.c
@@ -92,6 +92,12 @@ define_builtin_macros_for_compilation_flags (cpp_reader *pfile)
   if (flag_sanitize & SANITIZE_ADDRESS)
     cpp_define (pfile, "__SANITIZE_ADDRESS__");
 
+  if (flag_sanitize & SANITIZE_THREAD)
+    cpp_define (pfile, "__SANITIZE_THREAD__");
+
+  if (flag_sanitize & SANITIZE_UNDEFINED)
+    cpp_define (pfile, "__SANITIZE_UNDEFINED__");
+
   if (optimize_size)
     cpp_define (pfile, "__OPTIMIZE_SIZE__");
   if (optimize)
diff --git a/gcc/doc/cpp.texi b/gcc/doc/cpp.texi
index 9f914b2..965795b 100644
--- a/gcc/doc/cpp.texi
+++ b/gcc/doc/cpp.texi
@@ -2362,6 +2362,12 @@ in use.
 This macro is defined, with value 1, when @option{-fsanitize=address}
 or @option{-fsanitize=kernel-address} are in use.
 
+@item __SANITIZE_THREAD__
+This macro is defined, with value 1, when @option{-fsanitize=thread} is in use.
+
+@item __SANITIZE_UNDEFINED__
+This macro is defined, with value 1, when @option{-fsanitize=undefined} is in use.
+
 @item __TIMESTAMP__
 This macro expands to a string constant that describes the date and time
 of the last modification of the current source file. The string constant
diff --git a/gcc/testsuite/c-c++-common/tsan/sanitize-thread-macro.c b/gcc/testsuite/c-c++-common/tsan/sanitize-thread-macro.c
new file mode 100644
index 0000000..2b8a840
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/tsan/sanitize-thread-macro.c
@@ -0,0 +1,12 @@
+/* Check that -fsanitize=thread options defines __SANITIZE_THREAD__ macros.  */
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */
+
+int
+main ()
+{
+#ifndef __SANITIZE_THREAD__
+  bad construction
+#endif
+  return 0;
+}
diff --git a/gcc/testsuite/c-c++-common/ubsan/sanitize-undefined-macro.c b/gcc/testsuite/c-c++-common/ubsan/sanitize-undefined-macro.c
new file mode 100644
index 0000000..7461324
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/ubsan/sanitize-undefined-macro.c
@@ -0,0 +1,13 @@
+/* Check that -fsanitize=undefined options defines __SANITIZE_UNDEFINED__ macros.  */
+/* { dg-do compile } */
+/* { dg-options "-fsanitize=undefined" } */
+/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */
+
+int
+main ()
+{
+#ifndef __SANITIZE_UNDEFINED__
+  bad construction
+#endif
+  return 0;
+}

Re: [PATCH][PR sanitizer/64354] Define __SANITIZE_THREAD__ and __SANITIZE_UNDEFINED__ macros if corresponding switches are enabled.

[Jakub Jelinek]

On Wed, May 18, 2016 at 08:33:53PM +0300, Maxim Ostapenko wrote:
> when compiling with -fsanitize=address we define __SANITIZE_ADDRESS__
> macros, but we don't do this for -fsanitize=thread and -fsanitize=undefined.
> Perhaps we should be more symmetric here and define corresponding
> __SANITIZE_THREAD__ and __SANITIZE_UNDEFINED__ macros respectively?
> 
> I added two simple test cases to c-c++-common/{ub, t}san/ directories that
> just verify if __SANITIZE_THREAD__ (__SANITIZE_UNDEFINED__) is defined. Is
> that a proper way how we check that the macros defined correctly? Does this
> patch looks reasonable?

I can understand __SANITIZE_THREAD__, but I fail to see what
__SANITIZE_UNDEFINED__ would be good for, especially when it is not just
a single sanitizer, but dozens of them.

	Jakub

Re: [PATCH][PR sanitizer/64354] Define __SANITIZE_THREAD__ and __SANITIZE_UNDEFINED__ macros if corresponding switches are enabled.

[Yuri Gribov]

On Wed, May 18, 2016 at 8:36 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, May 18, 2016 at 08:33:53PM +0300, Maxim Ostapenko wrote:
>> when compiling with -fsanitize=address we define __SANITIZE_ADDRESS__
>> macros, but we don't do this for -fsanitize=thread and -fsanitize=undefined.
>> Perhaps we should be more symmetric here and define corresponding
>> __SANITIZE_THREAD__ and __SANITIZE_UNDEFINED__ macros respectively?
>>
>> I added two simple test cases to c-c++-common/{ub, t}san/ directories that
>> just verify if __SANITIZE_THREAD__ (__SANITIZE_UNDEFINED__) is defined. Is
>> that a proper way how we check that the macros defined correctly? Does this
>> patch looks reasonable?
>
> I can understand __SANITIZE_THREAD__, but I fail to see what
> __SANITIZE_UNDEFINED__ would be good for, especially when it is not just
> a single sanitizer, but dozens of them.

Some low-level codes do nasty things like unaligned or NULL memory
accesses. This would allow them to selectively disable UBSan. Good
point about different UB checks though.

-Y

Re: [PATCH][PR sanitizer/64354] Define __SANITIZE_THREAD__ and __SANITIZE_UNDEFINED__ macros if corresponding switches are enabled.

[Yuri Gribov]

On Wed, May 18, 2016 at 8:33 PM, Maxim Ostapenko
<m.ostapenko@samsung.com> wrote:
> Hi,
>
> when compiling with -fsanitize=address we define __SANITIZE_ADDRESS__
> macros, but we don't do this for -fsanitize=thread and -fsanitize=undefined.
> Perhaps we should be more symmetric here and define corresponding
> __SANITIZE_THREAD__ and __SANITIZE_UNDEFINED__ macros respectively?

Are these available in Clang as well?

> I added two simple test cases to c-c++-common/{ub, t}san/ directories that
> just verify if __SANITIZE_THREAD__ (__SANITIZE_UNDEFINED__) is defined. Is
> that a proper way how we check that the macros defined correctly? Does this
> patch looks reasonable?
>
> -Maxim

Re: [PATCH][PR sanitizer/64354] Define __SANITIZE_THREAD__ and __SANITIZE_UNDEFINED__ macros if corresponding switches are enabled.

[Maxim Ostapenko]

On 18/05/16 20:39, Yuri Gribov wrote:
> On Wed, May 18, 2016 at 8:33 PM, Maxim Ostapenko
> <m.ostapenko@samsung.com> wrote:
>> Hi,
>>
>> when compiling with -fsanitize=address we define __SANITIZE_ADDRESS__
>> macros, but we don't do this for -fsanitize=thread and -fsanitize=undefined.
>> Perhaps we should be more symmetric here and define corresponding
>> __SANITIZE_THREAD__ and __SANITIZE_UNDEFINED__ macros respectively?
> Are these available in Clang as well?

AFAIK Clang has __has_feature(address_sanitizer) and 
__has_feature(thread_sanitizer). UBSan doesn't have such things.

>
>> I added two simple test cases to c-c++-common/{ub, t}san/ directories that
>> just verify if __SANITIZE_THREAD__ (__SANITIZE_UNDEFINED__) is defined. Is
>> that a proper way how we check that the macros defined correctly? Does this
>> patch looks reasonable?
>>
>> -Maxim
>

Re: [PATCH][PR sanitizer/64354] Define __SANITIZE_THREAD__ and __SANITIZE_UNDEFINED__ macros if corresponding switches are enabled.

[Maxim Ostapenko]

[-- Attachment #1: Type: text/plain, Size: 1046 bytes --]

On 18/05/16 20:36, Jakub Jelinek wrote:
> On Wed, May 18, 2016 at 08:33:53PM +0300, Maxim Ostapenko wrote:
>> when compiling with -fsanitize=address we define __SANITIZE_ADDRESS__
>> macros, but we don't do this for -fsanitize=thread and -fsanitize=undefined.
>> Perhaps we should be more symmetric here and define corresponding
>> __SANITIZE_THREAD__ and __SANITIZE_UNDEFINED__ macros respectively?
>>
>> I added two simple test cases to c-c++-common/{ub, t}san/ directories that
>> just verify if __SANITIZE_THREAD__ (__SANITIZE_UNDEFINED__) is defined. Is
>> that a proper way how we check that the macros defined correctly? Does this
>> patch looks reasonable?
> I can understand __SANITIZE_THREAD__, but I fail to see what
> __SANITIZE_UNDEFINED__ would be good for, especially when it is not just
> a single sanitizer, but dozens of them.

Ok, I've removed -fsanitize=undefined part from this patch, is it OK 
now? As for UBSan, perhaps it's not desirable to define separate macros 
for each -fsanitize={null, ...} switch.

>
> 	Jakub
>
>


[-- Attachment #2: pr64354-2.diff --]
[-- Type: text/x-diff, Size: 1973 bytes --]

gcc/ChangeLog:

2016-05-19  Maxim Ostapenko  <m.ostapenko@samsung.com>

	PR sanitizer/64354
	* cppbuiltin.c (define_builtin_macros_for_compilation_flags): Add new
	builtin __SANITIZE_THREAD__ macros for fsanitize=thread switch.
	* doc/cpp.texi: Document new macros.

gcc/testsuite/ChangeLog:

2016-05-19  Maxim Ostapenko  <m.ostapenko@samsung.com>

	PR sanitizer/64354
	* c-c++-common/tsan/sanitize-thread-macro.c: New test.

diff --git a/gcc/cppbuiltin.c b/gcc/cppbuiltin.c
index 6d494ad..69ccdb9 100644
--- a/gcc/cppbuiltin.c
+++ b/gcc/cppbuiltin.c
@@ -92,6 +92,9 @@ define_builtin_macros_for_compilation_flags (cpp_reader *pfile)
   if (flag_sanitize & SANITIZE_ADDRESS)
     cpp_define (pfile, "__SANITIZE_ADDRESS__");
 
+  if (flag_sanitize & SANITIZE_THREAD)
+    cpp_define (pfile, "__SANITIZE_THREAD__");
+
   if (optimize_size)
     cpp_define (pfile, "__OPTIMIZE_SIZE__");
   if (optimize)
diff --git a/gcc/doc/cpp.texi b/gcc/doc/cpp.texi
index 9f914b2..44f59bd 100644
--- a/gcc/doc/cpp.texi
+++ b/gcc/doc/cpp.texi
@@ -2362,6 +2362,9 @@ in use.
 This macro is defined, with value 1, when @option{-fsanitize=address}
 or @option{-fsanitize=kernel-address} are in use.
 
+@item __SANITIZE_THREAD__
+This macro is defined, with value 1, when @option{-fsanitize=thread} is in use.
+
 @item __TIMESTAMP__
 This macro expands to a string constant that describes the date and time
 of the last modification of the current source file. The string constant
diff --git a/gcc/testsuite/c-c++-common/tsan/sanitize-thread-macro.c b/gcc/testsuite/c-c++-common/tsan/sanitize-thread-macro.c
new file mode 100644
index 0000000..2b8a840
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/tsan/sanitize-thread-macro.c
@@ -0,0 +1,12 @@
+/* Check that -fsanitize=thread options defines __SANITIZE_THREAD__ macros.  */
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */
+
+int
+main ()
+{
+#ifndef __SANITIZE_THREAD__
+  bad construction
+#endif
+  return 0;
+}

Re: [PATCH][PR sanitizer/64354] Define __SANITIZE_THREAD__ and __SANITIZE_UNDEFINED__ macros if corresponding switches are enabled.

[Jakub Jelinek]

On Thu, May 19, 2016 at 02:48:32PM +0300, Maxim Ostapenko wrote:
> On 18/05/16 20:36, Jakub Jelinek wrote:
> >On Wed, May 18, 2016 at 08:33:53PM +0300, Maxim Ostapenko wrote:
> >>when compiling with -fsanitize=address we define __SANITIZE_ADDRESS__
> >>macros, but we don't do this for -fsanitize=thread and -fsanitize=undefined.
> >>Perhaps we should be more symmetric here and define corresponding
> >>__SANITIZE_THREAD__ and __SANITIZE_UNDEFINED__ macros respectively?
> >>
> >>I added two simple test cases to c-c++-common/{ub, t}san/ directories that
> >>just verify if __SANITIZE_THREAD__ (__SANITIZE_UNDEFINED__) is defined. Is
> >>that a proper way how we check that the macros defined correctly? Does this
> >>patch looks reasonable?
> >I can understand __SANITIZE_THREAD__, but I fail to see what
> >__SANITIZE_UNDEFINED__ would be good for, especially when it is not just
> >a single sanitizer, but dozens of them.
> 
> Ok, I've removed -fsanitize=undefined part from this patch, is it OK now? As
> for UBSan, perhaps it's not desirable to define separate macros for each
> -fsanitize={null, ...} switch.
> 
> >
> >	Jakub
> >
> >
> 

> gcc/ChangeLog:
> 
> 2016-05-19  Maxim Ostapenko  <m.ostapenko@samsung.com>
> 
> 	PR sanitizer/64354
> 	* cppbuiltin.c (define_builtin_macros_for_compilation_flags): Add new
> 	builtin __SANITIZE_THREAD__ macros for fsanitize=thread switch.
> 	* doc/cpp.texi: Document new macros.
> 
> gcc/testsuite/ChangeLog:
> 
> 2016-05-19  Maxim Ostapenko  <m.ostapenko@samsung.com>
> 
> 	PR sanitizer/64354
> 	* c-c++-common/tsan/sanitize-thread-macro.c: New test.

Ok.

	Jakub