Re: [PATCH] issue consistent warning for past-the-end array stores (PR 91457)
On 8/28/19 9:44 AM, Martin Sebor wrote: > With the PR 91490 subset committed, attached is what's left > of the original fix. It seems simple enough that I decided it's > not worth chopping up any further. The meat of the change is in > builtins.c. Everything else just adjusts warnings either by > making use of it (tree-ssa-strlen.c) or by setting the no-warning > bit (tree-vrp.c). > > Retested on x86_64-linux. > > Martin > > gcc-91457.diff > > PR tree-optimization/91457 - inconsistent warning for writing past the end of > an array member > > gcc/ChangeLog: > > PR tree-optimization/91457 > * builtins.c (component_size): New function. > (compute_objsize): Add argument. Handle ARRAY_REF and COMPONENT_REF. > * builtins.h (compute_objsize): Add argument. > * tree-ssa-strlen.c (handle_store): Handle no-warning bit. > * tree-vrp.c (vrp_prop::check_array_ref): Return warning result. > (vrp_prop::check_mem_ref): Same. > (vrp_prop::search_for_addr_array): Set no-warning bit. > (check_array_bounds): Same. > > gcc/testsuite/ChangeLog: > > PR tree-optimization/91457 > * c-c++-common/Wstringop-overflow-2.c: New test. > * g++.dg/warn/Warray-bounds-8.C: New test. > * g++.dg/warn/Wstringop-overflow-3.C: New test. > * gcc.dg/Wstringop-overflow-15.c: New test. OK jeff
Re: [PATCH] issue consistent warning for past-the-end array stores (PR 91457)
With the PR 91490 subset committed, attached is what's left
of the original fix. It seems simple enough that I decided it's
not worth chopping up any further. The meat of the change is in
builtins.c. Everything else just adjusts warnings either by
making use of it (tree-ssa-strlen.c) or by setting the no-warning
bit (tree-vrp.c).
Retested on x86_64-linux.
Martin
PR tree-optimization/91457 - inconsistent warning for writing past the end of an array member
gcc/ChangeLog:
PR tree-optimization/91457
* builtins.c (component_size): New function.
(compute_objsize): Add argument. Handle ARRAY_REF and COMPONENT_REF.
* builtins.h (compute_objsize): Add argument.
* tree-ssa-strlen.c (handle_store): Handle no-warning bit.
* tree-vrp.c (vrp_prop::check_array_ref): Return warning result.
(vrp_prop::check_mem_ref): Same.
(vrp_prop::search_for_addr_array): Set no-warning bit.
(check_array_bounds): Same.
gcc/testsuite/ChangeLog:
PR tree-optimization/91457
* c-c++-common/Wstringop-overflow-2.c: New test.
* g++.dg/warn/Warray-bounds-8.C: New test.
* g++.dg/warn/Wstringop-overflow-3.C: New test.
* gcc.dg/Wstringop-overflow-15.c: New test.
diff --git a/gcc/builtins.c b/gcc/builtins.c
index 0b25adc17a0..f8063c138a3 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -72,6 +72,7 @@ along with GCC; see the file COPYING3. If not see
#include "file-prefix-map.h" /* remap_macro_filename() */
#include "gomp-constants.h"
#include "omp-general.h"
+#include "tree-dfa.h"
struct target_builtins default_target_builtins;
#if SWITCHABLE_TARGET
@@ -3561,6 +3562,54 @@ check_access (tree exp, tree, tree, tree dstwrite,
return true;
}
+/* Determines the size of the member referenced by the COMPONENT_REF
+ REF, using its initializer expression if necessary in order to
+ determine the size of an initialized flexible array member.
+ Returns the size (which might be zero for an object with
+ an uninitialized flexible array member) or null if the size
+ cannot be determined. */
+
+static tree
+component_size (tree ref)
+{
+ gcc_assert (TREE_CODE (ref) == COMPONENT_REF);
+
+ tree member = TREE_OPERAND (ref, 1);
+
+ /* If the member is not last or has a size greater than one, return
+ it. Otherwise it's either a flexible array member or a zero-length
+ array member, or an array of length one treated as such. */
+ tree size = DECL_SIZE_UNIT (member);
+ if (size
+ && (!array_at_struct_end_p (ref)
+ || (!integer_zerop (size)
+ && !integer_onep (size
+return size;
+
+ /* If the reference is to a declared object and the member a true
+ flexible array, try to determine its size from its initializer. */
+ poly_int64 off = 0;
+ tree base = get_addr_base_and_unit_offset (ref, );
+ if (!base || !VAR_P (base))
+return NULL_TREE;
+
+ /* The size of any member of a declared object other than a flexible
+ array member is that obtained above. */
+ if (size)
+return size;
+
+ if (tree init = DECL_INITIAL (base))
+if (TREE_CODE (init) == CONSTRUCTOR)
+ {
+ off <<= LOG2_BITS_PER_UNIT;
+ init = fold_ctor_reference (NULL_TREE, init, off, 0, base);
+ if (init)
+ return TYPE_SIZE_UNIT (TREE_TYPE (init));
+ }
+
+ return DECL_EXTERNAL (base) ? NULL_TREE : integer_zero_node;
+}
+
/* Helper to compute the size of the object referenced by the DEST
expression which must have pointer type, using Object Size type
OSTYPE (only the least significant 2 bits are used). Return
@@ -3568,12 +3617,18 @@ check_access (tree exp, tree, tree, tree dstwrite,
the size cannot be determined. When the referenced object involves
a non-constant offset in some range the returned value represents
the largest size given the smallest non-negative offset in the
- range. The function is intended for diagnostics and should not
- be used to influence code generation or optimization. */
+ range. If nonnull, set *PDECL to the decl of the referenced
+ subobject if it can be determined, or to null otherwise.
+ The function is intended for diagnostics and should not be used
+ to influence code generation or optimization. */
tree
-compute_objsize (tree dest, int ostype)
+compute_objsize (tree dest, int ostype, tree *pdecl /* = NULL */)
{
+ tree dummy = NULL_TREE;
+ if (!pdecl)
+pdecl =
+
unsigned HOST_WIDE_INT size;
/* Only the two least significant bits are meaningful. */
@@ -3600,7 +3655,7 @@ compute_objsize (tree dest, int ostype)
tree off = gimple_assign_rhs2 (stmt);
if (TREE_CODE (off) == INTEGER_CST)
{
- if (tree size = compute_objsize (dest, ostype))
+ if (tree size = compute_objsize (dest, ostype, pdecl))
{
wide_int wioff = wi::to_wide (off);
wide_int wisiz = wi::to_wide (size);
@@ -3625,7 +3680,7 @@ compute_objsize (tree dest, int ostype)
if (rng == VR_RANGE)
{
- if (tree size = compute_objsize (dest, ostype))
+ if (tree size = compute_objsize (dest, ostype,
Re: [PATCH] issue consistent warning for past-the-end array stores (PR 91457)
On Thu, Aug 22, 2019 at 3:43 AM Richard Biener
wrote:
> On Wed, Aug 21, 2019 at 10:10 PM Martin Sebor wrote:
> > On 8/20/19 1:26 AM, Richard Biener wrote:
> > > On Tue, Aug 20, 2019 at 4:32 AM Martin Sebor wrote:
> > >> On 8/19/19 8:10 AM, Richard Biener wrote:
> > >>> On Sat, Aug 17, 2019 at 12:43 AM Martin Sebor wrote:
> >
> > With the recent enhancement to the strlen handling of multibyte
> > stores the g++.dg/warn/Warray-bounds-4.C for zero-length arrays
> > started failing on hppa (and probably elsewhere as well). This
> > is partly the result of the added detection of past-the-end
> > writes into the strlen pass which detects more instances of
> > the problem than -Warray-bounds. Since the IL each warning
> > works with varies between targets, the same invalid code can
> > be diagnosed by one warning one target and different warning
> > on another.
> >
> > The attached patch does three things:
> >
> > 1) It enhances compute_objsize to also determine the size of
> > a flexible array member (and its various variants), including
> > from its initializer if necessary. (This resolves 91457 but
> > introduces another warning where was previously just one.)
> > 2) It guards the new instance of -Wstringop-overflow with
> > the no-warning bit on the assignment to avoid warning on code
> > that's already been diagnosed.
> > 3) It arranges for -Warray-bounds to set the no-warning bit on
> > the enclosing expression to keep -Wstringop-overflow from issuing
> > another warning for the same problem.
> >
> > Testing the compute_objsize enhancement to bring it up to par
> > with -Warray-bounds in turn exposed a weakness in the latter
> > warning for flexible array members. Rather than snowballing
> > additional improvements into this one I decided to put that
> > off until later, so the new -Warray-bounds test has a bunch
> > of XFAILs. I'll see if I can find the time to deal with those
> > either still in stage 1 or in stage 3 (one of them is actually
> > an ancient regression).
> > >>>
> > >>> +static tree
> > >>> +get_initializer_for (tree init, tree decl)
> > >>> +{
> > >>>
> > >>> can't you use fold_ctor_reference here?
> > >>
> > >> Yes, but only with an additional enhancement. Char initializers
> > >> for flexible array members aren't transformed to STRING_CSTs yet,
> > >> so without the size of the initializer specified, the function
> > >> returns the initializer for the smallest subobject, or char in
> > >> this case. I've enhanced the function to handle them.
> > >
> > > So at the moment it returns an empty array constructor, correct?
> > > Isn't that the more canonical representation? The STRING_CST
> > > index type doesn't really handle "empty" strings and I expect code
> > > more confused about that than about an empty CTOR?
> >
> > Yes. Returning an empty CTOR is more general than an empty
> > string and enables more optimizations. It requires changing
> > the caller(s) that look for a string but I think that's fine.
> > Thanks for the hint!
> >
> > >>> +/* Determine the size of the flexible array FLD from the initializer
> > >>> + expression for the struct object DECL in which the meber is declared
> > >>> + (possibly recursively). Return the size or zero constant if it
> > >>> isn't
> > >>> + initialized. */
> > >>> +
> > >>> +static tree
> > >>> +get_flexarray_size (tree decl, tree fld)
> > >>> +{
> > >>> + if (tree init = DECL_INITIAL (decl))
> > >>> +{
> > >>> + init = get_initializer_for (init, fld);
> > >>> + if (init)
> > >>> + return TYPE_SIZE_UNIT (TREE_TYPE (init));
> > >>> +}
> > >>> +
> > >>> + return integer_zero_node;
> > >>>
> > >>> so you're hoping that the (sub-)CONSTRUCTOR get_initializer_for
> > >>> returns has a complete type but the initialized object didn't get it
> > >>> completed. Isnt that wishful thinking?
> > >>
> > >> I don't know what you mean. When might a CONSTRUCTOR not have
> > >> a complete type, and if/when it doesn't, why would that be
> > >> a problem here? TYPE_SIZE_UNIT will evaluate to null meaning
> > >> "don't know" and that's fine. Could you try to be more specific
> > >> about the problem you're pointing out?
> > >>
> > >>> And why return integer_zero_node
> > >>> rather than NULL_TREE here?
> > >>
> > >> Because the size of a flexible array member with no initializer
> > >> is zero.
> > >>
> > >>>
> > >>> + if (TREE_CODE (dest) == COMPONENT_REF)
> > >>> +{
> > >>> + *pdecl = TREE_OPERAND (dest, 1);
> > >>> +
> > >>> + /* If the member has a size return it. Otherwise it's a flexible
> > >>> +array member. */
> > >>> + if (tree size = DECL_SIZE_UNIT (*pdecl))
> > >>> + return size;
> > >>>
> > >>> because here you do.
> > >>
> > >> Not sure what you mean here either. (This code was also a bit
> > >
> > > You
Re: [PATCH] issue consistent warning for past-the-end array stores (PR 91457)
On 8/20/19 1:26 AM, Richard Biener wrote:
> On Tue, Aug 20, 2019 at 4:32 AM Martin Sebor wrote:
>>
>> On 8/19/19 8:10 AM, Richard Biener wrote:
>>> On Sat, Aug 17, 2019 at 12:43 AM Martin Sebor wrote:
With the recent enhancement to the strlen handling of multibyte
stores the g++.dg/warn/Warray-bounds-4.C for zero-length arrays
started failing on hppa (and probably elsewhere as well). This
is partly the result of the added detection of past-the-end
writes into the strlen pass which detects more instances of
the problem than -Warray-bounds. Since the IL each warning
works with varies between targets, the same invalid code can
be diagnosed by one warning one target and different warning
on another.
The attached patch does three things:
1) It enhances compute_objsize to also determine the size of
a flexible array member (and its various variants), including
from its initializer if necessary. (This resolves 91457 but
introduces another warning where was previously just one.)
2) It guards the new instance of -Wstringop-overflow with
the no-warning bit on the assignment to avoid warning on code
that's already been diagnosed.
3) It arranges for -Warray-bounds to set the no-warning bit on
the enclosing expression to keep -Wstringop-overflow from issuing
another warning for the same problem.
Testing the compute_objsize enhancement to bring it up to par
with -Warray-bounds in turn exposed a weakness in the latter
warning for flexible array members. Rather than snowballing
additional improvements into this one I decided to put that
off until later, so the new -Warray-bounds test has a bunch
of XFAILs. I'll see if I can find the time to deal with those
either still in stage 1 or in stage 3 (one of them is actually
an ancient regression).
>>>
>>> +static tree
>>> +get_initializer_for (tree init, tree decl)
>>> +{
>>>
>>> can't you use fold_ctor_reference here?
>>
>> Yes, but only with an additional enhancement. Char initializers
>> for flexible array members aren't transformed to STRING_CSTs yet,
>> so without the size of the initializer specified, the function
>> returns the initializer for the smallest subobject, or char in
>> this case. I've enhanced the function to handle them.
>
> So at the moment it returns an empty array constructor, correct?
> Isn't that the more canonical representation? The STRING_CST
> index type doesn't really handle "empty" strings and I expect code
> more confused about that than about an empty CTOR?
>
>>>
>>> +/* Determine the size of the flexible array FLD from the initializer
>>> + expression for the struct object DECL in which the meber is declared
>>> + (possibly recursively). Return the size or zero constant if it isn't
>>> + initialized. */
>>> +
>>> +static tree
>>> +get_flexarray_size (tree decl, tree fld)
>>> +{
>>> + if (tree init = DECL_INITIAL (decl))
>>> +{
>>> + init = get_initializer_for (init, fld);
>>> + if (init)
>>> + return TYPE_SIZE_UNIT (TREE_TYPE (init));
>>> +}
>>> +
>>> + return integer_zero_node;
>>>
>>> so you're hoping that the (sub-)CONSTRUCTOR get_initializer_for
>>> returns has a complete type but the initialized object didn't get it
>>> completed. Isnt that wishful thinking?
>>
>> I don't know what you mean. When might a CONSTRUCTOR not have
>> a complete type, and if/when it doesn't, why would that be
>> a problem here? TYPE_SIZE_UNIT will evaluate to null meaning
>> "don't know" and that's fine. Could you try to be more specific
>> about the problem you're pointing out?
>>
>>> And why return integer_zero_node
>>> rather than NULL_TREE here?
>>
>> Because the size of a flexible array member with no initializer
>> is zero.
>>
>>>
>>> + if (TREE_CODE (dest) == COMPONENT_REF)
>>> +{
>>> + *pdecl = TREE_OPERAND (dest, 1);
>>> +
>>> + /* If the member has a size return it. Otherwise it's a flexible
>>> +array member. */
>>> + if (tree size = DECL_SIZE_UNIT (*pdecl))
>>> + return size;
>>>
>>> because here you do.
>>
>> Not sure what you mean here either. (This code was also a bit
>
> You return NULL_TREE.
>
>> out of date WRT to the patch I had tested. Not sure how that
>> happened. The attached patch is up to date.)
>>
>>>
>>> Also once you have an underlying VAR_DECL you can compute
>>> the flexarray size by DECL_SIZE (var) - offset-of flexarray member.
>>> Isn't that way cheaper than walking the initializer (possibly many
>>> times?)
>>
>> It would be nice if it were this easy. Is the value of DECL_SIZE
>> (var) supposed to include the size of the flexible array member?
>
> Yes, DECL_SIZE of the VAR_DECL is the size we use for assembling.
> It is usually only the types that remain incomplete (or too small) since
> the FE doesn't create many variants of a struct S { int n; char x[]; }
> when "instantiating" it
Re: [PATCH] issue consistent warning for past-the-end array stores (PR 91457)
On Wed, Aug 21, 2019 at 10:10 PM Martin Sebor wrote:
>
> On 8/20/19 1:26 AM, Richard Biener wrote:
> > On Tue, Aug 20, 2019 at 4:32 AM Martin Sebor wrote:
> >>
> >> On 8/19/19 8:10 AM, Richard Biener wrote:
> >>> On Sat, Aug 17, 2019 at 12:43 AM Martin Sebor wrote:
>
> With the recent enhancement to the strlen handling of multibyte
> stores the g++.dg/warn/Warray-bounds-4.C for zero-length arrays
> started failing on hppa (and probably elsewhere as well). This
> is partly the result of the added detection of past-the-end
> writes into the strlen pass which detects more instances of
> the problem than -Warray-bounds. Since the IL each warning
> works with varies between targets, the same invalid code can
> be diagnosed by one warning one target and different warning
> on another.
>
> The attached patch does three things:
>
> 1) It enhances compute_objsize to also determine the size of
> a flexible array member (and its various variants), including
> from its initializer if necessary. (This resolves 91457 but
> introduces another warning where was previously just one.)
> 2) It guards the new instance of -Wstringop-overflow with
> the no-warning bit on the assignment to avoid warning on code
> that's already been diagnosed.
> 3) It arranges for -Warray-bounds to set the no-warning bit on
> the enclosing expression to keep -Wstringop-overflow from issuing
> another warning for the same problem.
>
> Testing the compute_objsize enhancement to bring it up to par
> with -Warray-bounds in turn exposed a weakness in the latter
> warning for flexible array members. Rather than snowballing
> additional improvements into this one I decided to put that
> off until later, so the new -Warray-bounds test has a bunch
> of XFAILs. I'll see if I can find the time to deal with those
> either still in stage 1 or in stage 3 (one of them is actually
> an ancient regression).
> >>>
> >>> +static tree
> >>> +get_initializer_for (tree init, tree decl)
> >>> +{
> >>>
> >>> can't you use fold_ctor_reference here?
> >>
> >> Yes, but only with an additional enhancement. Char initializers
> >> for flexible array members aren't transformed to STRING_CSTs yet,
> >> so without the size of the initializer specified, the function
> >> returns the initializer for the smallest subobject, or char in
> >> this case. I've enhanced the function to handle them.
> >
> > So at the moment it returns an empty array constructor, correct?
> > Isn't that the more canonical representation? The STRING_CST
> > index type doesn't really handle "empty" strings and I expect code
> > more confused about that than about an empty CTOR?
>
> Yes. Returning an empty CTOR is more general than an empty
> string and enables more optimizations. It requires changing
> the caller(s) that look for a string but I think that's fine.
> Thanks for the hint!
>
> >>> +/* Determine the size of the flexible array FLD from the initializer
> >>> + expression for the struct object DECL in which the meber is declared
> >>> + (possibly recursively). Return the size or zero constant if it isn't
> >>> + initialized. */
> >>> +
> >>> +static tree
> >>> +get_flexarray_size (tree decl, tree fld)
> >>> +{
> >>> + if (tree init = DECL_INITIAL (decl))
> >>> +{
> >>> + init = get_initializer_for (init, fld);
> >>> + if (init)
> >>> + return TYPE_SIZE_UNIT (TREE_TYPE (init));
> >>> +}
> >>> +
> >>> + return integer_zero_node;
> >>>
> >>> so you're hoping that the (sub-)CONSTRUCTOR get_initializer_for
> >>> returns has a complete type but the initialized object didn't get it
> >>> completed. Isnt that wishful thinking?
> >>
> >> I don't know what you mean. When might a CONSTRUCTOR not have
> >> a complete type, and if/when it doesn't, why would that be
> >> a problem here? TYPE_SIZE_UNIT will evaluate to null meaning
> >> "don't know" and that's fine. Could you try to be more specific
> >> about the problem you're pointing out?
> >>
> >>> And why return integer_zero_node
> >>> rather than NULL_TREE here?
> >>
> >> Because the size of a flexible array member with no initializer
> >> is zero.
> >>
> >>>
> >>> + if (TREE_CODE (dest) == COMPONENT_REF)
> >>> +{
> >>> + *pdecl = TREE_OPERAND (dest, 1);
> >>> +
> >>> + /* If the member has a size return it. Otherwise it's a flexible
> >>> +array member. */
> >>> + if (tree size = DECL_SIZE_UNIT (*pdecl))
> >>> + return size;
> >>>
> >>> because here you do.
> >>
> >> Not sure what you mean here either. (This code was also a bit
> >
> > You return NULL_TREE.
> >
> >> out of date WRT to the patch I had tested. Not sure how that
> >> happened. The attached patch is up to date.)
> >>
> >>>
> >>> Also once you have an underlying VAR_DECL you can compute
> >>> the flexarray size by DECL_SIZE (var) - offset-of
Re: [PATCH] issue consistent warning for past-the-end array stores (PR 91457)
On 8/20/19 1:26 AM, Richard Biener wrote:
On Tue, Aug 20, 2019 at 4:32 AM Martin Sebor wrote:
On 8/19/19 8:10 AM, Richard Biener wrote:
On Sat, Aug 17, 2019 at 12:43 AM Martin Sebor wrote:
With the recent enhancement to the strlen handling of multibyte
stores the g++.dg/warn/Warray-bounds-4.C for zero-length arrays
started failing on hppa (and probably elsewhere as well). This
is partly the result of the added detection of past-the-end
writes into the strlen pass which detects more instances of
the problem than -Warray-bounds. Since the IL each warning
works with varies between targets, the same invalid code can
be diagnosed by one warning one target and different warning
on another.
The attached patch does three things:
1) It enhances compute_objsize to also determine the size of
a flexible array member (and its various variants), including
from its initializer if necessary. (This resolves 91457 but
introduces another warning where was previously just one.)
2) It guards the new instance of -Wstringop-overflow with
the no-warning bit on the assignment to avoid warning on code
that's already been diagnosed.
3) It arranges for -Warray-bounds to set the no-warning bit on
the enclosing expression to keep -Wstringop-overflow from issuing
another warning for the same problem.
Testing the compute_objsize enhancement to bring it up to par
with -Warray-bounds in turn exposed a weakness in the latter
warning for flexible array members. Rather than snowballing
additional improvements into this one I decided to put that
off until later, so the new -Warray-bounds test has a bunch
of XFAILs. I'll see if I can find the time to deal with those
either still in stage 1 or in stage 3 (one of them is actually
an ancient regression).
+static tree
+get_initializer_for (tree init, tree decl)
+{
can't you use fold_ctor_reference here?
Yes, but only with an additional enhancement. Char initializers
for flexible array members aren't transformed to STRING_CSTs yet,
so without the size of the initializer specified, the function
returns the initializer for the smallest subobject, or char in
this case. I've enhanced the function to handle them.
So at the moment it returns an empty array constructor, correct?
Isn't that the more canonical representation? The STRING_CST
index type doesn't really handle "empty" strings and I expect code
more confused about that than about an empty CTOR?
Yes. Returning an empty CTOR is more general than an empty
string and enables more optimizations. It requires changing
the caller(s) that look for a string but I think that's fine.
Thanks for the hint!
+/* Determine the size of the flexible array FLD from the initializer
+ expression for the struct object DECL in which the meber is declared
+ (possibly recursively). Return the size or zero constant if it isn't
+ initialized. */
+
+static tree
+get_flexarray_size (tree decl, tree fld)
+{
+ if (tree init = DECL_INITIAL (decl))
+{
+ init = get_initializer_for (init, fld);
+ if (init)
+ return TYPE_SIZE_UNIT (TREE_TYPE (init));
+}
+
+ return integer_zero_node;
so you're hoping that the (sub-)CONSTRUCTOR get_initializer_for
returns has a complete type but the initialized object didn't get it
completed. Isnt that wishful thinking?
I don't know what you mean. When might a CONSTRUCTOR not have
a complete type, and if/when it doesn't, why would that be
a problem here? TYPE_SIZE_UNIT will evaluate to null meaning
"don't know" and that's fine. Could you try to be more specific
about the problem you're pointing out?
And why return integer_zero_node
rather than NULL_TREE here?
Because the size of a flexible array member with no initializer
is zero.
+ if (TREE_CODE (dest) == COMPONENT_REF)
+{
+ *pdecl = TREE_OPERAND (dest, 1);
+
+ /* If the member has a size return it. Otherwise it's a flexible
+array member. */
+ if (tree size = DECL_SIZE_UNIT (*pdecl))
+ return size;
because here you do.
Not sure what you mean here either. (This code was also a bit
You return NULL_TREE.
out of date WRT to the patch I had tested. Not sure how that
happened. The attached patch is up to date.)
Also once you have an underlying VAR_DECL you can compute
the flexarray size by DECL_SIZE (var) - offset-of flexarray member.
Isn't that way cheaper than walking the initializer (possibly many
times?)
It would be nice if it were this easy. Is the value of DECL_SIZE
(var) supposed to include the size of the flexible array member?
Yes, DECL_SIZE of the VAR_DECL is the size we use for assembling.
It is usually only the types that remain incomplete (or too small) since
the FE doesn't create many variants of a struct S { int n; char x[]; }
when "instantiating" it via struct S s = { 5, "abcde" }; that then holds
true for the DECL_SIZE of the FIELD_DECL of x as well.
I don't see it mentioned in the comments in tree.h and in my tests
it only does in C but not in C++.
Re: [PATCH] issue consistent warning for past-the-end array stores (PR 91457)
On Tue, Aug 20, 2019 at 4:32 AM Martin Sebor wrote:
>
> On 8/19/19 8:10 AM, Richard Biener wrote:
> > On Sat, Aug 17, 2019 at 12:43 AM Martin Sebor wrote:
> >>
> >> With the recent enhancement to the strlen handling of multibyte
> >> stores the g++.dg/warn/Warray-bounds-4.C for zero-length arrays
> >> started failing on hppa (and probably elsewhere as well). This
> >> is partly the result of the added detection of past-the-end
> >> writes into the strlen pass which detects more instances of
> >> the problem than -Warray-bounds. Since the IL each warning
> >> works with varies between targets, the same invalid code can
> >> be diagnosed by one warning one target and different warning
> >> on another.
> >>
> >> The attached patch does three things:
> >>
> >> 1) It enhances compute_objsize to also determine the size of
> >> a flexible array member (and its various variants), including
> >> from its initializer if necessary. (This resolves 91457 but
> >> introduces another warning where was previously just one.)
> >> 2) It guards the new instance of -Wstringop-overflow with
> >> the no-warning bit on the assignment to avoid warning on code
> >> that's already been diagnosed.
> >> 3) It arranges for -Warray-bounds to set the no-warning bit on
> >> the enclosing expression to keep -Wstringop-overflow from issuing
> >> another warning for the same problem.
> >>
> >> Testing the compute_objsize enhancement to bring it up to par
> >> with -Warray-bounds in turn exposed a weakness in the latter
> >> warning for flexible array members. Rather than snowballing
> >> additional improvements into this one I decided to put that
> >> off until later, so the new -Warray-bounds test has a bunch
> >> of XFAILs. I'll see if I can find the time to deal with those
> >> either still in stage 1 or in stage 3 (one of them is actually
> >> an ancient regression).
> >
> > +static tree
> > +get_initializer_for (tree init, tree decl)
> > +{
> >
> > can't you use fold_ctor_reference here?
>
> Yes, but only with an additional enhancement. Char initializers
> for flexible array members aren't transformed to STRING_CSTs yet,
> so without the size of the initializer specified, the function
> returns the initializer for the smallest subobject, or char in
> this case. I've enhanced the function to handle them.
So at the moment it returns an empty array constructor, correct?
Isn't that the more canonical representation? The STRING_CST
index type doesn't really handle "empty" strings and I expect code
more confused about that than about an empty CTOR?
> >
> > +/* Determine the size of the flexible array FLD from the initializer
> > + expression for the struct object DECL in which the meber is declared
> > + (possibly recursively). Return the size or zero constant if it isn't
> > + initialized. */
> > +
> > +static tree
> > +get_flexarray_size (tree decl, tree fld)
> > +{
> > + if (tree init = DECL_INITIAL (decl))
> > +{
> > + init = get_initializer_for (init, fld);
> > + if (init)
> > + return TYPE_SIZE_UNIT (TREE_TYPE (init));
> > +}
> > +
> > + return integer_zero_node;
> >
> > so you're hoping that the (sub-)CONSTRUCTOR get_initializer_for
> > returns has a complete type but the initialized object didn't get it
> > completed. Isnt that wishful thinking?
>
> I don't know what you mean. When might a CONSTRUCTOR not have
> a complete type, and if/when it doesn't, why would that be
> a problem here? TYPE_SIZE_UNIT will evaluate to null meaning
> "don't know" and that's fine. Could you try to be more specific
> about the problem you're pointing out?
>
> > And why return integer_zero_node
> > rather than NULL_TREE here?
>
> Because the size of a flexible array member with no initializer
> is zero.
>
> >
> > + if (TREE_CODE (dest) == COMPONENT_REF)
> > +{
> > + *pdecl = TREE_OPERAND (dest, 1);
> > +
> > + /* If the member has a size return it. Otherwise it's a flexible
> > +array member. */
> > + if (tree size = DECL_SIZE_UNIT (*pdecl))
> > + return size;
> >
> > because here you do.
>
> Not sure what you mean here either. (This code was also a bit
You return NULL_TREE.
> out of date WRT to the patch I had tested. Not sure how that
> happened. The attached patch is up to date.)
>
> >
> > Also once you have an underlying VAR_DECL you can compute
> > the flexarray size by DECL_SIZE (var) - offset-of flexarray member.
> > Isn't that way cheaper than walking the initializer (possibly many
> > times?)
>
> It would be nice if it were this easy. Is the value of DECL_SIZE
> (var) supposed to include the size of the flexible array member?
Yes, DECL_SIZE of the VAR_DECL is the size we use for assembling.
It is usually only the types that remain incomplete (or too small) since
the FE doesn't create many variants of a struct S { int n; char x[]; }
when "instantiating" it via struct S s = { 5, "abcde" }; that then holds
true for the DECL_SIZE of the FIELD_DECL of x as
Re: [PATCH] issue consistent warning for past-the-end array stores (PR 91457)
On 8/19/19 8:10 AM, Richard Biener wrote:
On Sat, Aug 17, 2019 at 12:43 AM Martin Sebor wrote:
With the recent enhancement to the strlen handling of multibyte
stores the g++.dg/warn/Warray-bounds-4.C for zero-length arrays
started failing on hppa (and probably elsewhere as well). This
is partly the result of the added detection of past-the-end
writes into the strlen pass which detects more instances of
the problem than -Warray-bounds. Since the IL each warning
works with varies between targets, the same invalid code can
be diagnosed by one warning one target and different warning
on another.
The attached patch does three things:
1) It enhances compute_objsize to also determine the size of
a flexible array member (and its various variants), including
from its initializer if necessary. (This resolves 91457 but
introduces another warning where was previously just one.)
2) It guards the new instance of -Wstringop-overflow with
the no-warning bit on the assignment to avoid warning on code
that's already been diagnosed.
3) It arranges for -Warray-bounds to set the no-warning bit on
the enclosing expression to keep -Wstringop-overflow from issuing
another warning for the same problem.
Testing the compute_objsize enhancement to bring it up to par
with -Warray-bounds in turn exposed a weakness in the latter
warning for flexible array members. Rather than snowballing
additional improvements into this one I decided to put that
off until later, so the new -Warray-bounds test has a bunch
of XFAILs. I'll see if I can find the time to deal with those
either still in stage 1 or in stage 3 (one of them is actually
an ancient regression).
+static tree
+get_initializer_for (tree init, tree decl)
+{
can't you use fold_ctor_reference here?
Yes, but only with an additional enhancement. Char initializers
for flexible array members aren't transformed to STRING_CSTs yet,
so without the size of the initializer specified, the function
returns the initializer for the smallest subobject, or char in
this case. I've enhanced the function to handle them.
+/* Determine the size of the flexible array FLD from the initializer
+ expression for the struct object DECL in which the meber is declared
+ (possibly recursively). Return the size or zero constant if it isn't
+ initialized. */
+
+static tree
+get_flexarray_size (tree decl, tree fld)
+{
+ if (tree init = DECL_INITIAL (decl))
+{
+ init = get_initializer_for (init, fld);
+ if (init)
+ return TYPE_SIZE_UNIT (TREE_TYPE (init));
+}
+
+ return integer_zero_node;
so you're hoping that the (sub-)CONSTRUCTOR get_initializer_for
returns has a complete type but the initialized object didn't get it
completed. Isnt that wishful thinking?
I don't know what you mean. When might a CONSTRUCTOR not have
a complete type, and if/when it doesn't, why would that be
a problem here? TYPE_SIZE_UNIT will evaluate to null meaning
"don't know" and that's fine. Could you try to be more specific
about the problem you're pointing out?
And why return integer_zero_node
rather than NULL_TREE here?
Because the size of a flexible array member with no initializer
is zero.
+ if (TREE_CODE (dest) == COMPONENT_REF)
+{
+ *pdecl = TREE_OPERAND (dest, 1);
+
+ /* If the member has a size return it. Otherwise it's a flexible
+array member. */
+ if (tree size = DECL_SIZE_UNIT (*pdecl))
+ return size;
because here you do.
Not sure what you mean here either. (This code was also a bit
out of date WRT to the patch I had tested. Not sure how that
happened. The attached patch is up to date.)
Also once you have an underlying VAR_DECL you can compute
the flexarray size by DECL_SIZE (var) - offset-of flexarray member.
Isn't that way cheaper than walking the initializer (possibly many
times?)
It would be nice if it were this easy. Is the value of DECL_SIZE
(var) supposed to include the size of the flexible array member?
I don't see it mentioned in the comments in tree.h and in my tests
it only does in C but not in C++. Is that a bug that in C++ it
doesn't?
Attached is an updated patch that uses fold_ctor_reference as you
suggested. I've also made a few other minor changes to diagnose
a few more invalid strlen calls with out-of-bounds offsets.
(More still remain.)
Martin
Richard.
Martin
PS I imagine the new get_flexarray_size function will probably
need to move somewhere more appropriate so that other warnings
(like -Warray-bounds to remove the XFAILs) and optimizations
can make use of it.
PR tree-optimization/91457 - inconsistent warning for writing past the end of an array member
gcc/testsuite/ChangeLog:
PR tree-optimization/91457
* c-c++-common/Wstringop-overflow-2.c: New test.
* g++.dg/warn/Warray-bounds-8.C: New test.
* g++.dg/warn/Wstringop-overflow-3.C: New test.
* gcc.dg/Wstringop-overflow-15.c: New test.
gcc/ChangeLog:
PR tree-optimization/91457
* builtins.c (c_strlen): Rename argument and
Re: [PATCH] issue consistent warning for past-the-end array stores (PR 91457)
On Sat, Aug 17, 2019 at 12:43 AM Martin Sebor wrote:
>
> With the recent enhancement to the strlen handling of multibyte
> stores the g++.dg/warn/Warray-bounds-4.C for zero-length arrays
> started failing on hppa (and probably elsewhere as well). This
> is partly the result of the added detection of past-the-end
> writes into the strlen pass which detects more instances of
> the problem than -Warray-bounds. Since the IL each warning
> works with varies between targets, the same invalid code can
> be diagnosed by one warning one target and different warning
> on another.
>
> The attached patch does three things:
>
> 1) It enhances compute_objsize to also determine the size of
> a flexible array member (and its various variants), including
> from its initializer if necessary. (This resolves 91457 but
> introduces another warning where was previously just one.)
> 2) It guards the new instance of -Wstringop-overflow with
> the no-warning bit on the assignment to avoid warning on code
> that's already been diagnosed.
> 3) It arranges for -Warray-bounds to set the no-warning bit on
> the enclosing expression to keep -Wstringop-overflow from issuing
> another warning for the same problem.
>
> Testing the compute_objsize enhancement to bring it up to par
> with -Warray-bounds in turn exposed a weakness in the latter
> warning for flexible array members. Rather than snowballing
> additional improvements into this one I decided to put that
> off until later, so the new -Warray-bounds test has a bunch
> of XFAILs. I'll see if I can find the time to deal with those
> either still in stage 1 or in stage 3 (one of them is actually
> an ancient regression).
+static tree
+get_initializer_for (tree init, tree decl)
+{
can't you use fold_ctor_reference here?
+/* Determine the size of the flexible array FLD from the initializer
+ expression for the struct object DECL in which the meber is declared
+ (possibly recursively). Return the size or zero constant if it isn't
+ initialized. */
+
+static tree
+get_flexarray_size (tree decl, tree fld)
+{
+ if (tree init = DECL_INITIAL (decl))
+{
+ init = get_initializer_for (init, fld);
+ if (init)
+ return TYPE_SIZE_UNIT (TREE_TYPE (init));
+}
+
+ return integer_zero_node;
so you're hoping that the (sub-)CONSTRUCTOR get_initializer_for
returns has a complete type but the initialized object didn't get it
completed. Isnt that wishful thinking? And why return integer_zero_node
rather than NULL_TREE here?
+ if (TREE_CODE (dest) == COMPONENT_REF)
+{
+ *pdecl = TREE_OPERAND (dest, 1);
+
+ /* If the member has a size return it. Otherwise it's a flexible
+array member. */
+ if (tree size = DECL_SIZE_UNIT (*pdecl))
+ return size;
because here you do.
Also once you have an underlying VAR_DECL you can compute
the flexarray size by DECL_SIZE (var) - offset-of flexarray member.
Isn't that way cheaper than walking the initializer (possibly many
times?)
Richard.
>
> Martin
>
> PS I imagine the new get_flexarray_size function will probably
> need to move somewhere more appropriate so that other warnings
> (like -Warray-bounds to remove the XFAILs) and optimizations
> can make use of it.
[PATCH] issue consistent warning for past-the-end array stores (PR 91457)
With the recent enhancement to the strlen handling of multibyte
stores the g++.dg/warn/Warray-bounds-4.C for zero-length arrays
started failing on hppa (and probably elsewhere as well). This
is partly the result of the added detection of past-the-end
writes into the strlen pass which detects more instances of
the problem than -Warray-bounds. Since the IL each warning
works with varies between targets, the same invalid code can
be diagnosed by one warning one target and different warning
on another.
The attached patch does three things:
1) It enhances compute_objsize to also determine the size of
a flexible array member (and its various variants), including
from its initializer if necessary. (This resolves 91457 but
introduces another warning where was previously just one.)
2) It guards the new instance of -Wstringop-overflow with
the no-warning bit on the assignment to avoid warning on code
that's already been diagnosed.
3) It arranges for -Warray-bounds to set the no-warning bit on
the enclosing expression to keep -Wstringop-overflow from issuing
another warning for the same problem.
Testing the compute_objsize enhancement to bring it up to par
with -Warray-bounds in turn exposed a weakness in the latter
warning for flexible array members. Rather than snowballing
additional improvements into this one I decided to put that
off until later, so the new -Warray-bounds test has a bunch
of XFAILs. I'll see if I can find the time to deal with those
either still in stage 1 or in stage 3 (one of them is actually
an ancient regression).
Martin
PS I imagine the new get_flexarray_size function will probably
need to move somewhere more appropriate so that other warnings
(like -Warray-bounds to remove the XFAILs) and optimizations
can make use of it.
PR tree-optimization/91458 - inconsistent warning for writing past the end of an array member
gcc/testsuite/ChangeLog:
PR tree-optimization/91458
* g++.dg/warn/Warray-bounds-8.C: New test.
* g++.dg/warn/Wstringop-overflow-3.C: New test.
gcc/ChangeLog:
PR tree-optimization/91458
* builtins.c (get_initializer_for, get_flexarray_size): New functions.
(compute_objsize): Add argument. Handle ARRAY_REF and COMPONENT_REF.
* builtins.h ((compute_objsize): Add argument.
* tree-ssa-strlen.c (handle_store): Handle no-warning bit.
* tree-vrp.c (vrp_prop::check_array_ref): Return warning result.
(vrp_prop::check_mem_ref): Same.
(vrp_prop::search_for_addr_array): Set no-warning bit.
(check_array_bounds): Same.
Index: gcc/builtins.c
===
--- gcc/builtins.c (revision 274541)
+++ gcc/builtins.c (working copy)
@@ -3560,6 +3560,51 @@ check_access (tree exp, tree, tree, tree dstwrite,
return true;
}
+/* Given the initializer INIT, return the initializer for the field
+ DECL if it exists, otherwise null. Used to obtain the initializer
+ for a flexible array member and determine its size. */
+
+static tree
+get_initializer_for (tree init, tree decl)
+{
+ STRIP_NOPS (init);
+
+ tree fld, fld_init;
+ unsigned HOST_WIDE_INT i;
+ FOR_EACH_CONSTRUCTOR_ELT (CONSTRUCTOR_ELTS (init), i, fld, fld_init)
+{
+ if (decl == fld)
+ return fld_init;
+
+ if (TREE_CODE (fld) == CONSTRUCTOR)
+ {
+ fld_init = get_initializer_for (fld_init, decl);
+ if (fld_init)
+ return fld_init;
+ }
+}
+
+ return NULL_TREE;
+}
+
+/* Determine the size of the flexible array FLD from the initializer
+ expression for the struct object DECL in which the meber is declared
+ (possibly recursively). Return the size or zero constant if it isn't
+ initialized. */
+
+static tree
+get_flexarray_size (tree decl, tree fld)
+{
+ if (tree init = DECL_INITIAL (decl))
+{
+ init = get_initializer_for (init, fld);
+ if (init)
+ return TYPE_SIZE_UNIT (TREE_TYPE (init));
+}
+
+ return integer_zero_node;
+}
+
/* Helper to compute the size of the object referenced by the DEST
expression which must have pointer type, using Object Size type
OSTYPE (only the least significant 2 bits are used). Return
@@ -3567,12 +3612,18 @@ check_access (tree exp, tree, tree, tree dstwrite,
the size cannot be determined. When the referenced object involves
a non-constant offset in some range the returned value represents
the largest size given the smallest non-negative offset in the
- range. The function is intended for diagnostics and should not
- be used to influence code generation or optimization. */
+ range. If nonnull, set *PDECL to the decl of the referenced
+ subobject if it can be determined, or to null otherwise.
+ The function is intended for diagnostics and should not be used
+ to influence code generation or optimization. */
tree
-compute_objsize (tree dest, int ostype)
+compute_objsize (tree dest, int ostype, tree *pdecl /* = NULL */)
{
+ tree dummy = NULL_TREE;
+ if (!pdecl)
+pdecl =
+
unsigned HOST_WIDE_INT size;
/* Only the two least
