Re: [Tinycc-devel] tcc grammar problems

[Thomas Preud'homme]

Le lundi 18 août 2014, 20:39:26 jiang a écrit :
> Hi,Thomas
> 

Hi Jiang,

> 
> fixbitfields.patch
> Nothing bug

More than a week late sorry. Here is my review:

> commit 1c0c88408b56d0e4e36b795b7d92f25f6606988a
> Author: Jiang <address@hidden>
> Date:   Mon Aug 18 20:25:19 2014 +0800
> 
>     fix bitfields

"Fix assignment to bitfield" would be more precise.

>     
>     see:
>         
> http://lists.nongnu.org/archive/html/tinycc-devel/2014-07/msg00023.html
>     
>     Conflicts:
>       tests/tests2/03_struct.c

Remove the conflicts, there is no interest to see your private experiments in 
the commit message.

> 
> diff --git a/tcc.h b/tcc.h
> index c93cedf..fb618ec 100644
> --- a/tcc.h
> +++ b/tcc.h
> @@ -1192,6 +1192,13 @@ 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_FOCE,
> +    CTRL_INIT,

So you don't use CTRL_NONE and the use of CTRL_INIT seems wrong to me. 
Therefore just use a boolean variable like check_downcast and declare it in 
tccgen.c instead of declaring gen_ctrl.

By the way, it's not foce but force. ;)

> +};
> +ST_DATA int gen_ctrl;
>  
>  ST_INLN int is_float(int t);
>  ST_FUNC int ieee_finite(double d);
> diff --git a/tccgen.c b/tccgen.c
> index d3d1026..e321f96 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;

Here, declare check_downcast (you can use another name if you find a better 
one).

>  
>  ST_DATA CType char_pointer_type, func_old_type, int_type, size_type;
>  
> @@ -1017,6 +1018,7 @@ ST_FUNC void lexpand_nr(void)
>  }
>  #endif
>  
> +#ifndef TCC_TARGET_X86_64
>  /* build a long long from two ints */
>  static void lbuild(int t)
>  {
> @@ -1025,6 +1027,7 @@ static void lbuild(int t)
>      vtop[-1].type.t = t;
>      vpop();
>  }
> +#endif
>  
>  /* rotate n first stack elements to the bottom 
>     I1 ... In -> I2 ... In I1 [top is right]

Mmmh, you fix several issues together. It's ok here because your patch is 
already big anyway and it's related but try to do these in separate patch next 
time.

> @@ -1087,7 +1090,8 @@ static void gv_dup(void)
>      int rc, t, r, r1;
>      SValue sv;
>  
> -    t = vtop->type.t;
> +#ifndef TCC_TARGET_X86_64
> +    t = VT_INT;
>      if ((t & VT_BTYPE) == VT_LLONG) {
>          lexpand();
>          gv_dup();
> @@ -1097,15 +1101,16 @@ static void gv_dup(void)
>          vrotb(4);
>          /* stack: H L L1 H1 */
>          lbuild(t);
> -        vrotb(3);
> -        vrotb(3);
> +        vrott(3);
>          vswap();
>          lbuild(t);
>          vswap();
> -    } else {
> +    } else
> +#else
> +    t = vtop->type.t;
> +#endif
> +    {
>          /* duplicate value */
> -        rc = RC_INT;
> -        sv.type.t = VT_INT;
>          if (is_float(t)) {
>              rc = RC_FLOAT;
>  #ifdef TCC_TARGET_X86_64
> @@ -1113,8 +1118,9 @@ static void gv_dup(void)
>                  rc = RC_ST0;
>              }
>  #endif
> -            sv.type.t = t;
> -        }
> +        }else
> +            rc = RC_INT;
> +        sv.type.t = t;
>          r = gv(rc);
>          r1 = get_reg(rc);
>          sv.r = r;

Rather than doing all this, it would be better to guard the if (... VT_LLONG) 
by that conditional, like is done in gv () after the line "r = get_reg(rc);" 
to check against VT_QLONG or VT_LLONG.

