Re: PING 2 [PATCH] correct handling of variable offset minus constant in -Warray-bounds (PR 100137)

[Martin Sebor <msebor@gmail.com>]

On 7/7/21 1:38 AM, Richard Biener wrote:
> On Tue, Jul 6, 2021 at 5:47 PM Martin Sebor via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>> Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-June/573349.html
> 
> +  if (TREE_CODE (axstype) != UNION_TYPE)
> 
> what about QUAL_UNION_TYPE?  (why constrain union type accesses
> here - note you don't seem to constrain accesses of union members here)

I didn't know a QUAL_UNION_TYPE was a thing.  Removing the test
doesn't seem to cause any regressions so let me do that in a followup.

> 
> +    if (tree access_size = TYPE_SIZE_UNIT (axstype))
> 
> +  /* The byte size of the array has already been determined above
> +     based on a pointer ARG.  Set ELTSIZE to the size of the type
> +     it points to and REFTYPE to the array with the size, rounded
> +     down as necessary.  */
> +  if (POINTER_TYPE_P (reftype))
> +    reftype = TREE_TYPE (reftype);
> +  if (TREE_CODE (reftype) == ARRAY_TYPE)
> +    reftype = TREE_TYPE (reftype);
> +  if (tree refsize = TYPE_SIZE_UNIT (reftype))
> +    if (TREE_CODE (refsize) == INTEGER_CST)
> +      eltsize = wi::to_offset (refsize);
> 
> probably pre-existing but the pointer indirection is definitely confusing
> me again and again given the variable is named 'reftype' - obviously
> an access to a pointer does not have any element size.  Possibly the
> paths arriving here ensure somehow that the only case is when
> reftype is not the access type but a pointer to the accessed memory.
> "jump-threading" the source might help me avoiding to trip over this
> again and again ...

I agree (it is confusing).  There's more to simplify here.  It's on
my to do list so let me see about this piece of code then.

> 
> The patch removes a lot of odd code, I like that.  You know this code best
> and it's hard to spot errors.
> 
> So OK, you'll deal with the fallout.

I certainly will.  Pushed in r12-2132.

Thanks
Martin

> 
> Thanks,
> Richard.
> 
>> On 6/28/21 1:33 PM, Martin Sebor wrote:
>>> Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-June/573349.html
>>>
>>> On 6/21/21 4:25 PM, Martin Sebor wrote:
>>>> -Warray-bounds relies on similar logic as -Wstringop-overflow et al.,
>>>> but using its own algorithm, including its own bugs such as PR 100137.
>>>> The attached patch takes the first step toward unifying the logic
>>>> between the warnings.  It changes a subset of -Warray-bounds to call
>>>> compute_objsize() to detect out-of-bounds indices.  Besides fixing
>>>> the bug this also nicely simplifies the code and improves
>>>> the consistency between the informational messages printed by both
>>>> classes of warnings.
>>>>
>>>> The changes to the test suite are extensive mainly because of
>>>> the different format of the diagnostics resulting from slightly
>>>> tighter bounds of offsets computed by the new algorithm, and in
>>>> smaller part because the change lets -Warray-bounds diagnose some
>>>> problems it previously missed due to the limitations of its own
>>>> solution.
>>>>
>>>> The false positive reported in PR 100137 is a 10/11/12 regression
>>>> but this change is too intrusive to backport.  I have a smaller
>>>> and more targeted patch I plan to backport in its stead.
>>>>
>>>> Tested on x86_64-linux.
>>>>
>>>> Martin
>>>
>>