| Message ID | 20140403165630.GE24603@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 04/03/14 10:56, Marek Polacek wrote: > Under certain circumstances the sanitizer builtins are not initialized > properly and ubsan_instrument_return must make sure they are > initialized. Otherwise builtin_decl_explicit returns NULL and > we'll ICE in build_call_expr_loc_array. I'm not sure which other > ubsan routines need similar fix. > > No testcase attached since it's not trivial to reproduce this. > > Bootstrapped/ran ubsan testsuite on x86_64-linux, ok for trunk? > > 2014-04-03 Marek Polacek <polacek@redhat.com> > > PR sanitizer/60745 > * c-ubsan.c: Include asan.h. > (ubsan_instrument_return): Call initialize_sanitizer_builtins. So what are those circumstances? ISTM this deserves some kind of comment at the least. jeff
On 04/04/14 09:17 -0600, Jeff Law wrote: >So what are those circumstances? ISTM this deserves some kind of >comment at the least. I found the ICE using ubsan on the libstdc++ testsuite. Including the bits/stdc++.h PCH'd header (i.e. the entire library) seemed to trigger it. So either related to PCH, or just huge translation units ... not sure of the exact cause.
On Fri, Apr 04, 2014 at 04:48:48PM +0100, Jonathan Wakely wrote: > On 04/04/14 09:17 -0600, Jeff Law wrote: > >So what are those circumstances? ISTM this deserves some kind of > >comment at the least. > > I found the ICE using ubsan on the libstdc++ testsuite. Including the > bits/stdc++.h PCH'd header (i.e. the entire library) seemed to trigger > it. So either related to PCH, or just huge translation units ... not > sure of the exact cause. So what happens here is that normally we initialize the builtins via c_common_nodes_and_builtins, which has: 5711 if (!flag_preprocess_only) 5712 c_define_builtins (va_list_ref_type_node, va_list_arg_type_node); c_define_builtins defines builtins in builtins.def, that includes even sanitizer.def. I guess that flag_preprocess_only was in effect due to PCH and we didn't define the builtins. I still haven't managed to create some sweet & small testcase, but I've found another ICE with PCH: $ touch y.h $ cat y.c #include "y.h" int main () {} $ gcc y.h $ gcc y.c -fsanitize=undefined -S y.c: In function ‘main’: y.c:2:1: internal compiler error: Segmentation fault int main () {} ^ 0x9a20df crash_signal /home/marek/src/gcc/gcc/toplev.c:337 0x52dfde bind /home/marek/src/gcc/gcc/c/c-decl.c:646 0x53767f c_builtin_function(tree_node*) /home/marek/src/gcc/gcc/c/c-decl.c:3664 0x875340 add_builtin_function_common /home/marek/src/gcc/gcc/langhooks.c:573 0x875ff3 add_builtin_function(char const*, tree_node*, int, built_in_class, char const*, tree_node*) /home/marek/src/gcc/gcc/langhooks.c:589 0x9b46ba initialize_sanitizer_builtins() /home/marek/src/gcc/gcc/sanitizer.def:30 0x9bfca5 ubsan_pass /home/marek/src/gcc/gcc/ubsan.c:870 0x9bfca5 execute /home/marek/src/gcc/gcc/ubsan.c:938 Please submit a full bug report, with preprocessed source if appropriate. Please include the complete backtrace with any bug report. See <http://gcc.gnu.org/bugs.html> for instructions. Marek
On 04/04/14 11:14, Marek Polacek wrote: > > So what happens here is that normally we initialize the builtins via > c_common_nodes_and_builtins, which has: > > 5711 if (!flag_preprocess_only) > 5712 c_define_builtins (va_list_ref_type_node, va_list_arg_type_node); > > c_define_builtins defines builtins in builtins.def, that includes even > sanitizer.def. I guess that flag_preprocess_only was in effect due to PCH > and we didn't define the builtins. I still haven't managed to create > some sweet & small testcase, but I've found another ICE with PCH: What doesn't make sense to me is I don't see where PCH sets flag_preprocess_only. If you can point me to where that happens, then I think we'd just need a comment that PCH turns on flag_preprocess_only, which guards the creation of some builtins. jeff
On Fri, Apr 04, 2014 at 11:50:04AM -0600, Jeff Law wrote: > On 04/04/14 11:14, Marek Polacek wrote: > > > >So what happens here is that normally we initialize the builtins via > >c_common_nodes_and_builtins, which has: > > > > 5711 if (!flag_preprocess_only) > > 5712 c_define_builtins (va_list_ref_type_node, va_list_arg_type_node); > > > >c_define_builtins defines builtins in builtins.def, that includes even > >sanitizer.def. I guess that flag_preprocess_only was in effect due to PCH > >and we didn't define the builtins. I still haven't managed to create > >some sweet & small testcase, but I've found another ICE with PCH: > What doesn't make sense to me is I don't see where PCH sets > flag_preprocess_only. If you can point me to where that happens, > then I think we'd just need a comment that PCH turns on > flag_preprocess_only, which guards the creation of some builtins. I'm sorry, my guess was wrong, it's not about flag_preprocess_only at all. But still it's about PCH ;). In short: we define the builtins, but PCH then removes the definitions. The thing is, we define the builtins just fine (via c_common_nodes_and_builtins -> c_define_builtins), but the problem is that later on we call c_common_read_pch that calls gt_pch_restore which "reads the state of the compiler back in from file" -- and as a part of this, it overwrites the table with defined builtins (builtin_info.decl). And so we end up with zapped table and thus it's needed to re-initialize it. I don't see an easy way how to tell PCH to not to do the removal. Marek
On 04/04/14 13:45, Marek Polacek wrote: > > I'm sorry, my guess was wrong, it's not about flag_preprocess_only at > all. But still it's about PCH ;). In short: we define the builtins, > but PCH then removes the definitions. Ah. > The thing is, we define the > builtins just fine (via c_common_nodes_and_builtins -> c_define_builtins), > but the problem is that later on we call c_common_read_pch that calls > gt_pch_restore which "reads the state of the compiler back in from > file" -- and as a part of this, it overwrites the table with defined > builtins (builtin_info.decl). And so we end up with zapped table > and thus it's needed to re-initialize it. I don't see an easy way > how to tell PCH to not to do the removal. Presumably when we wrote out the PCH -fsanitize wasn't being used and thus those builtins are not initialized. Right? Assuming that's correct, your patch is fine with a comment to that effect. Jeff
diff --git gcc/c-family/c-ubsan.c gcc/c-family/c-ubsan.c index dc4d981..9d2403c 100644 --- gcc/c-family/c-ubsan.c +++ gcc/c-family/c-ubsan.c @@ -29,6 +29,7 @@ along with GCC; see the file COPYING3. If not see #include "ubsan.h" #include "c-family/c-common.h" #include "c-family/c-ubsan.h" +#include "asan.h" /* Instrument division by zero and INT_MIN / -1. If not instrumenting, return NULL_TREE. */ @@ -185,6 +186,8 @@ ubsan_instrument_vla (location_t loc, tree size) tree ubsan_instrument_return (location_t loc) { + initialize_sanitizer_builtins (); + tree data = ubsan_create_data ("__ubsan_missing_return_data", &loc, NULL, NULL_TREE); tree t = builtin_decl_explicit (BUILT_IN_UBSAN_HANDLE_MISSING_RETURN);