[v1.1] tree-optimization/105736: Don't let error_mark_node escape for ADDR_EXPR
Commit Message
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(-)
Comments
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
@@ -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 ();
@@ -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;