Re: [PATCH 15/17] scope: give a scope for labels & gotos

[Luc Van Oostenryck <luc.vanoostenryck@gmail.com>]

On Mon, Apr 13, 2020 at 10:52:19AM -0700, Linus Torvalds wrote:
> On Mon, Apr 13, 2020 at 9:16 AM Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> >
> > Note: the label's symbols are still created in the function
> >       scope since they belong to a single namespace.
> 
> Oh, I saw that 13/17 and was like "yeah, this is the right thing to
> do", because I thought you were going in a different direction.
> 
> But then here in 15/17 I think you're doing it wrong.
> 
> Just give the symbol the proper scope, and make it simply not even
> _parse_ right if somebody tries to use a label from the wrong scope,
> instead of making the symbol visible, and then having to check whether
> the scopes nested right later.
> 
> So I think you should just bite the bullet, and change the
> bind_symbol() function so that a NS_LABEL is bound to label scope.
> 
> Then you can remove this 15/17 entirely (and all the "is this scope
> nesting" code - nesting is automatically enforced by the symbol
> scope).
> 
> I think that's a much cleaner approach. Yes, it gives a different
> error from gcc, but I think it's a *better* error.
> 
> This:
> 
>    int fn1(int arg)
>    {
>       target:
>          return 0;
>    }
> 
>    int fn2(int arg)
>    {
>       goto target;
>    }
> 
> is invalid code, and 'target' isn't even visible in fn2, because it is
> a local label to fn1.
> 
> I think the exact same thing is the right thing to do for expression
> statements, so
> 
>    int fn(int arg)
>    {
>       goto inside;
>       return ({ inside: 0; });
>    }
> 
> should fail with the exact same error message of having an undefined
> label (which sparse currently gets wrong too, but you're fixing that
> elsewhere).
> 
> Because "inside" simply shouldn't be defined at all in the outer
> scope, and you can only branch _within_ a statement expression, the
> same way you can only branch within a function.
> 
> So I think statement expressions should basically work kind of like
> local "nested functions": they have access to the state outside, but
> the outside doesn't have access to the state inside that statement
> expression.

Yes, I agree and in fact (if I understand you correctly) it was what
I tried first, mainly because it was "conceptualy neat" and simpler.
But then it wasn't working correctly in all situations and I
convinced myself it couldn't. The problem was with code like:
	void foo(void)
	{
		... = ({ ...  goto out; ... });

	out:
		...;
	}

In this case, when 'goto out' is parsed, the corresponding label
symbol would be created in the inner scope and later when the label
is defined the symbol lookup will only look in the outer scope, see
nothing and declare another symbol for it, then the obvious scope
check will complain that the goto's label is undeclared.
But this code is legit and both occurences of the ident 'out' should
refer to the same label, right?

I didn't saw a proper solution for this, hence the current patch 15
where I'm keeping all labels in the usual function scope but where
the new label scope is associated to the STMT_GOTO & STMT_LABEL
and where evaluate_goto_statement() check in the scope of the
goto is contained in the one of the label definition via the
new helper is_in_scope(). This is less elegant than I would have
liked but again I don't see a better solution.

-- Luc