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
