| Message ID | 20220614153154.366953-1-siddhesh@gotplt.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v1.1] tree-optimization/105736: Don't let error_mark_node escape for ADDR_EXPR | expand |
Hello, ping! On 14/06/2022 21:01, Siddhesh Poyarekar wrote: > The addr_expr computation does not check for error_mark_node before > returning the size expression. This used to work in the constant case > because the conversion to uhwi would end up causing it to return > size_unknown, but that won't work for the dynamic case. > > Modify the control flow to explicitly return size_unknown if the offset > computation returns an error_mark_node. > > gcc/ChangeLog: > > PR tree-optimization/105736 > * tree-object-size.cc (addr_object_size): Return size_unknown > when object offset computation returns an error. > > gcc/testsuite/ChangeLog: > > PR tree-optimization/105736 > * gcc.dg/builtin-dynamic-object-size-0.c (TV4, val3, > test_pr105736): New struct declaration, variable and function to > test PR. > (main): Use them. > > Signed-off-by: Siddhesh Poyarekar <siddhesh@gotplt.org> > --- > Changes from v1: > - Used FAIL() instead of __builtin_abort() in the test. > > Tested: > > - x86_64 bootstrap and test > - --with-build-config=bootstrap-ubsan build > > May I also backport this to gcc12? > > .../gcc.dg/builtin-dynamic-object-size-0.c | 18 +++++++++++++++++ > gcc/tree-object-size.cc | 20 ++++++++++--------- > 2 files changed, 29 insertions(+), 9 deletions(-) > > diff --git a/gcc/testsuite/gcc.dg/builtin-dynamic-object-size-0.c b/gcc/testsuite/gcc.dg/builtin-dynamic-object-size-0.c > index b5b0b3a677c..01a280b2d7b 100644 > --- a/gcc/testsuite/gcc.dg/builtin-dynamic-object-size-0.c > +++ b/gcc/testsuite/gcc.dg/builtin-dynamic-object-size-0.c > @@ -479,6 +479,20 @@ test_loop (int *obj, size_t sz, size_t start, size_t end, int incr) > return __builtin_dynamic_object_size (ptr, 0); > } > > +/* Other tests. */ > + > +struct TV4 > +{ > + __attribute__((vector_size (sizeof (int) * 4))) int v; > +}; > + > +struct TV4 val3; > +int * > +test_pr105736 (struct TV4 *a) > +{ > + return &a->v[0]; > +} > + > unsigned nfails = 0; > > #define FAIL() ({ \ > @@ -633,6 +647,10 @@ main (int argc, char **argv) > FAIL (); > if (test_loop (arr, 42, 20, 52, 1) != 0) > FAIL (); > + /* pr105736. */ > + int *t = test_pr105736 (&val3); > + if (__builtin_dynamic_object_size (t, 0) != -1) > + FAIL (); > > if (nfails > 0) > __builtin_abort (); > diff --git a/gcc/tree-object-size.cc b/gcc/tree-object-size.cc > index 5ca87ae3504..12bc0868b77 100644 > --- a/gcc/tree-object-size.cc > +++ b/gcc/tree-object-size.cc > @@ -695,19 +695,21 @@ addr_object_size (struct object_size_info *osi, const_tree ptr, > var_size = pt_var_size; > bytes = compute_object_offset (TREE_OPERAND (ptr, 0), var); > if (bytes != error_mark_node) > - bytes = size_for_offset (var_size, bytes); > - if (var != pt_var > - && pt_var_size > - && TREE_CODE (pt_var) == MEM_REF > - && bytes != error_mark_node) > { > - tree bytes2 = compute_object_offset (TREE_OPERAND (ptr, 0), pt_var); > - if (bytes2 != error_mark_node) > + bytes = size_for_offset (var_size, bytes); > + if (var != pt_var && pt_var_size && TREE_CODE (pt_var) == MEM_REF) > { > - bytes2 = size_for_offset (pt_var_size, bytes2); > - bytes = size_binop (MIN_EXPR, bytes, bytes2); > + tree bytes2 = compute_object_offset (TREE_OPERAND (ptr, 0), > + pt_var); > + if (bytes2 != error_mark_node) > + { > + bytes2 = size_for_offset (pt_var_size, bytes2); > + bytes = size_binop (MIN_EXPR, bytes, bytes2); > + } > } > } > + else > + bytes = size_unknown (object_size_type); > > wholebytes > = object_size_type & OST_SUBOBJECT ? var_size : pt_var_wholesize;
On Tue, Jun 14, 2022 at 09:01:54PM +0530, Siddhesh Poyarekar wrote: > The addr_expr computation does not check for error_mark_node before > returning the size expression. This used to work in the constant case > because the conversion to uhwi would end up causing it to return > size_unknown, but that won't work for the dynamic case. Regarding subject/first line of commit, it should be something like: tree-object-size: Don't let error_mark_node escape for ADDR_EXPR [PR105736] instead of what you have. > Modify the control flow to explicitly return size_unknown if the offset > computation returns an error_mark_node. > > gcc/ChangeLog: > > PR tree-optimization/105736 > * tree-object-size.cc (addr_object_size): Return size_unknown > when object offset computation returns an error. > > gcc/testsuite/ChangeLog: > > PR tree-optimization/105736 > * gcc.dg/builtin-dynamic-object-size-0.c (TV4, val3, > test_pr105736): New struct declaration, variable and function to > test PR. If you want to spell the exact changes in the test, it would be better to do it separately when it is different changes. * gcc.dg/builtin-dynamic-object-size-0.c (struct TV4): New type. (val3): New variable. (test_pr105736): New function. > (main): Use them. Otherwise LGTM, but for GCC 13, it would be nice to add support for BIT_FIELD_REF if both second and third arguments are multiples of BITS_PER_UNIT. Something like: case BIT_FIELD_REF: if (!tree_fits_shwi_p (TREE_OPERAND (expr, 1)) || !tree_fits_shwi_p (TREE_OPERAND (expr, 2)) || (tree_to_shwi (TREE_OPERAND (expr, 1)) % BITS_PER_UNIT) || (tree_to_shwi (TREE_OPERAND (expr, 2)) % BITS_PER_UNIT)) return error_mark_node; base = compute_object_offset (TREE_OPERAND (expr, 0), var); if (base == error_mark_node) return base; off = size_int (tree_to_shwi (TREE_OPERAND (expr, 2) / BITS_PER_UNIT)); break; or so. > > Signed-off-by: Siddhesh Poyarekar <siddhesh@gotplt.org> > --- > Changes from v1: > - Used FAIL() instead of __builtin_abort() in the test. > > Tested: > > - x86_64 bootstrap and test > - --with-build-config=bootstrap-ubsan build > > May I also backport this to gcc12? Ok. Jakub
On 20/06/2022 15:20, Jakub Jelinek wrote: > On Tue, Jun 14, 2022 at 09:01:54PM +0530, Siddhesh Poyarekar wrote: >> The addr_expr computation does not check for error_mark_node before >> returning the size expression. This used to work in the constant case >> because the conversion to uhwi would end up causing it to return >> size_unknown, but that won't work for the dynamic case. > > Regarding subject/first line of commit, it should be something like: > > tree-object-size: Don't let error_mark_node escape for ADDR_EXPR [PR105736] > instead of what you have. > >> Modify the control flow to explicitly return size_unknown if the offset >> computation returns an error_mark_node. >> >> gcc/ChangeLog: >> >> PR tree-optimization/105736 >> * tree-object-size.cc (addr_object_size): Return size_unknown >> when object offset computation returns an error. >> >> gcc/testsuite/ChangeLog: >> >> PR tree-optimization/105736 >> * gcc.dg/builtin-dynamic-object-size-0.c (TV4, val3, >> test_pr105736): New struct declaration, variable and function to >> test PR. > > If you want to spell the exact changes in the test, it would be better > to do it separately when it is different changes. Thanks, fixed up before pushing to master. > * gcc.dg/builtin-dynamic-object-size-0.c (struct TV4): New type. > (val3): New variable. > (test_pr105736): New function. >> (main): Use them. > > Otherwise LGTM, but for GCC 13, it would be nice to add support > for BIT_FIELD_REF if both second and third arguments are multiples of > BITS_PER_UNIT. Ack, I'll test and post this as a separate change. >> Signed-off-by: Siddhesh Poyarekar <siddhesh@gotplt.org> >> --- >> Changes from v1: >> - Used FAIL() instead of __builtin_abort() in the test. >> >> Tested: >> >> - x86_64 bootstrap and test >> - --with-build-config=bootstrap-ubsan build >> >> May I also backport this to gcc12? > > Ok. Thanks, I'll give it a couple of days in master and then cherry-pick. Sid
diff --git a/gcc/testsuite/gcc.dg/builtin-dynamic-object-size-0.c b/gcc/testsuite/gcc.dg/builtin-dynamic-object-size-0.c index b5b0b3a677c..01a280b2d7b 100644 --- a/gcc/testsuite/gcc.dg/builtin-dynamic-object-size-0.c +++ b/gcc/testsuite/gcc.dg/builtin-dynamic-object-size-0.c @@ -479,6 +479,20 @@ test_loop (int *obj, size_t sz, size_t start, size_t end, int incr) return __builtin_dynamic_object_size (ptr, 0); } +/* Other tests. */ + +struct TV4 +{ + __attribute__((vector_size (sizeof (int) * 4))) int v; +}; + +struct TV4 val3; +int * +test_pr105736 (struct TV4 *a) +{ + return &a->v[0]; +} + unsigned nfails = 0; #define FAIL() ({ \ @@ -633,6 +647,10 @@ main (int argc, char **argv) FAIL (); if (test_loop (arr, 42, 20, 52, 1) != 0) FAIL (); + /* pr105736. */ + int *t = test_pr105736 (&val3); + if (__builtin_dynamic_object_size (t, 0) != -1) + FAIL (); if (nfails > 0) __builtin_abort (); diff --git a/gcc/tree-object-size.cc b/gcc/tree-object-size.cc index 5ca87ae3504..12bc0868b77 100644 --- a/gcc/tree-object-size.cc +++ b/gcc/tree-object-size.cc @@ -695,19 +695,21 @@ addr_object_size (struct object_size_info *osi, const_tree ptr, var_size = pt_var_size; bytes = compute_object_offset (TREE_OPERAND (ptr, 0), var); if (bytes != error_mark_node) - bytes = size_for_offset (var_size, bytes); - if (var != pt_var - && pt_var_size - && TREE_CODE (pt_var) == MEM_REF - && bytes != error_mark_node) { - tree bytes2 = compute_object_offset (TREE_OPERAND (ptr, 0), pt_var); - if (bytes2 != error_mark_node) + bytes = size_for_offset (var_size, bytes); + if (var != pt_var && pt_var_size && TREE_CODE (pt_var) == MEM_REF) { - bytes2 = size_for_offset (pt_var_size, bytes2); - bytes = size_binop (MIN_EXPR, bytes, bytes2); + tree bytes2 = compute_object_offset (TREE_OPERAND (ptr, 0), + pt_var); + if (bytes2 != error_mark_node) + { + bytes2 = size_for_offset (pt_var_size, bytes2); + bytes = size_binop (MIN_EXPR, bytes, bytes2); + } } } + else + bytes = size_unknown (object_size_type); wholebytes = object_size_type & OST_SUBOBJECT ? var_size : pt_var_wholesize;
The addr_expr computation does not check for error_mark_node before returning the size expression. This used to work in the constant case because the conversion to uhwi would end up causing it to return size_unknown, but that won't work for the dynamic case. Modify the control flow to explicitly return size_unknown if the offset computation returns an error_mark_node. gcc/ChangeLog: PR tree-optimization/105736 * tree-object-size.cc (addr_object_size): Return size_unknown when object offset computation returns an error. gcc/testsuite/ChangeLog: PR tree-optimization/105736 * gcc.dg/builtin-dynamic-object-size-0.c (TV4, val3, test_pr105736): New struct declaration, variable and function to test PR. (main): Use them. Signed-off-by: Siddhesh Poyarekar <siddhesh@gotplt.org> --- Changes from v1: - Used FAIL() instead of __builtin_abort() in the test. Tested: - x86_64 bootstrap and test - --with-build-config=bootstrap-ubsan build May I also backport this to gcc12? .../gcc.dg/builtin-dynamic-object-size-0.c | 18 +++++++++++++++++ gcc/tree-object-size.cc | 20 ++++++++++--------- 2 files changed, 29 insertions(+), 9 deletions(-)