diff mbox

Fix -fsanitize=undefined with PCH, part #2 (PR sanitizer/66343)

Message ID 20161005152949.GB16311@tucnak.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek Oct. 5, 2016, 3:29 p.m. UTC 
Hi!

When writing test for this PR, I've noticed ICE if the header is compiled
without -fsanitize=undefined, but source is compiled with it.

We had various issues like this in the past, and we handle it by calling
initialize_sanitizer_builtins, which does nothing if the sanitizer bultins
are already created, but after loading PCH (which can kill them) can fix stuff
up again.  I found various spots where the call has been missing in the
ubsan instrumentation, but common feature of all those spots is first
calling ubsan_create_data and only then using builtin_decl_explicit
for the ubsan builtins.  So, this patch puts the initialization call into
that routine, which fixes all uses, and removes the two calls that are now
unnecessary because of that.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2016-10-05  Jakub Jelinek  <jakub@redhat.com>

	PR sanitizer/66343
	* ubsan.c (ubsan_create_data): Call initialize_sanitizer_builtins here.
	(ubsan_instrument_float_cast): And not here.

	* c-ubsan.c (ubsan_instrument_return): Don't call
	initialize_sanitizer_builtins here.

	* gcc.dg/pch/pr66343-3.c: New test.
	* gcc.dg/pch/pr66343-3.hs: New file.


	Jakub

Comments

Marek Polacek Oct. 5, 2016, 4:51 p.m. UTC | #1 
On Wed, Oct 05, 2016 at 05:29:49PM +0200, Jakub Jelinek wrote:
> Hi!
> 
> When writing test for this PR, I've noticed ICE if the header is compiled
> without -fsanitize=undefined, but source is compiled with it.
> 
> We had various issues like this in the past, and we handle it by calling
> initialize_sanitizer_builtins, which does nothing if the sanitizer bultins
> are already created, but after loading PCH (which can kill them) can fix stuff
> up again.  I found various spots where the call has been missing in the
> ubsan instrumentation, but common feature of all those spots is first
> calling ubsan_create_data and only then using builtin_decl_explicit
> for the ubsan builtins.  So, this patch puts the initialization call into
> that routine, which fixes all uses, and removes the two calls that are now
> unnecessary because of that.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

LGTM, but can't approve neither.

	Marek
Bernd Schmidt Oct. 5, 2016, 5:57 p.m. UTC | #2 
On 10/05/2016 06:51 PM, Marek Polacek wrote:
> On Wed, Oct 05, 2016 at 05:29:49PM +0200, Jakub Jelinek wrote:
>> Hi!
>>
>> When writing test for this PR, I've noticed ICE if the header is compiled
>> without -fsanitize=undefined, but source is compiled with it.
>>
>> We had various issues like this in the past, and we handle it by calling
>> initialize_sanitizer_builtins, which does nothing if the sanitizer bultins
>> are already created, but after loading PCH (which can kill them) can fix stuff
>> up again.  I found various spots where the call has been missing in the
>> ubsan instrumentation, but common feature of all those spots is first
>> calling ubsan_create_data and only then using builtin_decl_explicit
>> for the ubsan builtins.  So, this patch puts the initialization call into
>> that routine, which fixes all uses, and removes the two calls that are now
>> unnecessary because of that.
>>
>> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> LGTM, but can't approve neither.

Ok. Although I wonder, pass_ubsan::execute calls this function: why 
isn't this enough? Maybe we just need to add it to other pass execute 
functions as well?


Bernd
Jakub Jelinek Oct. 5, 2016, 6:03 p.m. UTC | #3 
On Wed, Oct 05, 2016 at 07:57:12PM +0200, Bernd Schmidt wrote:
> On 10/05/2016 06:51 PM, Marek Polacek wrote:
> >On Wed, Oct 05, 2016 at 05:29:49PM +0200, Jakub Jelinek wrote:
> >>When writing test for this PR, I've noticed ICE if the header is compiled
> >>without -fsanitize=undefined, but source is compiled with it.
> >>
> >>We had various issues like this in the past, and we handle it by calling
> >>initialize_sanitizer_builtins, which does nothing if the sanitizer bultins
> >>are already created, but after loading PCH (which can kill them) can fix stuff
> >>up again.  I found various spots where the call has been missing in the
> >>ubsan instrumentation, but common feature of all those spots is first
> >>calling ubsan_create_data and only then using builtin_decl_explicit
> >>for the ubsan builtins.  So, this patch puts the initialization call into
> >>that routine, which fixes all uses, and removes the two calls that are now
> >>unnecessary because of that.
> >>
> >>Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> >
> >LGTM, but can't approve neither.
> 
> Ok. Although I wonder, pass_ubsan::execute calls this function: why isn't
> this enough? Maybe we just need to add it to other pass execute functions as
> well?

