Re: [Tinycc-devel] tcc grammar problems

[Thomas Preud'homme]

Le mardi 05 août 2014, 22:08:13 Thomas Preud'homme a écrit :
> Le vendredi 01 août 2014, 16:37:15 jiang a écrit :
> > my patch:See Attachment
> > You look at, if no problem, I'll push mob
> 
> Ok, here is what I noticed so far:
> 

[SNIP review part 1]

> 
> 
> Ok. I'll stop this for now and will continue to review tomorrow or the day
> after tomorrow. Wait before doing any modifications as I might have some
> more important remarks that would require a more important rewrite. However
> from what I've seen so far (the ctrl enum excepted) it seems to be going in
> the right direction.

Let's continue, shall we?

@@ -2024,67 +2058,78 @@ static void gen_cast(CType *type)


What about the "else if (sf) case before that? A cast from a float to a 
bitfield 
is possible and does not seem to be handled here. Am I wrong?


                         gen_cast(type);
                     }
                 }
+            } else {
+
 #ifndef TCC_TARGET_X86_64
-            } else if ((dbt & VT_BTYPE) == VT_LLONG) {
-                if ((sbt & VT_BTYPE) != VT_LLONG) {
-                    /* scalar to long long */
-                    /* machine independent conversion */
-                    gv(RC_INT);
-                    /* generate high word */
-                    if (sbt == (VT_INT | VT_UNSIGNED)) {
-                        vpushi(0);
+                if ((dbt & VT_BTYPE) == VT_LLONG) {
+                    if ((sbt & VT_BTYPE) != VT_LLONG) {
+                        /* scalar to long long */
+                        /* machine independent conversion */
                         gv(RC_INT);
-                    } else {
-                        if (sbt == VT_PTR) {
-                            /* cast from pointer to int before we apply
-                               shift operation, which pointers don't 
support*/
-                            gen_cast(&int_type);
+                        /* generate high word */
+                        if (sbt == (VT_INT | VT_UNSIGNED)) {
+                            vpushi(0);
+                            gv(RC_INT);
+                        } else {
+                            if (sbt == VT_PTR) {
+                                /* cast from pointer to int before we apply
+                                   shift operation, which pointers don't 
support*/


You need to reformat this comment to don't go beyond 80 columns.


+                                gen_cast(&int_type);
+                            }
+                            gv_dup();
+                            vpushi(31);
+                            gen_op(TOK_SAR);
                         }
-                        gv_dup();
-                        vpushi(31);
-                        gen_op(TOK_SAR);
+                        /* patch second register */
+                        vtop[-1].r2 = vtop->r;
+                        vpop();
                     }
-                    /* patch second register */
-                    vtop[-1].r2 = vtop->r;
-                    vpop();
                 }
 #else
-            } else if ((dbt & VT_BTYPE) == VT_LLONG ||
-                       (dbt & VT_BTYPE) == VT_PTR ||
-                       (dbt & VT_BTYPE) == VT_FUNC) {
-                if ((sbt & VT_BTYPE) != VT_LLONG &&
-                    (sbt & VT_BTYPE) != VT_PTR &&
-                    (sbt & VT_BTYPE) != VT_FUNC) {
-                    /* need to convert from 32bit to 64bit */
-                    int r = gv(RC_INT);
-                    if (sbt != (VT_INT | VT_UNSIGNED)) {
-                        /* x86_64 specific: movslq */
-                        o(0x6348);
-                        o(0xc0 + (REG_VALUE(r) << 3) + REG_VALUE(r));
+                if((dbt & VT_BTYPE) == VT_LLONG || (dbt & VT_BTYPE) == VT_PTR 
||


For the same reason, you need to use a new line after the first ||.


+                            (dbt & VT_BTYPE) == VT_FUNC) {
+                    if ((sbt & VT_BTYPE) != VT_LLONG && (sbt & VT_BTYPE) != 
VT_PTR &&


And again here after the first &&.


+                        (sbt & VT_BTYPE) != VT_FUNC) {
+                        /* need to convert from 32bit to 64bit */
+                        int r = gv(RC_INT);
+                        if (sbt != (VT_INT | VT_UNSIGNED)) {
+                            /* x86_64 specific: movslq */
+                            o(0x6348);
+                            o(0xc0 + (REG_VALUE(r) << 3) + REG_VALUE(r));
+                        }
                     }
                 }
 #endif
-            } else if (dbt == VT_BOOL) {
-                /* scalar to bool */
-                vpushi(0);
-                gen_op(TOK_NE);
-            } else if ((dbt & VT_BTYPE) == VT_BYTE || 
-                       (dbt & VT_BTYPE) == VT_SHORT) {
-                if (sbt == VT_PTR) {
-                    vtop->type.t = VT_INT;
-                    tcc_warning("nonportable conversion from pointer to 
char/short");
+                else if (dbt == VT_BOOL) {
+                    /* scalar to bool */
+                    vpushi(0);
+                    gen_op(TOK_NE);
+                } else if ((dbt & VT_BTYPE) == VT_BYTE || (dbt & VT_BTYPE) == 
VT_SHORT) {
+                    if (sbt == VT_PTR) {
+                        vtop->type.t = VT_INT;
+                        tcc_warning("nonportable conversion from pointer to 
char/short");
+                    }
+                    force_charshort_cast(dbt);
+                } else if ((dbt & VT_BTYPE) == VT_INT) {
+                    /* scalar to int */
+                    if (sbt == VT_LLONG) {
+                        /* from long long: just take low order word */
+                        lexpand();
+                        vpop();
+                    }
+                    /* if lvalue and single word type, nothing to do because
+                       the lvalue already contains the real type size (see
+                       VT_LVAL_xxx constants) */
+                }

I'd put a blank line here to see more show more clearly that if is not part of 
this if sequence but is a new if. Also check for the 80 columns rule as there 
is some more cases and I don't want to list them all.


+                if(bb){
+                    int bit_size = (type->t >> (VT_STRUCT_SHIFT + 6)) & 0x3f;
+                    type->t  = type->t  & ~(VT_BITFIELD | (-1 << 
VT_STRUCT_SHIFT));


Same remark as in part 1 about how to properly zero the bitfield size bits.


+                    if(dt == VT_LLONG)
+                        vpushll((1ULL << bit_size) - 1ULL);
+                    else
+                        vpushi((1 << bit_size) - 1);
+                    gen_op('&');
                 }
-                force_charshort_cast(dbt);
-            } else if ((dbt & VT_BTYPE) == VT_INT) {
-                /* scalar to int */
-                if (sbt == VT_LLONG) {
-                    /* from long long: just take low order word */
-                    lexpand();
-                    vpop();
-                } 
-                /* if lvalue and single word type, nothing to do because
-                   the lvalue already contains the real type size (see
-                   VT_LVAL_xxx constants) */
             }
         }
     } else if ((dbt & VT_BTYPE) == VT_PTR && !(vtop->r & VT_LVAL)) {


Ok.

I'm more sceptic about the changes in vstore(). Let's see:


@@ -2463,24 +2508,36 @@ static void gen_assign_cast(CType *dt)
 /* store vtop in lvalue pushed on stack */
 ST_FUNC void vstore(void)
 {
-    int sbt, dbt, ft, r, t, size, align, bit_size, bit_pos, rc, delayed_cast;
+    int sbt, dbt, ft, cc, r, t, size, align, bit_size, bit_pos, rc, 
delayed_cast, ret;


In gen_cast the variable that holds whether the Svalue is constant or not is 
simply called "c", you should probably use the same name here.

More importantly I don't understand the ret variable. Its name is badly chosen 
and I don't see why we need it.
 

     ft = vtop[-1].type.t;
     sbt = vtop->type.t & VT_BTYPE;
     dbt = ft & VT_BTYPE;
+    ret = delayed_cast = 0;
+    cc = (vtop->r & (VT_VALMASK | VT_LVAL | VT_SYM)) == VT_CONST;
     if ((((sbt == VT_INT || sbt == VT_SHORT) && dbt == VT_BYTE) ||
-         (sbt == VT_INT && dbt == VT_SHORT))
-       && !(vtop->type.t & VT_BITFIELD)) {
+        (sbt == VT_INT && dbt == VT_SHORT)) && !(vtop->type.t & VT_BITFIELD) 
&& !cc) {
         /* optimize char/short casts */
         delayed_cast = VT_MUSTCAST;
         vtop->type.t = ft & (VT_TYPE & ~(VT_BITFIELD | (-1 << 
VT_STRUCT_SHIFT)));
         /* XXX: factorize */
         if (ft & VT_CONSTANT)
             tcc_warning("assignment of read-only location");
+    }else if(ft & VT_BITFIELD){
+        ret = (vtop > (vstack + 1) || gen_ctrl == CTRL_INIT);


Why this vtop > (vstack + 1)? What's so special about a stack with 3 entries 
or more? It looks fishy.


+        if(dbt == VT_BOOL)
+            vtop[-1].type.t = (vtop[-1].type.t & ~VT_BTYPE) | (VT_BYTE | 
VT_UNSIGNED);
+        gen_assign_cast(&vtop[-1].type);
+        if(ret){
+            if((vtop->r & (VT_VALMASK|VT_LVAL|VT_SYM)) < VT_CONST){
+                gv_dup();
+            }else{
+                vpushv(&vtop[0]);
+            }


Why was all of this moved from the if below to here? I'm maybe missing 
something as I'm quite new to patch review so please explain me. Also we lost 
the comments in the process. If this move is really needed, you can remove the 
curly braces just above as they are no longer needed.

Another thing: there was a vswap() before between the gv_dup and the vrott. 
Although I don't see how it would be useful, I prefer to assume that I'm too 
stupid and it was there for a good reason (of course it could be just a bug).


+            vrott(3);
+        }
     } else {
-        delayed_cast = 0;
-        if (!(ft & VT_BITFIELD))
-            gen_assign_cast(&vtop[-1].type);
+        gen_assign_cast(&vtop[-1].type);
     }


Remove the curly braces ('{' and '}') as they are no longer necessary.
 

     if (sbt == VT_STRUCT) {
@@ -2522,37 +2579,18 @@ ST_FUNC void vstore(void)
         /* bitfield store handling */
         bit_pos = (ft >> VT_STRUCT_SHIFT) & 0x3f;
         bit_size = (ft >> (VT_STRUCT_SHIFT + 6)) & 0x3f;
+
         /* remove bit field info to avoid loops */
         vtop[-1].type.t = ft & ~(VT_BITFIELD | (-1 << VT_STRUCT_SHIFT));
 
-        /* duplicate source into other register */
-        gv_dup();
-        vswap();
-        vrott(3);
-
-        if((ft & VT_BTYPE) == VT_BOOL) {
-            gen_cast(&vtop[-1].type);
-            vtop[-1].type.t = (vtop[-1].type.t & ~VT_BTYPE) | (VT_BYTE | 
VT_UNSIGNED);
-        }
+        vpushi(bit_pos);
+        gen_op(TOK_SHL);


Alright, the masking is now done in gen_cast().

 
         /* duplicate destination */
-        vdup();
-        vtop[-1] = vtop[-2];
+        vpushv(&vtop[-1]);
 
-        /* mask and shift source */
-        if((ft & VT_BTYPE) != VT_BOOL) {
-            if((ft & VT_BTYPE) == VT_LLONG) {
-                vpushll((1ULL << bit_size) - 1ULL);
-            } else {
-                vpushi((1 << bit_size) - 1);
-            }
-            gen_op('&');
-        }
-        vpushi(bit_pos);
-        gen_op(TOK_SHL);
         /* load destination, mask and or with source */
-        vswap();
-        if((ft & VT_BTYPE) == VT_LLONG) {
+        if(dbt == VT_LLONG) {
             vpushll(~(((1ULL << bit_size) - 1ULL) << bit_pos));
         } else {
             vpushi(~(((1 << bit_size) - 1) << bit_pos));
@@ -2563,8 +2601,8 @@ ST_FUNC void vstore(void)
         vstore();
 
         /* pop off shifted source from "duplicate source..." above */
-        vpop();
-
+        if(ret)
+            vtop--;
     } else {
 #ifdef CONFIG_TCC_BCHECK
         /* bound check case */

I actually don't understand why the original code keeps a copy of the source 
value into a register via a Svalue under the destination and then pop it off at 
the end. Also the comment that says it pop off the shifted source confuse me 
even more because the copy is done before the shift is made.

It seems you changed a bit since you copy the source after the gen_assign_cast 
and thus after it is masked (but not shifted). However since the copy at the 
bottom of the stack does not seem to be used I don't see the impact. Do you 
understand better what's going on with this gv_dup ()? If not, have you tried 
removing this? How does it break?


So here you are, a few comments but many questions. Please take your time to 
explain these questions as they are important to understand your patch. I 
suggest that you find a friend that speak well English to help you write that 
message as I will have trouble to understand if the explanation is not clear.


Ok, rest for part 3 (hopefully tomorrow). :)

Best regards,

Thomas

signature.asc
Description: This is a digitally signed message part.