Interesting (?) failure case

Interesting (?) failure case

[Linus Torvalds]

Try linearizing this with 'sparse', and see it fail miserably:

   int t(void)
   {
        goto inside;
        return 0 ?
                 ({ inside: return 3; 1; })
                :
                 2;
   }

I came up with that disgusting example after talking to Nick
Desaulniers about how sparse does some front-end optimizations early,
and it made me go "Hmm... What about.."

There are two reasonable approaches for the above:

 - return 3 (due to the "goto inside")

 - tell the user to pound sand for doing crazy things and jumping into
a statement expression from outside.

clang does #1. gcc does #2.

sparse does something bad, and just generates garbage silently.

             Linus

Re: Interesting (?) failure case

[Luc Van Oostenryck]

On Wed, Apr 08, 2020 at 09:02:59PM -0700, Linus Torvalds wrote:
> Try linearizing this with 'sparse', and see it fail miserably:
> 
>    int t(void)
>    {
>         goto inside;
>         return 0 ?
>                  ({ inside: return 3; 1; })
>                 :
>                  2;
>    }
> 
> I came up with that disgusting example after talking to Nick
> Desaulniers about how sparse does some front-end optimizations early,
> and it made me go "Hmm... What about.."

Funny, I worked on something very similar last week:
	void f(int x, int y)
	{
		1 ? x : ({
	a:
			 y;
		});
		goto a;
	}

> There are two reasonable approaches for the above:
> 
>  - return 3 (due to the "goto inside")
> 
>  - tell the user to pound sand for doing crazy things and jumping into
> a statement expression from outside.
> 
> clang does #1. gcc does #2.
> 
> sparse does something bad, and just generates garbage silently.

Yes, the problem is caused at expand_conditional() where one of
the sides is throwed away if the condition is known. So the label
doesn't exist anymore and at linearization Sparse ends with a
jump to an unexisting BB.

I tried to simply discard the early optimization in expand but
then when testing the kernel I got a whole bunch of warnings
(bad type or dereference of noderef type, I don't remember).
So it seems that in general (when nobody jump into the expression
statement) the conditional needs to be simplified before evaluation.

I tried also to warn on gotos jumping into an expression statement.
The idea was to give a new 'label_scope' for each such statement.
Things are a bit complicated because the labels are implicitly
declared by the gotos.

I'll need to look a bit more at this.

-- Luc

Re: Interesting (?) failure case

[Linus Torvalds]

On Wed, Apr 8, 2020 at 11:23 PM Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
>
> Yes, the problem is caused at expand_conditional() where one of
> the sides is throwed away if the condition is known. So the label
> doesn't exist anymore and at linearization Sparse ends with a
> jump to an unexisting BB.

Yes.

And I don't think that's really a problem per se. I think the gcc
model of saying "you jumped to an invalid place, go away" is fine -
particularly since this can only happen if you use a gcc extension to
begin with.

So I don't think sparse is wrong, except for the total lack of any
error messages.

> I tried to simply discard the early optimization in expand but
> then when testing the kernel I got a whole bunch of warnings

I don't think we want to get rid of the early tree-level
simplifications. They are sensible and help avoid unnecessary work
later.

So I'd much rather just figure out some way to say "hmm, this goto is
to something that was removed earlier, let's just say so".

> I tried also to warn on gotos jumping into an expression statement.
> The idea was to give a new 'label_scope' for each such statement.
> Things are a bit complicated because the labels are implicitly
> declared by the gotos.

Yes. I think the gcc warning is nice, but I also think that it would
be entirely sufficient to not notice at an early stage, but only when
linearizing and hitting the "I'm branching to something that I can't
generate code for", and report it at that point, instead of being
clever and analyzing scopes up front.

                  Linus

Re: Interesting (?) failure case

[Luc Van Oostenryck]

On Thu, Apr 09, 2020 at 09:51:28AM -0700, Linus Torvalds wrote:
> On Wed, Apr 8, 2020 at 11:23 PM Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> >
> > Yes, the problem is caused at expand_conditional() where one of
> > the sides is throwed away if the condition is known. So the label
> > doesn't exist anymore and at linearization Sparse ends with a
> > jump to an unexisting BB.
> 
> Yes.
> 
> And I don't think that's really a problem per se. I think the gcc
> model of saying "you jumped to an invalid place, go away" is fine -
> particularly since this can only happen if you use a gcc extension to
> begin with.
> 
> So I don't think sparse is wrong, except for the total lack of any
> error messages.
> 
> > I tried to simply discard the early optimization in expand but
> > then when testing the kernel I got a whole bunch of warnings
> 
> I don't think we want to get rid of the early tree-level
> simplifications. They are sensible and help avoid unnecessary work
> later.

Yes, sure. I had hopped to be able to keep the advantages of the
value-expansion while keeping the original information but it
would need quite a bit changes and is certainly not worth this
"jump inside a (discarded) expression statement".
It's interesting, though, that a simple
	if (0) ...stuff... 
can't be discarded at expand time because of the gotos/labels.

> So I'd much rather just figure out some way to say "hmm, this goto is
> to something that was removed earlier, let's just say so".
> 
> > I tried also to warn on gotos jumping into an expression statement.
> > The idea was to give a new 'label_scope' for each such statement.
> > Things are a bit complicated because the labels are implicitly
> > declared by the gotos.
> 
> Yes. I think the gcc warning is nice, but I also think that it would
> be entirely sufficient to not notice at an early stage, but only when
> linearizing and hitting the "I'm branching to something that I can't
> generate code for", and report it at that point, instead of being
> clever and analyzing scopes up front.

Yes, that's certainly much easier and avoids the current garbage with
the IR and the diagnostic will, I think, still be informative enough.

-- Luc