[PATCH] middle-end/105604 - snprintf dianostics and non-constant sizes/offsets
Martin Sebor
msebor@redhat.com
Wed May 18 22:42:35 GMT 2022
On 5/18/22 00:26, Richard Biener wrote:
> On Tue, 17 May 2022, Martin Sebor wrote:
>
>> On 5/16/22 03:16, Richard Biener wrote:
>>> The following tries to correct get_origin_and_offset_r not handling
>>> non-constant sizes of array elements in ARRAY_REFs and non-constant
>>> offsets of COMPONENT_REFs. It isn't exactly clear how such failures
>>> should be treated in this API and existing handling isn't consistent
>>> here either. The following applies two different variants, treating
>>> non-constant array sizes like non-constant array indices and
>>> treating non-constant offsets of COMPONENT_REFs by terminating
>>> the recursion (not sure what that means to the callers).
>>>
>>> Basically the code failed to use component_ref_field_offset and
>>> array_ref_element_size and instead relies on inappropriate
>>> helpers (that shouldn't exist in the first place ...). The code
>>> is also not safe-guarded against overflows in the final offset/size
>>> computations but I'm not trying to rectify that.
>>>
>>> Martin - can you comment on how the API should handle such
>>> situations?
>>
>> It looks like the -Wrestrict warning here ignores offsets equal to
>> HOST_WIDE_INT_MIN so presumably setting dst_offset (via *fldoff) to
>> that should avoid it. Or maybe to HWI_MAX as it does for variable
>> offsets.
>
> Can you suggest wording for the function comment as to how it handles
> the case when offset or size cannot be determined exactly? The
> comment currently only suggests that the caller possibly cannot
> trust fldsize or off when the function returns NULL but the actual
> implementation differs from that.
>
>> It also looks like the function only handles constant offsets and
>> sizes, and I have a vague recollection of enhancing it to work with
>> ranges. That should avoid the overflow problem too.
>
> So the correct thing is to return NULL?
No, I don't think so. The recursive get_origin_and_offset_r() assumes
its own invocations never return null (the one place it does that should
probably be moved to the nonrecursive caller).
>
> Is the patch OK as-is?
It's an improvement but it's not complete as the following also ICEs
(albeit somewhere else):
void* f (void);
void g (int n)
{
struct {
char a[n], b[];
} *p = f ();
__builtin_sprintf (p->b, "%s", p->a);
}
With the ICE fixed the warning triggers. That's not ideal but it's
unavoidable given the IR (I believe I mentioned this caveat some time
back). This is the same as for:
struct {
char a[8], b[8];
} *p = f ();
__builtin_sprintf (&p->b[n], "%s", p->a);
because the IR looks more or less the same for &p->a[n] as it is for
&p->b[n].
> As said, I'm not sure how the caller interprets
> the result and how it can distinguish the exact vs. non-exact cases
> or what a "conservative" inexact answer would be.
The warning triggers in both the certain cases and the inexact
ones like the one above when an overlap cannot be ruled out. To
differentiate the two it's phrased as "may overlap". The handling
is in maybe_warn_overlap().
>
> Please help properly documenting this API.
I can spend some time in the next few days to page it all in, see
if I can clean it up a bit in addition to fixing the ICEs and
improve the comment. Let me know if you have a different
preference.
Martin
>
> Thanks,
> Richard.
>
>> Martin
>>
>>>
>>> Bootstrapped and tested on x86_64-unknown-linux-gnu.
>>>
>>> OK for trunk and branches?
>>>
>>> Thanks,
>>> Richard.
>>>
>>> 2022-05-16 Richard Biener <rguenther@suse.de>
>>>
>>> PR middle-end/105604
>>> * gimple-ssa-sprintf.cc (get_origin_and_offset_r):
>>> Handle non-constant ARRAY_REF element size and non-constant
>>> COMPONENT_REF field offset.
>>>
>>> * gcc.dg/torture/pr105604.c: New testcase.
>>> ---
>>> gcc/gimple-ssa-sprintf.cc | 14 +++++++++++---
>>> gcc/testsuite/gcc.dg/torture/pr105604.c | 24 ++++++++++++++++++++++++
>>> 2 files changed, 35 insertions(+), 3 deletions(-)
>>> create mode 100644 gcc/testsuite/gcc.dg/torture/pr105604.c
>>>
>>> diff --git a/gcc/gimple-ssa-sprintf.cc b/gcc/gimple-ssa-sprintf.cc
>>> index c93f12f90b5..14e215ce69c 100644
>>> --- a/gcc/gimple-ssa-sprintf.cc
>>> +++ b/gcc/gimple-ssa-sprintf.cc
>>> @@ -2312,14 +2312,16 @@ get_origin_and_offset_r (tree x, HOST_WIDE_INT
>>> *fldoff, HOST_WIDE_INT *fldsize,
>>> HOST_WIDE_INT idx = (tree_fits_uhwi_p (offset)
>>> ? tree_to_uhwi (offset) : HOST_WIDE_INT_MAX);
>>> + tree elsz = array_ref_element_size (x);
>>> tree eltype = TREE_TYPE (x);
>>> if (TREE_CODE (eltype) == INTEGER_TYPE)
>>> {
>>> if (off)
>>> *off = idx;
>>> }
>>> - else if (idx < HOST_WIDE_INT_MAX)
>>> - *fldoff += idx * int_size_in_bytes (eltype);
>>> + else if (idx < HOST_WIDE_INT_MAX
>>> + && tree_fits_shwi_p (elsz))
>>> + *fldoff += idx * tree_to_shwi (elsz);
>>> else
>>> *fldoff = idx;
>>> @@ -2350,8 +2352,14 @@ get_origin_and_offset_r (tree x, HOST_WIDE_INT
>>> *fldoff, HOST_WIDE_INT *fldsize,
>>>
>>> case COMPONENT_REF:
>>> {
>>> + tree foff = component_ref_field_offset (x);
>>> tree fld = TREE_OPERAND (x, 1);
>>> - *fldoff += int_byte_position (fld);
>>> + if (!tree_fits_shwi_p (foff)
>>> + || !tree_fits_shwi_p (DECL_FIELD_BIT_OFFSET (fld)))
>>> + return x;
>>> + *fldoff += (tree_to_shwi (foff)
>>> + + (tree_to_shwi (DECL_FIELD_BIT_OFFSET (fld))
>>> + / BITS_PER_UNIT));
>>>
>>> get_origin_and_offset_r (fld, fldoff, fldsize, off);
>>> x = TREE_OPERAND (x, 0);
>>> diff --git a/gcc/testsuite/gcc.dg/torture/pr105604.c
>>> b/gcc/testsuite/gcc.dg/torture/pr105604.c
>>> new file mode 100644
>>> index 00000000000..b002251df10
>>> --- /dev/null
>>> +++ b/gcc/testsuite/gcc.dg/torture/pr105604.c
>>> @@ -0,0 +1,24 @@
>>> +/* { dg-do compile } */
>>> +/* { dg-additional-options "-Wall" } */
>>> +
>>> +struct {
>>> + long users;
>>> + long size;
>>> + char *data;
>>> +} * main_trans;
>>> +void *main___trans_tmp_1;
>>> +int sprintf(char *, char *, ...);
>>> +int main() {
>>> + int users = 0;
>>> + struct {
>>> + long users;
>>> + long size;
>>> + char *data;
>>> + int links[users];
>>> + char buf[];
>>> + } *trans = trans;
>>> + trans->data = trans->buf;
>>> + main___trans_tmp_1 = trans;
>>> + main_trans = main___trans_tmp_1;
>>> + sprintf(main_trans->data, "test");
>>> +}
>>
>>
>>
>
More information about the Gcc-patches
mailing list