Note: it would be nice to remove all these conditionals and replace by 
appropriate 
macro in the backends one day.


***********************

Let's do a first pause now. I reviewed your patch completely but when I arrived 
at vstore () I wondered if gen_cast is the right place to deal with store in a 
bitfield. In C there is no way to cast explicitely to a bitfield. It's only 
done 
implicitely when doing a store. And then I realized when reading init_putv 
that the initialization doesn't go through vstore which might be the reason to 
do this in gen_cast. Is it because of initialization that you deal with 
bitfield in gen_cast? Can you point me to where is the cast done in the 
initialization?

If it's not this, don't read further as my review only makes sense if checking 
for bitfield in gen_cast is the right approach.

***********************


> @@ -1909,8 +1915,11 @@ 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, bitfield;
> +    CType dtype;
>  
> +    dtype = *type;
> +    bitfield = dtype.t & VT_BITFIELD;

Naming is better, thanks. :)

Three things can be improved:

1) I'm not convince about the need for dtype (See below). I don't mind you to 
rename the type parameter to dtype to make it more clear what type we are 
talking about but there is no need for another CType parameter. Don't forget 
to update the comment on top of the function in this case.
2) Reverse dt and dbt. dbt should be dtype.t & VT_BTYPE while dt would include 
the signedness. For consistency rename sbt into st.
3) bitfield doesn't show that it's about whether the destination is a bitfield. 
And actually we don't need it. I suggest instead that dt should be the basic 
type, the signedness and whether it's a bitfield. For st you only keep the 
basic type and the signedness so that st != dt would also be true when the 
destination is a bitfield, no matter whether the source is or not. This also 
has some huge advantages for reducing the amount of code to change (see 
below).

