Re: [Tinycc-devel] Bug in expression parser?

[Ben]

On Mon, 2010-10-25 at 11:53 +0200, grischka wrote:
> Ben wrote:
> > I have a patch that I think fixes the issue and/or I can explain what I
> > think has gone wrong.
> 
> Explanations welcome here, patches welcome there:
>       http://repo.or.cz/w/tinycc.git

Ah, thanks.  I did not find the git repository first time round.  The
problem is still there in the git version.

The bug is that assignment expressions are parsed with with high
precedence (just lower than the unary operators and higher than the
multiplicative operators).

For example:

  int main(void) { int x; return 2 * x = 1; }

is accepted without error.  This should be a syntax error (if the parser
is very clever) or it should report that "2 * x" is not an lvalue if it
just uses an operator precedence grammar with "=" low and "*" high.
This is what gcc does.

I think that the function expr_eq has been changed at some time so that
it parses only conditional expressions and a new function uneq was added
to handle assignment expressions.  However, this new function was called
in places where it is wrong to do so (in expr_prod for example) and was
not called in places where it should have been (in init_putv where the
EXPR_ANY case needs parse an assignment expression, not a conditional
expression).

Also, uneq tries to follow the grammar by calling unary on the left.  I
think this is very hard to get right.  It's simpler to pretend that all
higher-binding operators are permitted on the left (i.e. you try to
parse a conditional expression) and to pick up the problems later when
testing for a lvalue expression.

Some of these effects were hidden by the high binding of "=".  For
example, at block scope:

  int x = 1, y = x = 2;

is valid and tcc accepted it because "=" has such a high priority depite
the fact that expr_eq only parses a condition expression for the
initialiser.  As a result, the same code is accepted a file scope where
it should not be.  In a similar vein, function arguments are currently
parsed as conditional expressions where they should be assignment
expressions.  Again, this does not show up because "=" is treated as a
high rather than a low priority operator.

I have pushed a patch to the mob patchwork that, I think, fixes these
errors.  This is the first time I've used git for anything but local
work so it is possible I've not done it right.  I hope it is OK.

It is not a minimal patch in that it seemed to make sense to rename the
functions and to move the one that parses assignment expressions lower
down the file so that the pattern or ever decreasing operator precedence
is maintained.

Therefore the patch

        * renames expr_eq to expr_cond because it parses conditionals;
        * renames uneq to expr_eq and moves it to below expr_cond;
        * modifies expr_prod to look for unary expressions rather than
        assignment expressions.

The other related issues are all handled by the name change.  expr_eq
now does parse assignment expressions and expr_cond does not so calling
expr_eq is now correct in function arguments, comma expressions and
block scope initialisers.

I hope this explanation is reasonably clear and that the patch is
useful.

I have done only a little testing.  "make test" works and the patched
tcc parses all the test cases that I found to be wrong initially, but I
don't feel that is enough testing for a change like this.

-- 
Ben Bacarisse