* Fix for PR79987 @ 2017-04-04 15:07 Alexander Ivchenko 2017-04-04 15:34 ` Jeff Law 0 siblings, 1 reply; 7+ messages in thread From: Alexander Ivchenko @ 2017-04-04 15:07 UTC (permalink / raw) To: Ilya Enkovich, marxin; +Cc: GCC Patches Hi, When creating static bounds for foo below we end up with: *((unsigned long *) &__chkp_bounds_of_foo + 8) = ~(__builtin_ia32_sizeof (foo) + ((long unsigned int) &foo + 18446744073709551615)); This fails in gimplify_function_tree with gcc_assert (!VOID_TYPE_P (TREE_TYPE (*expr_p))); Is it OK? gcc/ChangeLog: 2017-04-04 Alexander Ivchenko <aivchenk@gmail.com> * tree-chkp.c (chkp_get_bounds_for_decl_addr): assigning zero bounds to void variables gcc/testsuite/ChangeLog: 2017-04-04 Alexander Ivchenko <aivchenk@gmail.com> * gcc.target/i386/mpx/PR79987.c: New test. diff --git a/gcc/testsuite/gcc.target/i386/mpx/PR79987.c b/gcc/testsuite/gcc.target/i386/mpx/PR79987.c new file mode 100644 index 0000000..14a32f3 --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/mpx/PR79987.c @@ -0,0 +1,5 @@ +/* { dg-do compile } */ +/* { dg-options "-fcheck-pointer-bounds -mmpx" } */ + +extern void foo; +void *bar = &foo; diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c index b1ff218..8a16025 100644 --- a/gcc/tree-chkp.c +++ b/gcc/tree-chkp.c @@ -3146,6 +3146,11 @@ chkp_get_bounds_for_decl_addr (tree decl) && !flag_chkp_incomplete_type) return chkp_get_zero_bounds (); + /* For void variables use zero bounds as well - we cannot + possibly get the size of the void. */ + if (VOID_TYPE_P (TREE_TYPE (decl))) + return chkp_get_zero_bounds (); + if (flag_chkp_use_static_bounds && VAR_P (decl) && (TREE_STATIC (decl) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Fix for PR79987 2017-04-04 15:07 Fix for PR79987 Alexander Ivchenko @ 2017-04-04 15:34 ` Jeff Law 2017-04-08 19:59 ` Ilya Enkovich 0 siblings, 1 reply; 7+ messages in thread From: Jeff Law @ 2017-04-04 15:34 UTC (permalink / raw) To: Alexander Ivchenko, Ilya Enkovich, marxin; +Cc: GCC Patches On 04/04/2017 09:07 AM, Alexander Ivchenko wrote: > Hi, > > When creating static bounds for foo below we end up with: > > *((unsigned long *) &__chkp_bounds_of_foo + 8) = > ~(__builtin_ia32_sizeof (foo) + ((long unsigned int) &foo + > 18446744073709551615)); > > This fails in gimplify_function_tree with gcc_assert (!VOID_TYPE_P > (TREE_TYPE (*expr_p))); > > Is it OK? > > gcc/ChangeLog: > > 2017-04-04 Alexander Ivchenko <aivchenk@gmail.com> > > * tree-chkp.c (chkp_get_bounds_for_decl_addr): > assigning zero bounds to void variables > > > gcc/testsuite/ChangeLog: > > 2017-04-04 Alexander Ivchenko <aivchenk@gmail.com> > > * gcc.target/i386/mpx/PR79987.c: New test. I've put this (and other CHKP fixes) in the queue for gcc-8 as AFAICT it's not a regression. Jeff ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Fix for PR79987 2017-04-04 15:34 ` Jeff Law @ 2017-04-08 19:59 ` Ilya Enkovich 2017-05-09 15:41 ` Alexander Ivchenko 0 siblings, 1 reply; 7+ messages in thread From: Ilya Enkovich @ 2017-04-08 19:59 UTC (permalink / raw) To: Jeff Law; +Cc: Alexander Ivchenko, marxin, GCC Patches 2017-04-04 18:34 GMT+03:00 Jeff Law <law@redhat.com>: > On 04/04/2017 09:07 AM, Alexander Ivchenko wrote: >> >> Hi, >> >> When creating static bounds for foo below we end up with: >> >> *((unsigned long *) &__chkp_bounds_of_foo + 8) = >> ~(__builtin_ia32_sizeof (foo) + ((long unsigned int) &foo + >> 18446744073709551615)); >> >> This fails in gimplify_function_tree with gcc_assert (!VOID_TYPE_P >> (TREE_TYPE (*expr_p))); >> >> Is it OK? >> >> gcc/ChangeLog: >> >> 2017-04-04 Alexander Ivchenko <aivchenk@gmail.com> >> >> * tree-chkp.c (chkp_get_bounds_for_decl_addr): >> assigning zero bounds to void variables >> >> >> gcc/testsuite/ChangeLog: >> >> 2017-04-04 Alexander Ivchenko <aivchenk@gmail.com> >> >> * gcc.target/i386/mpx/PR79987.c: New test. > > I've put this (and other CHKP fixes) in the queue for gcc-8 as AFAICT it's > not a regression. > > Jeff > Hi, If we delay it for GCC8 anyway then I think we may fix it in a better way. If we cannot detect size of a variable then size relocations may be used. It is done already for arrays with unknown size and also can be done for void vars. Thanks, Ilya ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Fix for PR79987 2017-04-08 19:59 ` Ilya Enkovich @ 2017-05-09 15:41 ` Alexander Ivchenko [not found] ` <CAMbmDYYj++Trk1JHs0dXBR4NfLCaKNnzLHN1PBF7a_96YCJGWg@mail.gmail.com> 0 siblings, 1 reply; 7+ messages in thread From: Alexander Ivchenko @ 2017-05-09 15:41 UTC (permalink / raw) To: Ilya Enkovich; +Cc: Jeff Law, marxin, GCC Patches If we use chkp_generate_extern_var_bounds for void variable just as for arrays with unknown size, we will create the following gimple seq: # VUSE <.MEM> __size_tmp.0 = __builtin_ia32_sizeof (foo); __size_tmp.1_3 = __size_tmp.0; However, this will fail in verify_gimple_call: tree arg = gimple_call_arg (stmt, i); if ((is_gimple_reg_type (TREE_TYPE (arg)) && !is_gimple_val (arg)) || (!is_gimple_reg_type (TREE_TYPE (arg)) && !is_gimple_lvalue (arg))) { error ("invalid argument to gimple call"); debug_generic_expr (arg); return true; } ..here the TREE_TYPE(arg)==void. Any ideas for a good workaround ? Alexander 2017-04-08 21:59 GMT+02:00 Ilya Enkovich <enkovich.gnu@gmail.com>: > 2017-04-04 18:34 GMT+03:00 Jeff Law <law@redhat.com>: >> On 04/04/2017 09:07 AM, Alexander Ivchenko wrote: >>> >>> Hi, >>> >>> When creating static bounds for foo below we end up with: >>> >>> *((unsigned long *) &__chkp_bounds_of_foo + 8) = >>> ~(__builtin_ia32_sizeof (foo) + ((long unsigned int) &foo + >>> 18446744073709551615)); >>> >>> This fails in gimplify_function_tree with gcc_assert (!VOID_TYPE_P >>> (TREE_TYPE (*expr_p))); >>> >>> Is it OK? >>> >>> gcc/ChangeLog: >>> >>> 2017-04-04 Alexander Ivchenko <aivchenk@gmail.com> >>> >>> * tree-chkp.c (chkp_get_bounds_for_decl_addr): >>> assigning zero bounds to void variables >>> >>> >>> gcc/testsuite/ChangeLog: >>> >>> 2017-04-04 Alexander Ivchenko <aivchenk@gmail.com> >>> >>> * gcc.target/i386/mpx/PR79987.c: New test. >> >> I've put this (and other CHKP fixes) in the queue for gcc-8 as AFAICT it's >> not a regression. >> >> Jeff >> > > Hi, > > If we delay it for GCC8 anyway then I think we may fix it in a better way. If we > cannot detect size of a variable then size relocations may be used. It is done > already for arrays with unknown size and also can be done for void vars. > > Thanks, > Ilya ^ permalink raw reply [flat|nested] 7+ messages in thread
[parent not found: <CAMbmDYYj++Trk1JHs0dXBR4NfLCaKNnzLHN1PBF7a_96YCJGWg@mail.gmail.com>]
[parent not found: <CACysShjZboYogD31Ujgt-xpE3jK2co1nUpEzxbbrSWbOV_Z4Tg@mail.gmail.com>]
[parent not found: <CAMbmDYbHjZEXz3YcTvPQa950sxmR3fa+yYsCPkzhychFtjW4Cw@mail.gmail.com>]
[parent not found: <CACysShip_WNDryk1kUiJCO0Mpc_8SVNRkByTdmzEC0xfxFuviw@mail.gmail.com>]
* Re: Fix for PR79987 [not found] ` <CACysShip_WNDryk1kUiJCO0Mpc_8SVNRkByTdmzEC0xfxFuviw@mail.gmail.com> @ 2017-05-11 18:41 ` Ilya Enkovich 2017-06-09 12:22 ` Alexander Ivchenko 0 siblings, 1 reply; 7+ messages in thread From: Ilya Enkovich @ 2017-05-11 18:41 UTC (permalink / raw) To: Alexander Ivchenko, gcc-patches 2017-05-11 0:05 GMT+03:00 Alexander Ivchenko <aivchenk@gmail.com>: > Didn't quite get your point. I though that the idea is to hit this code: > > bounds = chkp_generate_extern_var_bounds (decl); > ... to get the builtin_sizeof call on this variable and hence to get > the size relocation. What exactly do you mean by reproducing the > problem? Currently the compiler just ICE on the testcase :) There are various ways to build bounds. On of them is used by default and causes ICE. With your patch another way is chosen causing another ICE. But you can get this another ICE by simply using compiler flag. Obviously both ways should be fixed. You don't have to hit chkp_generate_extern_var_bounds to use size relocation. If you create static bounds var then it is initialized later in chkp_output_static_bounds and size relocations can appear there either. Thanks Ilya > > Alexander > > 2017-05-10 22:51 GMT+02:00 Ilya Enkovich <enkovich.gnu@gmail.com>: >> 2017-05-10 22:08 GMT+03:00 Alexander Ivchenko <aivchenk@gmail.com>: >>> Hi, >>> >>> something like that: >>> >>> diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c >>> index b1ff218..be06d71 100644 >>> --- a/gcc/tree-chkp.c >>> +++ b/gcc/tree-chkp.c >>> @@ -3148,6 +3148,7 @@ chkp_get_bounds_for_decl_addr (tree decl) >>> >>> if (flag_chkp_use_static_bounds >>> && VAR_P (decl) >>> + && !VOID_TYPE_P (TREE_TYPE (decl)) >>> && (TREE_STATIC (decl) >>> || DECL_EXTERNAL (decl) >>> || TREE_PUBLIC (decl)) >>> @@ -3162,10 +3163,11 @@ chkp_get_bounds_for_decl_addr (tree decl) >>> gsi_insert_before (&gsi, stmt, GSI_SAME_STMT); >>> } >>> else if (!DECL_SIZE (decl) >>> - || (chkp_variable_size_type (TREE_TYPE (decl)) >>> - && (TREE_STATIC (decl) >>> - || DECL_EXTERNAL (decl) >>> - || TREE_PUBLIC (decl)))) >>> + || VOID_TYPE_P (TREE_TYPE (decl)) >>> + || (chkp_variable_size_type (TREE_TYPE (decl)) >>> + && (TREE_STATIC (decl) >>> + || DECL_EXTERNAL (decl) >>> + || TREE_PUBLIC (decl)))) >>> { >>> gcc_assert (VAR_P (decl)); >>> bounds = chkp_generate_extern_var_bounds (decl); >> >> Looking one more time into this issue I now think that bounds generation >> already uses size relocation and this is not a part to fix. >> >> BTW this patch just restricts static bounds creation for void vars which >> is not what you need. I think you should be able to reproduce the problem >> without any patch by just using -fno-chkp-use-static-bounds. >> >> Looks like gimple doesn't allow void vars as call arguments. I don't know >> what is the best way to handle this restriction. The simplest thing a can >> think of is to change builtin call so it gets address of a decl instead of >> a decl. >> >> Thanks, >> Ilya >> >>> >>> Thanks, >>> Alexander >>> >>> 2017-05-10 20:55 GMT+02:00 Ilya Enkovich <enkovich.gnu@gmail.com>: >>>> Hi, >>>> >>>> Please share a patch you use. >>>> >>>> Thanks, >>>> Ilya >>>> >>>> 2017-05-09 18:39 GMT+03:00 Alexander Ivchenko <aivchenk@gmail.com>: >>>>> If we use chkp_generate_extern_var_bounds for void variable just as >>>>> for arrays with unknown size, we will create the following gimple seq: >>>>> >>>>> # VUSE <.MEM> >>>>> __size_tmp.0 = __builtin_ia32_sizeof (foo); >>>>> __size_tmp.1_3 = __size_tmp.0; >>>>> >>>>> However, this will fail in verify_gimple_call: >>>>> >>>>> tree arg = gimple_call_arg (stmt, i); >>>>> if ((is_gimple_reg_type (TREE_TYPE (arg)) >>>>> && !is_gimple_val (arg)) >>>>> || (!is_gimple_reg_type (TREE_TYPE (arg)) >>>>> && !is_gimple_lvalue (arg))) >>>>> { >>>>> error ("invalid argument to gimple call"); >>>>> debug_generic_expr (arg); >>>>> return true; >>>>> } >>>>> ..here the TREE_TYPE(arg)==void. Any ideas for a good workaround ? >>>>> >>>>> Alexander >>>>> >>>>> >>>>> >>>>> 2017-04-08 21:59 GMT+02:00 Ilya Enkovich <enkovich.gnu@gmail.com>: >>>>>> 2017-04-04 18:34 GMT+03:00 Jeff Law <law@redhat.com>: >>>>>>> On 04/04/2017 09:07 AM, Alexander Ivchenko wrote: >>>>>>>> >>>>>>>> Hi, >>>>>>>> >>>>>>>> When creating static bounds for foo below we end up with: >>>>>>>> >>>>>>>> *((unsigned long *) &__chkp_bounds_of_foo + 8) = >>>>>>>> ~(__builtin_ia32_sizeof (foo) + ((long unsigned int) &foo + >>>>>>>> 18446744073709551615)); >>>>>>>> >>>>>>>> This fails in gimplify_function_tree with gcc_assert (!VOID_TYPE_P >>>>>>>> (TREE_TYPE (*expr_p))); >>>>>>>> >>>>>>>> Is it OK? >>>>>>>> >>>>>>>> gcc/ChangeLog: >>>>>>>> >>>>>>>> 2017-04-04 Alexander Ivchenko <aivchenk@gmail.com> >>>>>>>> >>>>>>>> * tree-chkp.c (chkp_get_bounds_for_decl_addr): >>>>>>>> assigning zero bounds to void variables >>>>>>>> >>>>>>>> >>>>>>>> gcc/testsuite/ChangeLog: >>>>>>>> >>>>>>>> 2017-04-04 Alexander Ivchenko <aivchenk@gmail.com> >>>>>>>> >>>>>>>> * gcc.target/i386/mpx/PR79987.c: New test. >>>>>>> >>>>>>> I've put this (and other CHKP fixes) in the queue for gcc-8 as AFAICT it's >>>>>>> not a regression. >>>>>>> >>>>>>> Jeff >>>>>>> >>>>>> >>>>>> Hi, >>>>>> >>>>>> If we delay it for GCC8 anyway then I think we may fix it in a better way. If we >>>>>> cannot detect size of a variable then size relocations may be used. It is done >>>>>> already for arrays with unknown size and also can be done for void vars. >>>>>> >>>>>> Thanks, >>>>>> Ilya ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Fix for PR79987 2017-05-11 18:41 ` Ilya Enkovich @ 2017-06-09 12:22 ` Alexander Ivchenko 2017-06-09 17:46 ` Ilya Enkovich 0 siblings, 1 reply; 7+ messages in thread From: Alexander Ivchenko @ 2017-06-09 12:22 UTC (permalink / raw) To: Ilya Enkovich; +Cc: gcc-patches Hi Ilya, I tried changing builtin call so it gets address of a decl instead of a decl, but it looked very unnatural and I hit some other problems implementing that. Keeping in mind the exposure of this problem, I think it is not worth it. I propose to reconsider the first and the simplest patch in this thread: just returning zero bounds for void vars. The standard does not specify that size anyways. What do you think? Alexander 2017-05-11 20:37 GMT+02:00 Ilya Enkovich <enkovich.gnu@gmail.com>: > 2017-05-11 0:05 GMT+03:00 Alexander Ivchenko <aivchenk@gmail.com>: >> Didn't quite get your point. I though that the idea is to hit this code: >> >> bounds = chkp_generate_extern_var_bounds (decl); >> ... to get the builtin_sizeof call on this variable and hence to get >> the size relocation. What exactly do you mean by reproducing the >> problem? Currently the compiler just ICE on the testcase :) > > There are various ways to build bounds. On of them is used by default > and causes ICE. With your patch another way is chosen causing another > ICE. But you can get this another ICE by simply using compiler flag. > Obviously both ways should be fixed. > > You don't have to hit chkp_generate_extern_var_bounds to use size > relocation. If you create static bounds var then it is initialized later > in chkp_output_static_bounds and size relocations can appear there > either. > > Thanks > Ilya > >> >> Alexander >> >> 2017-05-10 22:51 GMT+02:00 Ilya Enkovich <enkovich.gnu@gmail.com>: >>> 2017-05-10 22:08 GMT+03:00 Alexander Ivchenko <aivchenk@gmail.com>: >>>> Hi, >>>> >>>> something like that: >>>> >>>> diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c >>>> index b1ff218..be06d71 100644 >>>> --- a/gcc/tree-chkp.c >>>> +++ b/gcc/tree-chkp.c >>>> @@ -3148,6 +3148,7 @@ chkp_get_bounds_for_decl_addr (tree decl) >>>> >>>> if (flag_chkp_use_static_bounds >>>> && VAR_P (decl) >>>> + && !VOID_TYPE_P (TREE_TYPE (decl)) >>>> && (TREE_STATIC (decl) >>>> || DECL_EXTERNAL (decl) >>>> || TREE_PUBLIC (decl)) >>>> @@ -3162,10 +3163,11 @@ chkp_get_bounds_for_decl_addr (tree decl) >>>> gsi_insert_before (&gsi, stmt, GSI_SAME_STMT); >>>> } >>>> else if (!DECL_SIZE (decl) >>>> - || (chkp_variable_size_type (TREE_TYPE (decl)) >>>> - && (TREE_STATIC (decl) >>>> - || DECL_EXTERNAL (decl) >>>> - || TREE_PUBLIC (decl)))) >>>> + || VOID_TYPE_P (TREE_TYPE (decl)) >>>> + || (chkp_variable_size_type (TREE_TYPE (decl)) >>>> + && (TREE_STATIC (decl) >>>> + || DECL_EXTERNAL (decl) >>>> + || TREE_PUBLIC (decl)))) >>>> { >>>> gcc_assert (VAR_P (decl)); >>>> bounds = chkp_generate_extern_var_bounds (decl); >>> >>> Looking one more time into this issue I now think that bounds generation >>> already uses size relocation and this is not a part to fix. >>> >>> BTW this patch just restricts static bounds creation for void vars which >>> is not what you need. I think you should be able to reproduce the problem >>> without any patch by just using -fno-chkp-use-static-bounds. >>> >>> Looks like gimple doesn't allow void vars as call arguments. I don't know >>> what is the best way to handle this restriction. The simplest thing a can >>> think of is to change builtin call so it gets address of a decl instead of >>> a decl. >>> >>> Thanks, >>> Ilya >>> >>>> >>>> Thanks, >>>> Alexander >>>> >>>> 2017-05-10 20:55 GMT+02:00 Ilya Enkovich <enkovich.gnu@gmail.com>: >>>>> Hi, >>>>> >>>>> Please share a patch you use. >>>>> >>>>> Thanks, >>>>> Ilya >>>>> >>>>> 2017-05-09 18:39 GMT+03:00 Alexander Ivchenko <aivchenk@gmail.com>: >>>>>> If we use chkp_generate_extern_var_bounds for void variable just as >>>>>> for arrays with unknown size, we will create the following gimple seq: >>>>>> >>>>>> # VUSE <.MEM> >>>>>> __size_tmp.0 = __builtin_ia32_sizeof (foo); >>>>>> __size_tmp.1_3 = __size_tmp.0; >>>>>> >>>>>> However, this will fail in verify_gimple_call: >>>>>> >>>>>> tree arg = gimple_call_arg (stmt, i); >>>>>> if ((is_gimple_reg_type (TREE_TYPE (arg)) >>>>>> && !is_gimple_val (arg)) >>>>>> || (!is_gimple_reg_type (TREE_TYPE (arg)) >>>>>> && !is_gimple_lvalue (arg))) >>>>>> { >>>>>> error ("invalid argument to gimple call"); >>>>>> debug_generic_expr (arg); >>>>>> return true; >>>>>> } >>>>>> ..here the TREE_TYPE(arg)==void. Any ideas for a good workaround ? >>>>>> >>>>>> Alexander >>>>>> >>>>>> >>>>>> >>>>>> 2017-04-08 21:59 GMT+02:00 Ilya Enkovich <enkovich.gnu@gmail.com>: >>>>>>> 2017-04-04 18:34 GMT+03:00 Jeff Law <law@redhat.com>: >>>>>>>> On 04/04/2017 09:07 AM, Alexander Ivchenko wrote: >>>>>>>>> >>>>>>>>> Hi, >>>>>>>>> >>>>>>>>> When creating static bounds for foo below we end up with: >>>>>>>>> >>>>>>>>> *((unsigned long *) &__chkp_bounds_of_foo + 8) = >>>>>>>>> ~(__builtin_ia32_sizeof (foo) + ((long unsigned int) &foo + >>>>>>>>> 18446744073709551615)); >>>>>>>>> >>>>>>>>> This fails in gimplify_function_tree with gcc_assert (!VOID_TYPE_P >>>>>>>>> (TREE_TYPE (*expr_p))); >>>>>>>>> >>>>>>>>> Is it OK? >>>>>>>>> >>>>>>>>> gcc/ChangeLog: >>>>>>>>> >>>>>>>>> 2017-04-04 Alexander Ivchenko <aivchenk@gmail.com> >>>>>>>>> >>>>>>>>> * tree-chkp.c (chkp_get_bounds_for_decl_addr): >>>>>>>>> assigning zero bounds to void variables >>>>>>>>> >>>>>>>>> >>>>>>>>> gcc/testsuite/ChangeLog: >>>>>>>>> >>>>>>>>> 2017-04-04 Alexander Ivchenko <aivchenk@gmail.com> >>>>>>>>> >>>>>>>>> * gcc.target/i386/mpx/PR79987.c: New test. >>>>>>>> >>>>>>>> I've put this (and other CHKP fixes) in the queue for gcc-8 as AFAICT it's >>>>>>>> not a regression. >>>>>>>> >>>>>>>> Jeff >>>>>>>> >>>>>>> >>>>>>> Hi, >>>>>>> >>>>>>> If we delay it for GCC8 anyway then I think we may fix it in a better way. If we >>>>>>> cannot detect size of a variable then size relocations may be used. It is done >>>>>>> already for arrays with unknown size and also can be done for void vars. >>>>>>> >>>>>>> Thanks, >>>>>>> Ilya ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Fix for PR79987 2017-06-09 12:22 ` Alexander Ivchenko @ 2017-06-09 17:46 ` Ilya Enkovich 0 siblings, 0 replies; 7+ messages in thread From: Ilya Enkovich @ 2017-06-09 17:46 UTC (permalink / raw) To: Alexander Ivchenko; +Cc: gcc-patches 2017-06-09 15:21 GMT+03:00 Alexander Ivchenko <aivchenk@gmail.com>: > Hi Ilya, > > I tried changing builtin call so it gets address of a decl instead of > a decl, but it looked very unnatural and I hit some other problems What is unnatural in passing a pointer to a function to get its bounds? Please show your patch and describe problems you hit. > implementing that. Keeping in mind the exposure of this problem, I > think it is not worth it. I propose to reconsider the first and the > simplest patch in this thread: just returning zero bounds for void > vars. The standard does not specify that size anyways. What do you > think? I think declaring void var is similar to declaring static array with no size and we should handle these cases similarly. Thanks, Ilya > > Alexander > > > > 2017-05-11 20:37 GMT+02:00 Ilya Enkovich <enkovich.gnu@gmail.com>: >> 2017-05-11 0:05 GMT+03:00 Alexander Ivchenko <aivchenk@gmail.com>: >>> Didn't quite get your point. I though that the idea is to hit this code: >>> >>> bounds = chkp_generate_extern_var_bounds (decl); >>> ... to get the builtin_sizeof call on this variable and hence to get >>> the size relocation. What exactly do you mean by reproducing the >>> problem? Currently the compiler just ICE on the testcase :) >> >> There are various ways to build bounds. On of them is used by default >> and causes ICE. With your patch another way is chosen causing another >> ICE. But you can get this another ICE by simply using compiler flag. >> Obviously both ways should be fixed. >> >> You don't have to hit chkp_generate_extern_var_bounds to use size >> relocation. If you create static bounds var then it is initialized later >> in chkp_output_static_bounds and size relocations can appear there >> either. >> >> Thanks >> Ilya >> >>> >>> Alexander >>> >>> 2017-05-10 22:51 GMT+02:00 Ilya Enkovich <enkovich.gnu@gmail.com>: >>>> 2017-05-10 22:08 GMT+03:00 Alexander Ivchenko <aivchenk@gmail.com>: >>>>> Hi, >>>>> >>>>> something like that: >>>>> >>>>> diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c >>>>> index b1ff218..be06d71 100644 >>>>> --- a/gcc/tree-chkp.c >>>>> +++ b/gcc/tree-chkp.c >>>>> @@ -3148,6 +3148,7 @@ chkp_get_bounds_for_decl_addr (tree decl) >>>>> >>>>> if (flag_chkp_use_static_bounds >>>>> && VAR_P (decl) >>>>> + && !VOID_TYPE_P (TREE_TYPE (decl)) >>>>> && (TREE_STATIC (decl) >>>>> || DECL_EXTERNAL (decl) >>>>> || TREE_PUBLIC (decl)) >>>>> @@ -3162,10 +3163,11 @@ chkp_get_bounds_for_decl_addr (tree decl) >>>>> gsi_insert_before (&gsi, stmt, GSI_SAME_STMT); >>>>> } >>>>> else if (!DECL_SIZE (decl) >>>>> - || (chkp_variable_size_type (TREE_TYPE (decl)) >>>>> - && (TREE_STATIC (decl) >>>>> - || DECL_EXTERNAL (decl) >>>>> - || TREE_PUBLIC (decl)))) >>>>> + || VOID_TYPE_P (TREE_TYPE (decl)) >>>>> + || (chkp_variable_size_type (TREE_TYPE (decl)) >>>>> + && (TREE_STATIC (decl) >>>>> + || DECL_EXTERNAL (decl) >>>>> + || TREE_PUBLIC (decl)))) >>>>> { >>>>> gcc_assert (VAR_P (decl)); >>>>> bounds = chkp_generate_extern_var_bounds (decl); >>>> >>>> Looking one more time into this issue I now think that bounds generation >>>> already uses size relocation and this is not a part to fix. >>>> >>>> BTW this patch just restricts static bounds creation for void vars which >>>> is not what you need. I think you should be able to reproduce the problem >>>> without any patch by just using -fno-chkp-use-static-bounds. >>>> >>>> Looks like gimple doesn't allow void vars as call arguments. I don't know >>>> what is the best way to handle this restriction. The simplest thing a can >>>> think of is to change builtin call so it gets address of a decl instead of >>>> a decl. >>>> >>>> Thanks, >>>> Ilya >>>> >>>>> >>>>> Thanks, >>>>> Alexander >>>>> >>>>> 2017-05-10 20:55 GMT+02:00 Ilya Enkovich <enkovich.gnu@gmail.com>: >>>>>> Hi, >>>>>> >>>>>> Please share a patch you use. >>>>>> >>>>>> Thanks, >>>>>> Ilya >>>>>> >>>>>> 2017-05-09 18:39 GMT+03:00 Alexander Ivchenko <aivchenk@gmail.com>: >>>>>>> If we use chkp_generate_extern_var_bounds for void variable just as >>>>>>> for arrays with unknown size, we will create the following gimple seq: >>>>>>> >>>>>>> # VUSE <.MEM> >>>>>>> __size_tmp.0 = __builtin_ia32_sizeof (foo); >>>>>>> __size_tmp.1_3 = __size_tmp.0; >>>>>>> >>>>>>> However, this will fail in verify_gimple_call: >>>>>>> >>>>>>> tree arg = gimple_call_arg (stmt, i); >>>>>>> if ((is_gimple_reg_type (TREE_TYPE (arg)) >>>>>>> && !is_gimple_val (arg)) >>>>>>> || (!is_gimple_reg_type (TREE_TYPE (arg)) >>>>>>> && !is_gimple_lvalue (arg))) >>>>>>> { >>>>>>> error ("invalid argument to gimple call"); >>>>>>> debug_generic_expr (arg); >>>>>>> return true; >>>>>>> } >>>>>>> ..here the TREE_TYPE(arg)==void. Any ideas for a good workaround ? >>>>>>> >>>>>>> Alexander >>>>>>> >>>>>>> >>>>>>> >>>>>>> 2017-04-08 21:59 GMT+02:00 Ilya Enkovich <enkovich.gnu@gmail.com>: >>>>>>>> 2017-04-04 18:34 GMT+03:00 Jeff Law <law@redhat.com>: >>>>>>>>> On 04/04/2017 09:07 AM, Alexander Ivchenko wrote: >>>>>>>>>> >>>>>>>>>> Hi, >>>>>>>>>> >>>>>>>>>> When creating static bounds for foo below we end up with: >>>>>>>>>> >>>>>>>>>> *((unsigned long *) &__chkp_bounds_of_foo + 8) = >>>>>>>>>> ~(__builtin_ia32_sizeof (foo) + ((long unsigned int) &foo + >>>>>>>>>> 18446744073709551615)); >>>>>>>>>> >>>>>>>>>> This fails in gimplify_function_tree with gcc_assert (!VOID_TYPE_P >>>>>>>>>> (TREE_TYPE (*expr_p))); >>>>>>>>>> >>>>>>>>>> Is it OK? >>>>>>>>>> >>>>>>>>>> gcc/ChangeLog: >>>>>>>>>> >>>>>>>>>> 2017-04-04 Alexander Ivchenko <aivchenk@gmail.com> >>>>>>>>>> >>>>>>>>>> * tree-chkp.c (chkp_get_bounds_for_decl_addr): >>>>>>>>>> assigning zero bounds to void variables >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> gcc/testsuite/ChangeLog: >>>>>>>>>> >>>>>>>>>> 2017-04-04 Alexander Ivchenko <aivchenk@gmail.com> >>>>>>>>>> >>>>>>>>>> * gcc.target/i386/mpx/PR79987.c: New test. >>>>>>>>> >>>>>>>>> I've put this (and other CHKP fixes) in the queue for gcc-8 as AFAICT it's >>>>>>>>> not a regression. >>>>>>>>> >>>>>>>>> Jeff >>>>>>>>> >>>>>>>> >>>>>>>> Hi, >>>>>>>> >>>>>>>> If we delay it for GCC8 anyway then I think we may fix it in a better way. If we >>>>>>>> cannot detect size of a variable then size relocations may be used. It is done >>>>>>>> already for arrays with unknown size and also can be done for void vars. >>>>>>>> >>>>>>>> Thanks, >>>>>>>> Ilya ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-06-09 17:46 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-04 15:07 Fix for PR79987 Alexander Ivchenko 2017-04-04 15:34 ` Jeff Law 2017-04-08 19:59 ` Ilya Enkovich 2017-05-09 15:41 ` Alexander Ivchenko [not found] ` <CAMbmDYYj++Trk1JHs0dXBR4NfLCaKNnzLHN1PBF7a_96YCJGWg@mail.gmail.com> [not found] ` <CACysShjZboYogD31Ujgt-xpE3jK2co1nUpEzxbbrSWbOV_Z4Tg@mail.gmail.com> [not found] ` <CAMbmDYbHjZEXz3YcTvPQa950sxmR3fa+yYsCPkzhychFtjW4Cw@mail.gmail.com> [not found] ` <CACysShip_WNDryk1kUiJCO0Mpc_8SVNRkByTdmzEC0xfxFuviw@mail.gmail.com> 2017-05-11 18:41 ` Ilya Enkovich 2017-06-09 12:22 ` Alexander Ivchenko 2017-06-09 17:46 ` Ilya Enkovich
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).