-fsanitize=undefined is actually just a collection of various runtime
checks that are added at various times.  The call at the start of
pass_ubsan::execute is needed for any builtin_decl_explicit uses in that
pass.  But some sanitization happens already in the FEs.  That is sometimes
done by creating internal calls and lowered only later on during the ubsan
pass (which also instruments e.g. arithmetics overflows etc.), and sometimes
by creating the builtin calls right away.  The call I've added to
ubsan_create_data is for all those uses.  An alternative would be to add
IFN_UBSAN_* calls for all the stuff that we instrument in the FEs and lower
it in the ubsan pass to what we emit right away.

	Jakub
diff mbox

Patch

--- gcc/ubsan.c.jj	2016-10-05 11:59:04.000000000 +0200
+++ gcc/ubsan.c	2016-10-05 12:52:32.099353026 +0200
@@ -511,6 +511,10 @@  ubsan_create_data (const char *name, int
   size_t i = 0;
   int j;
 
+  /* It is possible that PCH zapped table with definitions of sanitizer
+     builtins.  Reinitialize them if needed.  */
+  initialize_sanitizer_builtins ();
+
   /* Firstly, create a pointer to type descriptor type.  */
   tree td_type = ubsan_get_type_descriptor_type ();
   td_type = build_pointer_type (td_type);
@@ -1589,7 +1593,6 @@  ubsan_instrument_float_cast (location_t
     {
       location_t *loc_ptr = NULL;
       unsigned num_locations = 0;
-      initialize_sanitizer_builtins ();
       /* Figure out if we can propagate location to ubsan_data and use new
          style handlers in libubsan.  */
       if (ubsan_use_new_style_p (loc))
--- gcc/c-family/c-ubsan.c.jj	2016-10-03 16:21:26.000000000 +0200
+++ gcc/c-family/c-ubsan.c	2016-10-05 12:52:54.623066115 +0200
@@ -233,9 +233,6 @@  ubsan_instrument_return (location_t loc)
 {
   if (flag_sanitize_undefined_trap_on_error)
     return build_call_expr_loc (loc, builtin_decl_explicit (BUILT_IN_TRAP), 0);
-  /* It is possible that PCH zapped table with definitions of sanitizer
-     builtins.  Reinitialize them if needed.  */
-  initialize_sanitizer_builtins ();
 
   tree data = ubsan_create_data ("__ubsan_missing_return_data", 1, &loc,
 				 NULL_TREE, NULL_TREE);
--- gcc/testsuite/gcc.dg/pch/pr66343-3.c.jj	2016-10-05 12:58:35.885719045 +0200
+++ gcc/testsuite/gcc.dg/pch/pr66343-3.c	2016-10-05 12:57:17.600716255 +0200
@@ -0,0 +1,15 @@ 
+/* PR sanitizer/66343 */
+/* { dg-do assemble } */
+/* { dg-options "-fsanitize=undefined" } */
+
+#include "pr66343-3.h"
+
+void
+bar (int a, int b)
+{
+  a / b;
+}
+
+/* Hack to turn off PCH assembly comparison, as it is incompatible
+   with dg-do assemble.  The target condition will be always false.  */
+/* { dg-error "" "" { target { lp64 && { ! lp64 } } } } */
--- gcc/testsuite/gcc.dg/pch/pr66343-3.hs.jj	2016-10-05 12:45:24.654797897 +0200
+++ gcc/testsuite/gcc.dg/pch/pr66343-3.hs	2016-10-05 12:56:10.017577142 +0200
@@ -0,0 +1,4 @@ 
+/* PR sanitizer/66343 */
+/* { dg-options "-fno-sanitize=undefined" } */
+
+/* Empty.  */