Re: [PATCH] store VLA bounds in attribute access as strings (PR 97172)
On 1/4/21 4:54 PM, Martin Sebor wrote:
> 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.
I would disagree. It's an improvement for what is most likely the most
common case. VLAs aren't used that heavily to begin with and VLAs with
bounds that require function calls to evaluate would seem to be quite rare.
>
> 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).
Hmm, so this goes back to Richi's comment/question. If we're not
evaluating the expression, then we're just doing a lexicographical
comparison? And yes, in that case we wouldn't need the SAVE_EXPR.
>>> 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.
You're reading way too much into the word "smuggle".
jeff
Re: [PATCH] store VLA bounds in attribute access as strings (PR 97172)
On 1/4/21 2:20 PM, Jakub Jelinek wrote: > On Mon, Jan 04, 2021 at 02:10:39PM -0700, Jeff Law wrote: >>> 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. > unshare_expr will not duplicate SAVE_EXPRs. > So, one would need to unshare with special handling of SAVE_EXPRs that would > throw them away (for the simple arguments case) rather than handling them > normally. My mental model of how this works must be broken then. I thought we would need to unshare the expression, then wrap it in the SAVE_EXPR. It seems like you're saying that we've already got the SAVE_EXPR and that unshare_expr won't traverse into it. That would indeed be problematical. jeff
Re: [PATCH] store VLA bounds in attribute access as strings (PR 97172)
On 1/5/21 5:38 AM, Richard Biener wrote:
On Mon, Jan 4, 2021 at 9: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
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
I wonder how this can work reliably without heavy-weight
"parsing" of the attribute? That is, how do you relate
the passed 24 constant to the N in int[N]?
There is some parsing involved but it's only slightly more complex
than in attribute fn spec. Just a string scan followed by constant
time lookup for each pointer argument.
The N is associated with int[N] via N's position in the argument
list and encoded as $N in the string. The attribute for the decl
above is "1[$],$0". The 1 is the VLA argument position, each
dollar sign in the brackets is one VLA bound (numbers are constant
bounds), and the $0 is the VLA bound argument.
(There is some redundancy here since all but the most significant
array (or VLA) bound are also encoded in the type of the argument.)
The front end redeclaration warnings consider all expressions,
including
int f (void);
void g (int[f () + 1]);
void g (int[f () + 2]); // warning
For redeclaration warning the attribute isn't needed since you
have both decls and can compare sizes directly?
The attribute is used here as well. It's attached to the first
decl irrespective of the form of the VLA, then created for
the second decl and the two are compared. Mismatches are then
diagnosed and dropped from the second attribute. The result
is merged with the first and added to the decl.
Martin
The patch turns these complex bounds into strings that the front
end compares 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.
Martin
Jeff
Re: [PATCH] store VLA bounds in attribute access as strings (PR 97172)
On Mon, Jan 4, 2021 at 9: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
>
> > 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
I wonder how this can work reliably without heavy-weight
"parsing" of the attribute? That is, how do you relate
the passed 24 constant to the N in int[N]?
>}
>
> The front end redeclaration warnings consider all expressions,
> including
>
>int f (void);
>
>void g (int[f () + 1]);
>void g (int[f () + 2]); // warning
For redeclaration warning the attribute isn't needed since you
have both decls and can compare sizes directly?
> The patch turns these complex bounds into strings that the front
> end compares 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.
>
> Martin
>
> >
> >
> > Jeff
> >
>
Re: [PATCH] store VLA bounds in attribute access as strings (PR 97172)
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
Re: [PATCH] store VLA bounds in attribute access as strings (PR 97172)
On Mon, Jan 04, 2021 at 02:10:39PM -0700, Jeff Law wrote: > > 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. unshare_expr will not duplicate SAVE_EXPRs. So, one would need to unshare with special handling of SAVE_EXPRs that would throw them away (for the simple arguments case) rather than handling them normally. Jakub
Re: [PATCH] store VLA bounds in attribute access as strings (PR 97172)
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.
> 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.
jeff
Re: [PATCH] store VLA bounds in attribute access as strings (PR 97172)
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
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. 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.
Martin
Jeff
Re: [PATCH] store VLA bounds in attribute access as strings (PR 97172)
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 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? Jeff
Re: [PATCH] store VLA bounds in attribute access as strings (PR 97172)
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. Jakub
Re: [PATCH] store VLA bounds in attribute access as strings (PR 97172)
On 1/4/21 5:59 AM, Richard Biener via Gcc-patches wrote: > On Sun, Dec 20, 2020 at 6:43 PM Martin Sebor wrote: >> On 12/19/20 12:48 AM, Richard Biener via Gcc-patches wrote: >>> On December 19, 2020 1:55:02 AM GMT+01:00, Martin Sebor via Gcc-patches >>> wrote: To keep tree expressions stored by the front end in attribute access for nontrivial VLA bounds from getting corrupted during Gimplification and to avoid breaking the preconditions verified by the LTO streamer that no such trees exist in the IL, the attached patch replaces those bounds with a string representation of those expressions (as STRING_CST). It also tweaks the pretty-printer to improve the formatting of the VLA bounds and avoid inserting spurious spaces in some cases. The strings are only used by the front end to verify that redeclarations of the same function match in the form and bounds of their VLA arguments, and they're not needed in the middle end. I considered removing them just before the front end finishes but I couldn't find an efficient way to do that. Is there some data structure that stores all function declarations in a translation unit? If there is, then traversing it and removing the attribute arguments might also be an option, either in addition to this change or in lieu of it. >>> There is the free lang data pass in tree.c which walks all reachable tree >>> nodes. >> You said in response to Honza's suggestion in pr97172 to do that: >> >>The frontend needs to make sure no frontend specific tree codes >>leak into GENERIC so GENERICization is the place where the FE >>should clear those. FLD is too late and doing it there would >>be a hack. >> >> Are you now saying you've had a change of heart and that doing it >> there isn't a hack after all? > Well, removing a FE specific attribute [when having non-constant arg] might > be OK there. But note you asked for a "pass over all tree nodes" thing > and I just pointed out free-lang-data. > > 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. Jeff
Re: [PATCH] store VLA bounds in attribute access as strings (PR 97172)
On Sun, Dec 20, 2020 at 6:43 PM Martin Sebor wrote: > > On 12/19/20 12:48 AM, Richard Biener via Gcc-patches wrote: > > On December 19, 2020 1:55:02 AM GMT+01:00, Martin Sebor via Gcc-patches > > wrote: > >> To keep tree expressions stored by the front end in attribute > >> access for nontrivial VLA bounds from getting corrupted during > >> Gimplification and to avoid breaking the preconditions verified > >> by the LTO streamer that no such trees exist in the IL, > >> the attached patch replaces those bounds with a string > >> representation of those expressions (as STRING_CST). It also > >> tweaks the pretty-printer to improve the formatting of the VLA > >> bounds and avoid inserting spurious spaces in some cases. > >> > >> The strings are only used by the front end to verify that > >> redeclarations of the same function match in the form and bounds > >> of their VLA arguments, and they're not needed in the middle end. > >> I considered removing them just before the front end finishes but > >> I couldn't find an efficient way to do that. Is there some data > >> structure that stores all function declarations in a translation > >> unit? If there is, then traversing it and removing the attribute > >> arguments might also be an option, either in addition to this > >> change or in lieu of it. > > > > There is the free lang data pass in tree.c which walks all reachable tree > > nodes. > > You said in response to Honza's suggestion in pr97172 to do that: > >The frontend needs to make sure no frontend specific tree codes >leak into GENERIC so GENERICization is the place where the FE >should clear those. FLD is too late and doing it there would >be a hack. > > Are you now saying you've had a change of heart and that doing it > there isn't a hack after all? Well, removing a FE specific attribute [when having non-constant arg] might be OK there. But note you asked for a "pass over all tree nodes" thing and I just pointed out free-lang-data. 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. Richard. > Martin > > >> The patch was tested on x86_64-linux. > >> > >> Martin > > >
Re: [PATCH] store VLA bounds in attribute access as strings (PR 97172)
On 12/19/20 12:48 AM, Richard Biener via Gcc-patches wrote: On December 19, 2020 1:55:02 AM GMT+01:00, Martin Sebor via Gcc-patches wrote: To keep tree expressions stored by the front end in attribute access for nontrivial VLA bounds from getting corrupted during Gimplification and to avoid breaking the preconditions verified by the LTO streamer that no such trees exist in the IL, the attached patch replaces those bounds with a string representation of those expressions (as STRING_CST). It also tweaks the pretty-printer to improve the formatting of the VLA bounds and avoid inserting spurious spaces in some cases. The strings are only used by the front end to verify that redeclarations of the same function match in the form and bounds of their VLA arguments, and they're not needed in the middle end. I considered removing them just before the front end finishes but I couldn't find an efficient way to do that. Is there some data structure that stores all function declarations in a translation unit? If there is, then traversing it and removing the attribute arguments might also be an option, either in addition to this change or in lieu of it. There is the free lang data pass in tree.c which walks all reachable tree nodes. You said in response to Honza's suggestion in pr97172 to do that: The frontend needs to make sure no frontend specific tree codes leak into GENERIC so GENERICization is the place where the FE should clear those. FLD is too late and doing it there would be a hack. Are you now saying you've had a change of heart and that doing it there isn't a hack after all? Martin The patch was tested on x86_64-linux. Martin
Re: [PATCH] store VLA bounds in attribute access as strings (PR 97172)
On December 19, 2020 1:55:02 AM GMT+01:00, Martin Sebor via Gcc-patches wrote: >To keep tree expressions stored by the front end in attribute >access for nontrivial VLA bounds from getting corrupted during >Gimplification and to avoid breaking the preconditions verified >by the LTO streamer that no such trees exist in the IL, >the attached patch replaces those bounds with a string >representation of those expressions (as STRING_CST). It also >tweaks the pretty-printer to improve the formatting of the VLA >bounds and avoid inserting spurious spaces in some cases. > >The strings are only used by the front end to verify that >redeclarations of the same function match in the form and bounds >of their VLA arguments, and they're not needed in the middle end. >I considered removing them just before the front end finishes but >I couldn't find an efficient way to do that. Is there some data >structure that stores all function declarations in a translation >unit? If there is, then traversing it and removing the attribute >arguments might also be an option, either in addition to this >change or in lieu of it. There is the free lang data pass in tree.c which walks all reachable tree nodes. >The patch was tested on x86_64-linux. > >Martin
[PATCH] store VLA bounds in attribute access as strings (PR 97172)
To keep tree expressions stored by the front end in attribute
access for nontrivial VLA bounds from getting corrupted during
Gimplification and to avoid breaking the preconditions verified
by the LTO streamer that no such trees exist in the IL,
the attached patch replaces those bounds with a string
representation of those expressions (as STRING_CST). It also
tweaks the pretty-printer to improve the formatting of the VLA
bounds and avoid inserting spurious spaces in some cases.
The strings are only used by the front end to verify that
redeclarations of the same function match in the form and bounds
of their VLA arguments, and they're not needed in the middle end.
I considered removing them just before the front end finishes but
I couldn't find an efficient way to do that. Is there some data
structure that stores all function declarations in a translation
unit? If there is, then traversing it and removing the attribute
arguments might also be an option, either in addition to this
change or in lieu of it.
The patch was tested on x86_64-linux.
Martin
Store nontrivial VLA bounds as strings (PR c/97172).
gcc/ChangeLog
PR c/97172
* attribs.c (attr_access::array_as_string): Handle VLA bounds
represented as strings.
* pretty-print.c (pp_string): Add argument and use it.
* pretty-print.h (pp_string): Add default argument.
* tree-pretty-print.c (print_generic_expr_to_string_cst): Define.
(dump_generic_node): Avoid printing spurious space; parenthesize
operands to avoid three consecutive minus signs.
(op_symbol_code):
* tree-pretty-print.h (print_generic_expr_to_string_cst): New.
* tree.c (array_bound_from_maxval): Define.
* tree.h (array_bound_from_maxval): New.
gcc/c-family/ChangeLog:
PR c/97172
* c-attribs.c (handle_argspec_attribute): Handle VLA bounds
represented as strings.
(build_attr_access_from_parms): Adjust comment.
* c-pretty-print.c (c_pretty_printer::direct_abstract_declarator):
Call array_bound_from_maxval.
* c-warn.c (warn_parm_array_mismatch): Handle VLA bounds represented
as strings.
gcc/c/ChangeLog:
PR c/97172
* c-decl.c (get_parm_array_spec): Store nontrivial VLA bounds as
strings.
gcc/testsuite/ChangeLog:
PR c/97172
* gcc.dg/Wvla-parameter-2.c: Adjust text of expected warning.
* gcc.dg/Wvla-parameter-9.c: New test.
diff --git a/gcc/attribs.c b/gcc/attribs.c
index a6f6b70e39e..8803e0e7260 100644
--- a/gcc/attribs.c
+++ b/gcc/attribs.c
@@ -2257,6 +2257,9 @@ attr_access::array_as_string (tree type) const
if (type == error_mark_node)
return std::string ();
+ /* The most significant bound of a VLA as a PARM_DECL or VAR_DECL,
+ or STRING_CST for expressions, or null. */
+ tree bound = NULL_TREE;
if (this->str)
{
/* For array parameters (but not pointers) create a temporary array
@@ -2275,15 +2278,20 @@ attr_access::array_as_string (tree type) const
const char *p = end;
for ( ; p != str && *p-- != ']'; );
if (*p == '$')
- index_type = build_index_type (TREE_VALUE (size));
+ {
+ bound = TREE_VALUE (size);
+ if (DECL_P (bound))
+ index_type = build_index_type (bound);
+ }
}
else if (minsize)
index_type = build_index_type (size_int (minsize - 1));
tree arat = NULL_TREE;
- if (static_p || vla_p)
+ if ((static_p || vla_p))
{
- tree flag = static_p ? integer_one_node : NULL_TREE;
+ tree flag = (static_p && (!bound || DECL_P (bound))
+ ? integer_one_node : NULL_TREE);
/* Hack: there's no language-independent way to encode
the "static" specifier or the "*" notation in an array type.
Add a "fake" attribute to have the pretty-printer add "static"
@@ -2307,6 +2315,21 @@ attr_access::array_as_string (tree type) const
typstr = pp_formatted_text (pp);
delete pp;
+ if (bound && TREE_CODE (bound) == STRING_CST)
+{
+ /* For the most significant bound that's not a DECL and that's
+ stored as a STRING_CST, replace the asterisk in the first [*]
+ it was formatted as, or the first "static", with its human-
+ readable representation. */
+ size_t pos = typstr.find ("[*]", 0);
+ if (static_p)
+ {
+ typstr.replace (pos + 1, 1, "static ");
+ pos += 7;
+ }
+ typstr.replace (pos + 1, 1, TREE_STRING_POINTER (bound));
+}
+
return typstr;
}
diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index 29e26728300..40d6389102f 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -3539,7 +3539,7 @@ handle_argspec_attribute (tree *, tree, tree args, int, bool *)
for (tree next = TREE_CHAIN (args); next; next = TREE_CHAIN (next))
{
tree val = TREE_VALUE (next);
- gcc_assert (DECL_P (val) || EXPR_P (val));
+ gcc_assert (DECL_P (val) || TREE_CODE (val) == STRING_CST);
}
return NULL_TREE;
}
@@ -4910,7 +4910,7 @@ tree
build_attr_access_from_parms (tree parms, bool skip_voidptr)
{
/* Maps each named integral argument DECL seen so far to its position
-