>      /* special delayed cast for char/short */
>      /* XXX: in some cases (multiple cascaded casts), it may still
>         be incorrect */
> @@ -1924,10 +1933,11 @@ static void gen_cast(CType *type)
>          gv(RC_INT);
>      }
>  
> -    dbt = type->t & (VT_BTYPE | VT_UNSIGNED);
> +    dbt = dtype.t & (VT_BTYPE | VT_UNSIGNED);
> +    dt = dbt & VT_BTYPE;
>      sbt = vtop->type.t & (VT_BTYPE | VT_UNSIGNED);
>  
> -    if (sbt != dbt) {
> +    if (sbt != dbt || bitfield) {
>          sf = is_float(sbt);
>          df = is_float(dbt);
>          c = (vtop->r & (VT_VALMASK | VT_LVAL | VT_SYM)) == VT_CONST;

As I said, with the point 3) you would not need to test for bitfield here.


The change below could be better done. You need to understand how is the code 
currently structured and why. To cast a constant, you need to read the 
constant from the appropriate CType field according to its current type (source 
type) and then store the result in the appropriate CType field according to its 
destination type. To avoid having a cartesian product (test all the 
combinations of source/destination), (almost, unsigned long long is handled 
separately because casting it to long long would loose some information) 
everything is cast to a long long and then to its destination type.

This way, you need one test for each possible source type and one test for 
each possible destination type. So you have NS + ND tests where NS is the 
number of source type test and ND is the number of destination type test. 
Otherwise the number of tests would be NS * ND.

> @@ -1959,6 +1969,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(bitfield)
> +                    goto cast_bitfield;

So here you keep the code unchanged as dt (you'll have renamed dbt in dt, 
remember?) would be VT_LLONG|VT_UNSIGNED|VT_BITFIELD if it's a bitfield with an 
unsigned long long base type. Therefore it won't match this if and continue 
below until the "if(sf)" and be cast to a long long. It is fine because we know 
the destination is a bitfield and thus at least one bit of the unsigned long 
long will not be used. Casting to long long instead of unsigned long long will 
do exactly that, remove one bit.

This means that we want to set "warn" (don't call it "warr") to 1 here and 
therefore "warn" should be declared just before, at the begining of the "} 
else {" line. By the way, you check for downcast between integer but what 
about downcast from a float? Even a float can have a much higher value than a 
long long.

Note that this is undefined if the integral part of the float value cannot be 
represented in the new integer type but that's the author of the code tcc is 
compiling to know what he's doing.


>              } else if (sf && dbt == VT_BOOL) {
>                  vtop->c.i = (vtop->c.ld != 0);
>              } else {
> @@ -1975,24 +1987,53 @@ static void gen_cast(CType *type)
>                  else if (sbt != VT_LLONG)
>                      vtop->c.ll = vtop->c.i;

Since the problem is only about writing to a bitfield that cannot store the 
source value, we should keep this part of the original code unchanged: we only 
care about the downcast part.

>  
> -                if (dbt == (VT_LLONG|VT_UNSIGNED))
> +                if (dbt == (VT_LLONG|VT_UNSIGNED)){
>                      vtop->c.ull = vtop->c.ll;
> -                else if (dbt == VT_BOOL)
> +                    if(bitfield)
> +                        goto cast_bitfield;
> +                }else if (dbt == VT_BOOL)

Here we can keep unchanged (except renaming dbt into dt) since a bitfield with 
an unsigned long long base type would have VT_LLONG|VT_UNSIGNED|VT_BITFIELD in 
dt and would thus not trigger this if condition.


>                      vtop->c.i = (vtop->c.ll != 0);
>  #ifdef TCC_TARGET_X86_64
>                  else if (dbt == VT_PTR)
>                      ;
>  #endif
>                  else if (dbt != VT_LLONG) {

Still ok to keept it unchanged since dt would have VT_BITFIELD if it's a 
bitfield and would be different from VT_LLONG.

> -                    int s = 0;

You'd insert the bitfield part here. You'd test "if (dt & VT_BITFIELD)" for 
that.

> -                    if ((dbt & VT_BTYPE) == VT_BYTE)

Here it would become an "else if".

> -                        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;

We don't need "ull" as everything will be in vtop->c.ull as explained above.

> +                    long long ll;
> +                    int s, warr;
> +cast_bitfield:
> +                    warr = 0;
> +                    if(bitfield){
> +                        s = 64 - ((dtype.t >> (VT_STRUCT_SHIFT + 6)) & 
0x3f);

Ok.

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

I'm not sure about this. Why do you need to remove the information about the 
bitfield? You're casting to a bitfield so it sounds about right to keep it. If 
at some point in another function you can ignore it there.

> +                    }else if (dt == VT_BYTE){
> +                        s = 56;
> +                    }else if (dt == VT_SHORT){
> +                        s = 48;
> +                    }else{
> +                        s = 32;
> +                    }

Follow my remarks above.

> +
> +                    ull = (vtop->c.ull << s) >> s;

Not needed.

> +                    ll = (vtop->c.ll << s) >> s;

> +                    if(ull != vtop->c.ull && ll != vtop->c.ll){
> +                        warr = 1;
> +                    }

Remove the useless '{' and '}' and 

> +                    if(warr){
> +                        if(dt == VT_ENUM)
> +                            dbt |= VT_UNSIGNED;

No, enum are int. C99 §6.7.2.2 paragraph 2.

> +                        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");

For the truncation to unsigned type you should check st & VT_UNSIGNED != dt & 
VT_UNSIGNED. You should also check this for cast from unsigned long long to 
long long but be careful to not warn twice.

> +                        }
> +                    }
> +                    if(dbt & VT_UNSIGNED){
> +                        vtop->c.ull = ull;
> +                    }else{
> +                        vtop->c.ll = ll;
> +                    }
>                  }
>              }
>          } else if (p && dbt == VT_BOOL) {

Tcc expect that char, short and int constant are stored in c.i and their 
unsigned counterpart in c.ui. But don't forget that the unsigned long long 
case is a possibility here because we explicitely made bitfield of unsigned 
long long base type go through the same path as int, short and char. So you 
need to test for this and store back in c.ull in this case.


> @@ -2018,14 +2059,33 @@ static void gen_cast(CType *type)
>                          dbt != VT_LLONG)
>                          dbt = VT_INT;
>                      gen_cvt_ftoi1(dbt);
> -                    if (dbt == VT_INT && (type->t & (VT_BTYPE | 
VT_UNSIGNED)) != dbt) {
> +                    if (dbt == VT_INT && (dtype.t & (VT_BTYPE | 
VT_UNSIGNED)) != dbt) {
>                          /* additional cast for char/short... */
>                          vtop->type.t = dbt;
>                          gen_cast(type);
>                      }
>                  }

