On Tue, Aug 20, 2019 at 4:32 AM Martin Sebor <msebor@gmail.com> wrote:
On 8/19/19 8:10 AM, Richard Biener wrote:
On Sat, Aug 17, 2019 at 12:43 AM Martin Sebor <msebor@gmail.com> 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 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++. Is that a bug that in C++ it
doesn't?
tree.h dcoumentation isn't complete. And for me the C++ FE for
the above simple example has
<var_decl 0x7ffff7fedb40 s
type <record_type 0x7ffff6d94f18 S cxx-odr-p type_5 type_6 BLK
size <integer_cst 0x7ffff6c5e0a8 constant 32>
unit-size <integer_cst 0x7ffff6c5e0c0 constant 4>
align:32 warn_if_not_align:0 symtab:0 alias-set -1
canonical-type 0x7ffff6d94f18
fields <function_decl 0x7ffff6d98b00 __dt type <method_type
0x7ffff6da8150>
public abstract external autoinline decl_3 QI t.ii:1:8
align:16 warn_if_not_align:0 context <record_type 0x7ffff6d94f18 S>
full-name "S::~S() noexcept (<uninstantiated>)"
not-really-extern chain <function_decl 0x7ffff6d98d00
__dt_base >> context <translation_unit_decl 0x7ffff6c4a168 t.ii>
full-name "struct S"
X() X(constX&) this=(X&) n_parents=0 use_template=0 interface-unknown
pointer_to_this <pointer_type 0x7ffff6da85e8>
reference_to_this <reference_type 0x7ffff6da8a80> chain <type_decl
0x7ffff6c6b850 S>>
public static tree_1 tree_2 tree_3 BLK t.ii:3:3 size <integer_cst
0x7ffff6c5e0a8 32> unit-size <integer_cst 0x7ffff6c5e0c0 4>
align:32 warn_if_not_align:0 context <translation_unit_decl
0x7ffff6c4a168 t.ii> initial <constructor 0x7ffff6da52a0> chain
<type_decl 0x7ffff6c6b850 S>>
and we assemble it like
.globl s
.data
.align 4
.type s, @object
.size s, 4
s:
.long 5
.string "abcde"
note how we have .size s, 4 here...
of course in the end nobody cares about a symbols size ...(?)
I expected layout_decl to fixup DECL_SIZE according to the iniitalizer
but it looks like the initializer isn't present when it is layouted.
I think this is a middle-end flaw we should try to fix that either in
the FEs or in the middle-end. The C FE calls add_flexible_array_elts_to_size
and complete_flexible_array_elts in its finish_decl routine (layouting happens
at build_decl time, too early, but if we want to move this functionality then
re-layouting might be posible). I'm not sure about other FEs but I guess
the easiest point we could do sanity checking is when assemble_variable
outputs the constructor.