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
p2