Ok.

> +            } else if (dbt == VT_BOOL) {
> +                /* scalar to bool */
> +                vpushi(0);
> +                gen_op(TOK_NE);
> +            }else if(bitfield){

Missing space after "if". Don't forget to change it to dt & VT_BITFIELD.

> +                int bits, bit_size = (dtype.t >> (VT_STRUCT_SHIFT + 6)) & 
0x3f;
> +
> +                dtype.t  = dtype.t  & ~(VT_BITFIELD | (-1 << 
VT_STRUCT_SHIFT));

The bit_size computation should be done at the top of the function since you 
already do it for the constant case. As to the dtype I already said I think 
it's the wrong place.

> +                gen_cast(&dtype);
> +                if (dt == VT_LLONG) {
> +                    bits = 64 - bit_size;
> +                } else
> +                    bits = 32 - bit_size;
> +                vpushi(bits);
> +                gen_op(TOK_SHL);
> +                vpushi(bits);
> +                /* NOTE: transformed to SHR if unsigned */
> +                gen_op(TOK_SAR);

It doesn't account for the fact that the source might also be in a bitfield. 
You should check if the source is a bitfield and if yes first cast it to its 
base type before continue. Previously this was done implicetly by gv_dup() in 
vstore (at the top of the block that deal with bitfield).

> +            }else

Remove the else line.

>  #ifndef TCC_TARGET_X86_64
> -            } else if ((dbt & VT_BTYPE) == VT_LLONG) {
> +            if ((dbt & VT_BTYPE) == VT_LLONG) {

Don't change anything here.

>                  if ((sbt & VT_BTYPE) != VT_LLONG) {
>                      /* scalar to long long */
>                      /* machine independent conversion */
> @@ -2048,12 +2108,11 @@ static void gen_cast(CType *type)
>                      vtop[-1].r2 = vtop->r;
>                      vpop();
>                  }
> +            }

Don't change anything here.

>  #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 &&
> +            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 &&

Extraneous change. Keep the lines unchanged.

>                      (sbt & VT_BTYPE) != VT_FUNC) {
>                      /* need to convert from 32bit to 64bit */
>                      int r = gv(RC_INT);
> @@ -2063,13 +2122,9 @@ static void gen_cast(CType *type)
>                          o(0xc0 + (REG_VALUE(r) << 3) + REG_VALUE(r));
>                      }

Oh wow, there is code in tccgen.c. I know this is only executed for x86_64 
target but still: erk. Another thing that would be nice to fix one day. (I know 
it's not your code, I'm *not* asking you to change something for your patch 
here)

>                  }
> +            }

Don't change anything here.

>  #endif
> -            } else if (dbt == VT_BOOL) {
> -                /* scalar to bool */
> -                vpushi(0);
> -                gen_op(TOK_NE);

Ok.

> -            } else if ((dbt & VT_BTYPE) == VT_BYTE || 
> -                       (dbt & VT_BTYPE) == VT_SHORT) {
> +            else  if ((dbt & VT_BTYPE) == VT_BYTE || (dbt & VT_BTYPE) == 
VT_SHORT) {

Don't change anything here.

>                  if (sbt == VT_PTR) {
>                      vtop->type.t = VT_INT;
>                      tcc_warning("nonportable conversion from pointer to 
char/short");
> @@ -2081,7 +2136,7 @@ static void gen_cast(CType *type)
>                      /* from long long: just take low order word */
>                      lexpand();
>                      vpop();
> -                } 
> +                }

Extraneous change.

>                  /* if lvalue and single word type, nothing to do because
>                     the lvalue already contains the real type size (see
>                     VT_LVAL_xxx constants) */
> @@ -2093,7 +2148,7 @@ static void gen_cast(CType *type)
>          vtop->r = (vtop->r & ~VT_LVAL_TYPE)
>                    | (lvalue_type(type->ref->type.t) & VT_LVAL_TYPE);
>      }
> -    vtop->type = *type;
> +    vtop->type = dtype;
>  }

