Re: [Tinycc-devel] [PATCH] When handling '.' operator, cope with VT_LLOCAL

[Michael Matz]

Hi,

On Fri, 20 Feb 2015, Edmund Grimley Evans wrote:

> Does this look all right?

Do you have a testcase?  I'll agree that there's possibly something fishy, 
but the change doesn't look 100% right.

... looksy looksy ...

Hmm, maybe I trace your path how you found this :)  When trying to use 
VT_LLOCAL to replace VT_REF, when the member in question is not an array, 
right? ;)  Because then without your patch this example generates wrong 
code under win64:

struct S {int a; int x[7];};
int f (int a, int b, int c, int d, struct S s)
{
  return s.a;
}

So, something is wrong indeed.  The problem I have with your patch:

@@ -4024,6 +4024,8 @@ ST_FUNC void unary(void)
             vtop->type.t |= qualifiers;
             /* an array is never an lvalue */
             if (!(vtop->type.t & VT_ARRAY)) {
+                if ((vtop->r & (VT_VALMASK | VT_LVAL)) == (VT_LOCAL | VT_LVAL))
+                    vtop->r += VT_LLOCAL - VT_LOCAL;
                 vtop->r |= lvalue_type(vtop->type.t);
 #ifdef CONFIG_TCC_BCHECK
                 /* if bound checking, the referenced pointer must be checked */

is: this patches only one place where lvalue_type is called (i.e. where 
something on the stack is lvalueized again).  Why are the others trivially 
correct and don't need such handling?  Also, conceptually, I might have a 
problem with this: it fixes up things after they went wrong.  For 
instance, why isn't the problem in gen_op?  I mean, one invariant of 
gen_op (when called with non-optimizable args) is that it leaves a 
non-lvalue on the stack, so the code in unary() is correct as is, for 
those cases.  So, wouldn't it be better if the lvalue-to-rvalue conversion 
would happen also with gen_op(+,0)?


Ciao,
Michael.