Re: [Tinycc-devel] tcc grammar problems

[Thomas Preud'homme]

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:

commit 1988c974137f3042d9c38000fda3e00779fecab3
Author: Jiang <address@hidden>
Date:   Fri Aug 1 16:27:58 2014 +0800

    fix bitfields
    
    see:
    http://lists.nongnu.org/archive/html/tinycc-devel/2014-07/msg00023.html


"Fix casts to bitfield" followed by the quick testcase provided by grishka 
would be better.


diff --git a/tcc.h b/tcc.h
index c93cedf..a8cabb6 100644
--- a/tcc.h
+++ b/tcc.h
@@ -1192,6 +1192,17 @@ ST_DATA int func_var; /* true if current function is 
variadic */
 ST_DATA int func_vc;
 ST_DATA int last_line_num, last_ind, func_ind; /* debug last line number and 
pc */
 ST_DATA char *funcname;
+/* gen_ctrl */
+enum {
+    CTRL_NONE,
+    CTRL_CALL,
+    CTRL_FOCE,
+    CTRL_ARGS,
+    CTRL_RETS,
+    CTRL_INIT,
+    CTRL_USED,
+};
+ST_DATA int gen_ctrl;

A description of the use for each enumerator would be nice. Also, unless you 
mean something else, I think you should rename CTRL_FOCE into CTRL_FORCE. Also 
you seem to only use CTRL_FOCE and CTRL_INIT. So you should probably remove 
all the others. And why separating the kind of check? Why not just a switch 
which enables or not warnings? Please explain me why you feel the need for 
this enum.
 
 ST_INLN int is_float(int t);
 ST_FUNC int ieee_finite(double d);
diff --git a/tccgen.c b/tccgen.c
index 1a89d4a..73b759f 100644
--- a/tccgen.c
+++ b/tccgen.c
@@ -70,6 +70,7 @@ ST_DATA int func_var; /* true if current function is 
variadic (used by return in
 ST_DATA int func_vc;
 ST_DATA int last_line_num, last_ind, func_ind; /* debug last line number and 
pc */
 ST_DATA char *funcname;
+ST_DATA int gen_ctrl;
 
 ST_DATA CType char_pointer_type, func_old_type, int_type, size_type;
 
@@ -1909,8 +1910,9 @@ static void force_charshort_cast(int t)
 /* cast 'vtop' to 'type'. Casting to bitfields is forbidden. */
 static void gen_cast(CType *type)
 {
-    int sbt, dbt, sf, df, c, p;
+    int sbt, dbt, dt, sf, df, c, p, bb;
 
+    bb = type->t & VT_BITFIELD;

Why bb for the bitfield? Maybe you could use db (destination bitfield) to 
follow the same naming convention as df (destination float). Also you should 
add it just before the c, but that's really nitpick.

I'm not sure about dt. On one hand its the real destination basic type but on 
the other hand dbt is already used.

     /* special delayed cast for char/short */
     /* XXX: in some cases (multiple cascaded casts), it may still
        be incorrect */
@@ -1925,9 +1927,10 @@ static void gen_cast(CType *type)
     }
 
     dbt = type->t & (VT_BTYPE | VT_UNSIGNED);
+    dt = dbt & VT_BTYPE;
     sbt = vtop->type.t & (VT_BTYPE | VT_UNSIGNED);
 
-    if (sbt != dbt) {
+    if (sbt != dbt || bb) {
         sf = is_float(sbt);
         df = is_float(dbt);
         c = (vtop->r & (VT_VALMASK | VT_LVAL | VT_SYM)) == VT_CONST;
@@ -1959,6 +1962,8 @@ static void gen_cast(CType *type)
                     vtop->c.d = (double)vtop->c.ld;
             } else if (sf && dbt == (VT_LLONG|VT_UNSIGNED)) {
                 vtop->c.ull = (unsigned long long)vtop->c.ld;
+                if(bb)
+                    goto to_min;
             } else if (sf && dbt == VT_BOOL) {
                 vtop->c.i = (vtop->c.ld != 0);
             } else {
@@ -1975,24 +1980,53 @@ static void gen_cast(CType *type)
                 else if (sbt != VT_LLONG)
                     vtop->c.ll = vtop->c.i;
 
-                if (dbt == (VT_LLONG|VT_UNSIGNED))
+                if (dbt == (VT_LLONG|VT_UNSIGNED)){
                     vtop->c.ull = vtop->c.ll;
-                else if (dbt == VT_BOOL)
+                    if(bb)
+                        goto to_min;
+                }else if (dbt == VT_BOOL)

Spacing issue. You should have a space before '{' and after '}'

                     vtop->c.i = (vtop->c.ll != 0);
 #ifdef TCC_TARGET_X86_64
                 else if (dbt == VT_PTR)
                     ;
 #endif
                 else if (dbt != VT_LLONG) {
-                    int s = 0;
-                    if ((dbt & VT_BTYPE) == VT_BYTE)
-                        s = 24;
-                    else if ((dbt & VT_BTYPE) == VT_SHORT)
-                        s = 16;
-                    if(dbt & VT_UNSIGNED)
-                        vtop->c.ui = ((unsigned int)vtop->c.ll << s) >> s;
-                    else
-                        vtop->c.i = ((int)vtop->c.ll << s) >> s;
+                    unsigned long long ull;
+                    long long ll;
+                    int s, warr;

It would be better to name this variable "warn".

+to_min:

Why not use the label "cast_bitfield"?

+                    warr = 0;
+                    if(bb){
+                        s = 64 - ((type->t >> (VT_STRUCT_SHIFT + 6)) & 0x3f);

Spacing issue again and I don't understand the + 6. The bitfield size is 
encoded from bit (1 << VT_STRUCT_SHIFT) on 6 bits. So you should do:
(type->t >> VT_STRUCT_SHIFT) & 0x3f (that is remove all the bits before the 
bitfield size and then only keep the 6 least significant bits.

+                        type->t  = type->t  & ~(VT_BITFIELD | (-1 << 
VT_STRUCT_SHIFT));

(-1 << VT_STRUCT_SHIFT) will not do what you want it to do. Currently 
VT_STRUCT_SHIFT is 19 so you'd have 0xffffffff << 19 which would give 
0xfff80000 when you actually want 0x01f80000.

So instead you should do ((0x3f << VT_BITFIELD) - 1). Also it would be nice if 
you get rid of 0x3f and other 6 in tcc's code by adding 2 macros (Let's say 
BITFIELD_WIDTH_BITS and BITFIELD_WIDTH_MASK).


+                    }else if (dt == VT_BYTE){
+                        s = 56;
+                    }else if (dt == VT_SHORT){
+                        s = 48;
+                    }else{
+                        s = 32;
+                    }
+
+                    ull = (vtop->c.ull << s) >> s;
+                    ll = (vtop->c.ll << s) >> s;

Mmmh, clever for the ll :)

+                    if(ull != vtop->c.ull && ll != vtop->c.ll){
+                        warr = 1;
+                    }
+                    if(warr){
+                        if(dt == VT_ENUM)
+                            dbt |= VT_UNSIGNED;
+                        if(gen_ctrl != CTRL_FOCE){
+                            if(dt == VT_ENUM)
+                                tcc_warning("large integer implicitly 
truncated to unsigned type");
+                            else
+                                tcc_warning("overflow in implicit constant 
conversion");
+                        }
+                    }
+                    if(dbt & VT_UNSIGNED){
+                        vtop->c.ull = ull;
+                    }else{
+                        vtop->c.ll = ll;
+                    }
                 }
             }
         } else if (p && dbt == VT_BOOL) {

Ok so I'm not a big fan of this goto. Maybe you could put all this code just 
before the line 1997 ("} else if (p && dbt == VT_BOOL) {" above) and use a 
variable "check_bitfield" that would be initialized to 0 and set to 1 in all 
the place where you currently have a goto min. What do you think?


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.

Best regards,

Thomas

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