Ok.

>  
>  /* return type size as known at compile time. Put alignment at 'a' */
> @@ -2463,24 +2518,35 @@ 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;

cc makes me think at "conditional code" so you'd better use just "c" instead, 
as in gen_cast. No need for "ret" (see below).

>  
>      ft = vtop[-1].type.t;
>      sbt = vtop->type.t & VT_BTYPE;
>      dbt = ft & VT_BTYPE;
> -    if ((((sbt == VT_INT || sbt == VT_SHORT) && dbt == VT_BYTE) ||
> -         (sbt == VT_INT && dbt == VT_SHORT))
> -     && !(vtop->type.t & VT_BITFIELD)) {
> +    ret = delayed_cast = 0;
> +    cc = (vtop->r & (VT_VALMASK | VT_LVAL | VT_SYM)) == VT_CONST;
> +    if (ft & VT_BITFIELD) {

Why put it here? It would be better to keep it below where it was.

> +        ret = (vtop > (vstack + 1) || gen_ctrl == CTRL_INIT);

I already told you vstore should not deal with more than 2 variables. Else why 
just deal with it here but not in other cases? So forget about ret and this 
whole line unless there is something tricky I missed with the initialization.

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

Maybe gv_dup should check for constness and do a vdup() instead otherwise do 
what it's already doing.

Why remove the vswap? Can you explain why it was useless?

> +            vrott(3);
> +        }
> +    }else if ((((sbt == VT_INT || sbt == VT_SHORT) && dbt == VT_BYTE) ||
> +        (sbt == VT_INT && dbt == VT_SHORT)) && !cc) {
>          /* optimize char/short casts */
>          delayed_cast = VT_MUSTCAST;
> -        vtop->type.t = ft & (VT_TYPE & ~(VT_BITFIELD | (-1 << 
VT_STRUCT_SHIFT)));

I think I'm getting tired, I'm not sure why this line was necessary or even 
correct. If the source was a bitfield there might be a shift necessary. I 
suppose it's done elsewhere but it's a bit confusing.

>          /* XXX: factorize */
>          if (ft & VT_CONSTANT)
>              tcc_warning("assignment of read-only location");
>      } else {
> -        delayed_cast = 0;
> -        if (!(ft & VT_BITFIELD))
> -            gen_assign_cast(&vtop[-1].type);
> +        gen_assign_cast(&vtop[-1].type);

Yeah since now you to deal with bitfields in gen_cast.

>      }
>  
>      if (sbt == VT_STRUCT) {
> @@ -2522,37 +2588,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);

You should have left this here instead of doing it at the top of the function 
(unless I missed something).

> -
> -        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);
> -        }

Same here.

> +        vpushi(bit_pos);
> +        gen_op(TOK_SHL);

Ok, you avoid the vswap later by moving this here.

>  
>          /* 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('&');
> -        }

Ok, now it's node in gen_cast with the double shift.

> -        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));

Good idea to improve things as you adapt the code.

> @@ -2563,8 +2610,8 @@ ST_FUNC void vstore(void)
>          vstore();
>  
>          /* pop off shifted source from "duplicate source..." above */
> -        vpop();
> -
> +        if(ret)
> +            vtop--;

As I said in my previous mail, you should not deal with more than 2 values in 
vstore(). So remove this if.

>      } else {
>  #ifdef CONFIG_TCC_BCHECK
>          /* bound check case */

Overall quite good so far. :)

