On 1/4/21 2:10 PM, Jeff Law wrote:
On 1/4/21 1:53 PM, Martin Sebor wrote:
On 1/4/21 12:23 PM, Jeff Law wrote:
On 1/4/21 12:19 PM, Jakub Jelinek wrote:
On Mon, Jan 04, 2021 at 12:14:15PM -0700, Jeff Law via Gcc-patches
wrote:
Doing the STRING_CST is certainly less fragile since the SSA names
created at gimplification time could even be ggc_freed when no longer
used in the IL.
Obviously we can't use SSA_NAMEs as they're specific to each
function as
they get compiled. But what's not as clear to me is why we can't
use a
SAVE_EXPR of the original expression that indicates the size of the
parameter.
The gimplifier is destructive, so if the expressions are partly
(e.g. in
those SAVE_EXPRs) shared with what is in the actual IL, we lose.
And if they aren't shared and there are side-effects, if we tried to
gimplify them again we'd get the side-effects duplicated.
So it all depends on what the code wants to handle, if e.g. just
values of
parameters with simple arithmetics on those and punt on everything
else,
then it is doable, but generally it is not.
I explained what the code handles and when in the pipeline in
the discussion of the previous patch:
https://gcc.gnu.org/pipermail/gcc-patches/2020-November/559770.html
Right, but that message talks about GC. This is not a GC issue.
This feels like we need a SAVE_EXPR to me to ensure single evaluation
and an unshare_expr to avoid problems with destructive gimplification.
I would expect the expressions to be values of parameters (or objects in
static storage) and simple arithemetic on them. If there's other cases,
punting seems appropriate.
Martin -- are there nontrivial expressions we need to be worried
about here?
At the moment the middle warnings only consider parameters, like
the N in
void f (int N, int[N]);
void g (void)
{
int a[3];
f (sizeof a, a); // warning
}
The front end redeclaration warnings consider all expressions,
including
int f (void);
void g (int[f () + 1]);
void g (int[f () + 2]); // warning
The patch turns these complex bounds into strings that the front
end compares instead.
If you can have an arbitrary expression, such as a function call like
that, then ISTM that a SAVE_EPR is mandatory as you can't call the
function more than once. BUt it also seems to me that for cases that
aren't simple arithmetic of leaf nodes we could just punt. I doubt
we're going to miss significant real world diagnostics by doing that.
I don't know about that. Bugs are rare and often in unusual and
hard to read/understand code, so focusing on the simple cases and
doing nothing for the rest would certainly not be an improvement.
My understanding from the discussion at the link above is that
using SAVE_EXPRs is only necessary when the expression is evaluated
(the warning doesn't evaluate them). I didn't use the SAVE_EXPRs
in this patch even though they're readily available (I had initially
missed it) or unsharing because Jakub preferred to avoid the space
overhead (IIUC). I didn't remove the expressions because (as
I explained) the only way I could find to do it was one that Richard
was quite emphatic should be avoided.
If you want me to use the SHARE_EXPRs I can easily make that change
(if that counts for anything that would be my preference). If there's
preference for removing the saved bounds in free_lang_data pass I can
presumably do that, either in addition or instead.
After the front end is done the strings
don't serve any purpose (and I don't think ever will) and could
be removed. I looked for a way to do it but couldn't find one
other than the free_lang_data pass in tree.c that Richard had
initially said wasn't the right place. Sounds like he's
reconsidered but at this point, given that VLA parameters are
used only infraquently, and VLAs with these nontrivial bounds
are exceedingly rare, going to the trouble of removing them
doesn't seem worth the effort.
But I'm not sure that inventing a new method for smuggling the data
around is all that wise or necessary here. I don't see a message from
anyone suggesting that, but I could have missed it.
No one suggested "smuggling" anything around. It also wasn't
my intent, nor do I think the code code that. It just stores
the bounds in a form that the middle end can cope with. There
are other front-end-only attributes that store strings (e.g.,
attribute deprecated) so this is not new. But as I said, I'm
open to removing either the strings or the expressions. I'd
just like to know which before I do the work this time.
Martin
