On 12/21/18 2:03 AM, Martin Sebor wrote:
On 12/20/18 2:07 PM, Bernd Edlinger wrote:
On 12/20/18 6:50 PM, Martin Sebor wrote:
On 12/20/18 10:46 AM, Martin Sebor wrote:
On 12/17/18 7:58 AM, Jason Merrill wrote:
On 12/15/18 3:36 AM, Bernd Edlinger wrote:
this patch implements an error message, for non-static initialization of a
flexible array member.
This duplicates the existing error message from the C-FE, to avoid ICE and
wrong code generation
issues, as pointed out in the PR.
It is a bit funny that a non-functional feature like that has already rather
much test coverage.
The most easy adjustment seems to change the existing test cases to use static
declarations.
Martin, thoughts?
Our high-level goal when tightening up how flexible array members
are handled in C++ was to accept what's accepted in standard C mode
and reject (or, at a minimum, warn for) C++ extensions that could
be relied on in existing code.
I meant "reject what couldn't be relied on" and "warn for that could
be."
I believe the problem here is effectively that initializing non-static
flexible array is not supported by the middle-end. All examples
where flexible array members are initialized on automatic variable
work only as long as they are simple enough that they are optimized
away so that they do not survive until expansion.
Take as example gcc/testsuite/g++.dg/ext/flexary13.C,
it compiles and runs successfully, but the assertions start to
fail if Ax is declared volatile, and at the same time, we know
that the automatic variables are allocated in a way that they
can overlap and crash at any time.
My impression is that the existing C error made the middle-end kind of rely
on this behavior.
So I think the right thing to do is duplicate the existing C error in
the C++ FE. I do not see any automatic variable with initialized flexible
data members where it would be safe to only warn about them.
If there are no reasonable use cases that code out there could
be relying on because none of them works correctly then rejecting
the initialization makes sense to me.
(Sorry for the delay, by the way. I've been migrating to a new machine
this week and things aren't yet working quite like I'm used to.)
The flexarray tests I added back then were for features that looked
like intentional extensions and that seemed to work for at least
some use cases as far as I could tell. What I noticed didn't work
I created bugs for: 69338, 69696, and 69338 look related, but there
are others.
I think all these bugs should all be reviewed and a decision made
about what's intended to work and what continues to be accepted as
an accident and should be rejected. After that, we can adjust
the existing tests.
I would not rule out the possibility that there can be more bugs.
But I think the existing tests need to avoid the case which evokes
the new error. The question is, if changing from automatic to static
objects prevents those tests to test what they were originally written for.
I believe this is not the case, but I do probably not know all the
background here.
IIRC, most of the tests I added were meant to exercise just
the front-end, not any later stages (if that's what you meant).
Otherwise, if you're worried about the changes from auto to
static no longer exercising downstream front-end code, whether
that matters depends on the intent of each test.
flexary13.C was most likely meant to also verify codegen (hence
the assertions) so I would suggest to make it do that (i.e.,
verify the assertions are optimized out if in fact they are,
or make the test run so they must pass).
Oh well, unfortunately the modified test case with static objects
fails one assertion when executed at -O0, I missed that before,
because I used -O2 or higher. I filed that as PR 88578, so in the
moment I would like to leave the test case as compile only,
and change that to run once PR 88578 is resolved.
The changes to the rest of the flexary*.C tests seem okay,
though a new test should be added to explicitly exercise this
change (bug 88261), even if the error happens to be tested by
one of the changed tests.
That is the case, because the array-6.c test case was moved
to c-c++-common. That is the reproducer for the ICE from the PR.
In changes to the Wplacement-new-size*.C tests I would suggest
to follow the same approach of using statics instead of testing
for errors so the code that exercises warnings doesn't depend
on erroneous constructs.
The comment in Wplacement-new-size-2.C just above the code your
patch changes that reads:
// Initialization of non-static objects with flexible array members
// isn't allowed in C and should perhaps be disallowed in C++ as
// well to avoid c++/69696 - incorrect initialization of block-scope
// flexible array members.
Ax ax2 = { 1, { 2, 3 } };
should be updated and the referenced bug and any others that this
change prevents should be resolved.
Done.
I also added PR c++/69696 to the changelog as this should definitely
be fixed by this patch as well.
So despite the newly discovered problem with the non-constant
initializers which appears to be a separate problem, I would still like
to get an OK for this patch in the current form.