> @@ -3610,7 +3657,7 @@ static void vpush_tokc(int t)
>  
>  ST_FUNC void unary(void)
>  {
> -    int n, t, align, size, r, sizeof_caller;
> +    int n, t, align, size, r, sizeof_caller, save_ctrl;
>      CType type;
>      Sym *s;
>      AttributeDef ad;
> @@ -3719,7 +3766,10 @@ ST_FUNC void unary(void)
>                      return;
>                  }
>                  unary();
> +                save_ctrl = gen_ctrl;
> +                gen_ctrl = CTRL_FOCE;
>                  gen_cast(&type);
> +                gen_ctrl = save_ctrl;
>              }
>          } else if (tok == '{') {
>              /* save all registers */

Ok, just change gen_ctrl as already mentionned at the top.

> @@ -5132,9 +5182,8 @@ static void decl_designator(CType *type, Section *sec, 
unsigned long c,
>  static void init_putv(CType *type, Section *sec, unsigned long c, 
>                        int v, int expr_type)
>  {
> -    int saved_global_expr, bt, bit_pos, bit_size;
> +    int saved_global_expr, bt, bit_pos, bit_size, save_ctrl;
>      void *ptr;
> -    unsigned long long bit_mask;
>      CType dtype;
>  
>      switch(expr_type) {
> @@ -5152,7 +5201,10 @@ static void init_putv(CType *type, Section *sec, 
unsigned long c,
>              tcc_error("initializer element is not constant");
>          break;
>      case EXPR_ANY:
> +        save_ctrl = gen_ctrl;
> +        gen_ctrl = CTRL_INIT;
>          expr_eq();
> +        gen_ctrl = save_ctrl;
>          break;
>      }
>      

You should add this in expr_eq(void) instead.

> @@ -5173,11 +5225,9 @@ static void init_putv(CType *type, Section *sec, 
unsigned long c,
>          if (!(type->t & VT_BITFIELD)) {
>              bit_pos = 0;
>              bit_size = 32;
> -            bit_mask = -1LL;
>          } else {
> -            bit_pos = (vtop->type.t >> VT_STRUCT_SHIFT) & 0x3f;
> -            bit_size = (vtop->type.t >> (VT_STRUCT_SHIFT + 6)) & 0x3f;
> -            bit_mask = (1LL << bit_size) - 1;
> +            bit_pos = (dtype.t >> VT_STRUCT_SHIFT) & 0x3f;
> +            bit_size = (dtype.t >> (VT_STRUCT_SHIFT + 6)) & 0x3f;

Why change to use dtype?

>          }
>          if ((vtop->r & VT_SYM) &&
>              (bt == VT_BYTE ||
> @@ -5191,10 +5241,10 @@ static void init_putv(CType *type, Section *sec, 
unsigned long c,
>          case VT_BOOL:
>              vtop->c.i = (vtop->c.i != 0);
>          case VT_BYTE:
> -            *(char *)ptr |= (vtop->c.i & bit_mask) << bit_pos;
> +            *(char *)ptr |= vtop->c.i << bit_pos;
>              break;
>          case VT_SHORT:
> -            *(short *)ptr |= (vtop->c.i & bit_mask) << bit_pos;
> +            *(short *)ptr |= vtop->c.i << bit_pos;
>              break;
>          case VT_DOUBLE:
>              *(double *)ptr = vtop->c.d;
> @@ -5203,19 +5253,19 @@ static void init_putv(CType *type, Section *sec, 
unsigned long c,
>              *(long double *)ptr = vtop->c.ld;
>              break;
>          case VT_LLONG:
> -            *(long long *)ptr |= (vtop->c.ll & bit_mask) << bit_pos;
> +            *(long long *)ptr |= vtop->c.ll << bit_pos;
>              break;
>          case VT_PTR:
>              if (vtop->r & VT_SYM) {
>                  greloc(sec, vtop->sym, c, R_DATA_PTR);
>              }
> -            *(addr_t *)ptr |= (vtop->c.ptr_offset & bit_mask) << bit_pos;
> +            *(addr_t *)ptr |= vtop->c.ptr_offset << bit_pos;
>              break;
>          default:
>              if (vtop->r & VT_SYM) {
>                  greloc(sec, vtop->sym, c, R_DATA_PTR);
>              }
> -            *(int *)ptr |= (vtop->c.i & bit_mask) << bit_pos;
> +            *(int *)ptr |= vtop->c.i << bit_pos;
>              break;
>          }
>          vtop--;

Oh I see. gen_cast is called before doing an initialization but not vstore 
which is why you do everything in gen_cast? Can you confirm that's the reason? 
Where is the cast performed?

> diff --git a/tests/tests2/03_struct.c b/tests/tests2/03_struct.c
> index 3b99e30..8b3acee 100644
> --- a/tests/tests2/03_struct.c
> +++ b/tests/tests2/03_struct.c
> @@ -13,12 +13,24 @@ static int eval_rept()
>      return 0;
>  }
>  
> +struct command_switch {
> +    unsigned int env:1;              /* Can come from MAKEFLAGS.  */
> +    unsigned int toenv:1;    /* Should be put in MAKEFLAGS.  */
> +    unsigned int no_makefile:1;      /* Don't propagate when remaking 
> makefiles.  
*/
> +};
> +static const struct command_switch switches = {1, 1, 3};
> +

I'm not sure I want to see so descriptive names. It seems to be taken from 
somewhere else. It's a testsuite, it doesn't have to look like normal code. 
It's just there to check that tcc behaves correctly given a code sample.

>  struct fred
>  {
>     int boris;
>     int natasha;
>  };
>  
> +void foo(int a, int b, int c)
> +{
> +    printf("%d / %d / %d\n", a, b, c);
> +}
> +

Why not call the function print_values() or something like this?

>  int main()
>  {
>      struct fred bloggs;
> @@ -43,5 +55,40 @@ int main()
>      printf("%d\n", jones[1].boris);
>      printf("%d\n", jones[1].natasha);
>  
> -   return 0;
> +    struct sbf1 {
> +        int f1 : 3;
> +        int : 2;
> +        int f2 : 1;
> +        int : 0;
> +        int f3 : 5;
> +        int f4 : 7;
> +        unsigned int f5 : 7;
> +    } st1;
> +    st1.f1 = st1.f2 = st1.f3 = st1.f4 = st1.f5 = 3;
> +    printf("%d %d %d %d %d\n",
> +         st1.f1, st1.f2, st1.f3, st1.f4, st1.f5);
> +
> +    struct { unsigned a:9, b:7, c:5; } s1;
> +            s1.a = s1.b = s1.c = 3;
> +    printf("%d / %d / %d\n", s1.a, s1.b, s1.c);
> +
> +    struct {
> +        unsigned a:9, b:5, c:7;
> +    } s2, *ps = &s2;
> +    int n = 250;
> +
> +    int ii = ps->a = ps->b = ps->c = n + 4;
> +    printf("%d / %d / %d\n", ps->a, ps->b, ps->c);
> +    printf("%d\n", ii);
> +
> +    ps->a = n + 4;
> +    ps->b = n + 4;
> +    ps->c = n + 4;
> +    printf("%d / %d / %d\n", ps->a, ps->b, ps->c);
> +
> +    printf("%d, %d, %d\n", switches.env, switches.toenv, 
switches.no_makefile);
> +
> +    foo(ps->a = n + 4, ps->b = n + 4, ps->c = n + 4);
> +
> +    return 0;
>  }
> diff --git a/tests/tests2/03_struct.expect b/tests/tests2/03_struct.expect
> index ecbf589..e6a0db1 100644
> --- a/tests/tests2/03_struct.expect
> +++ b/tests/tests2/03_struct.expect
> @@ -1,6 +1,15 @@
> +03_struct.c:8: warning: overflow in implicit constant conversion
> +03_struct.c:51: warning: overflow in implicit constant conversion
>  12
>  34
>  12
>  34
>  56
>  78
> +-1 -1 3 3 3
> +3 / 3 / 3
> +30 / 30 / 126
> +30
> +254 / 30 / 126
> +1, 1, 1
> +254 / 30 / 126

Ok for tests.

Cheers,

Thomas

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