Le lundi 18 août 2014, 20:39:26 jiang a écrit : > Hi,Thomas > > fixstruct.patch > Let tcc compile yasm software
> commit 0c67ef96bc161927ea6562f6ba0fa3578132de38 > Author: Jiang <30155...@qq.com> > Date: Mon Aug 18 19:38:23 2014 +0800 > > Let the following be compiled: > > struct buffered_lines_head { > int *slh_first; > }; > > static int eval_rept() > { > struct buffered_lines_head { > int *slh_first; > } lines; > return 0; > } > > int main() > { > struct buffered_lines_head ppp; > ppp.slh_first = 0; > return 0; > } The main is useless here. The bug can already be reproduced with just the first 2 elements. > > diff --git a/tccgen.c b/tccgen.c > index 5fd127f..639ad64 100644 > --- a/tccgen.c > +++ b/tccgen.c > @@ -317,7 +317,7 @@ static void apply_visibility(Sym *sym, CType *type) > ElfW(Sym) *esym; > > esym = &((ElfW(Sym) *)symtab_section->data)[sym->c]; > - vis >>= VT_VIS_SHIFT; > + vis >>= VT_VIS_SHIFT; > esym->st_other = (esym->st_other & ~ELFW(ST_VISIBILITY)(-1)) | vis; > } > } Spurious change. Please remove. > @@ -2851,15 +2851,17 @@ static void struct_decl(CType *type, int u, int tdef) > /* we put an undefined size for struct/union */ > s = sym_push(v | SYM_STRUCT, &type1, 0, -1); > s->r = 0; /* default alignment is zero as gcc */ > - /* put struct/union/enum name in type */ > do_decl: > - type->t = u; > - type->ref = s; > - > if (tok == '{') { > + if (s->c != -1){ > + if(local_stack){ > + s = sym_push(v | SYM_STRUCT, &type1, 0, -1); > + s->r = 0; /* default alignment is zero as gcc */ > + }else{ > + tcc_error("struct/union/enum already defined"); > + } > + } > next(); > - if (s->c != -1) > - tcc_error("struct/union/enum already defined"); Why so much reordering? Only the extra if should be added and only to guard against tcc_error (that is the then clause is the tcc_error and there is no else clause). Also you should check its position against scope_stack_bottom. Basically you should forbid a definition of a structure with the same name in the same scope but authorize it in a more nested scope. I know, I forgot to do this myself in 3e56584223a67a6c2f41a43cf38e0960e9992238. I wonder if this should not be done in sym_push2 instead. I cannot think of something that could be defined locally but not redefined in inner scopes. This would benefit lots of places instead of just struct. > /* cannot be empty */ > c = 0; > /* non empty enums are not allowed */ > @@ -2900,9 +2902,9 @@ static void struct_decl(CType *type, int u, int tdef) > while (tok != '}') { > parse_btype(&btype, &ad); > while (1) { > - if (flexible) > - tcc_error("flexible array member '%s' not at the end of struct", > - get_tok_str(v, NULL)); > + if (flexible) > + tcc_error("flexible array member '%s' not at the end of struct", > + get_tok_str(v, NULL)); > bit_size = -1; > v = 0; > type1 = btype; Spurious change, remove it. > @@ -3035,6 +3037,9 @@ static void struct_decl(CType *type, int u, int tdef) > s->r = maxalign; > } > } > + /* put struct/union/enum name in type */ > + type->t = u; > + type->ref = s; > } Why move this from the top of the function to the end? > > /* return 1 if basic type is a type size (short, long, long long) */ > @@ -5704,12 +5709,12 @@ static void decl_initializer_alloc(CType *type, AttributeDef *ad, int r, > } else { > /* push global reference */ > sym = get_sym_ref(type, sec, addr, size); > - vpushsym(type, sym); > + vpushsym(type, sym); > } > /* patch symbol weakness */ > if (type->t & VT_WEAK) > weaken_symbol(sym); > - apply_visibility(sym, type); > + apply_visibility(sym, type); > #ifdef CONFIG_TCC_BCHECK > /* handles bounds now because the symbol must be defined > before for the relocation */ Spurious change, remove them. > @@ -6027,10 +6032,10 @@ static int decl0(int l, int is_for_loop_init) > if (sym->type.t & VT_STATIC) > type.t = (type.t & ~VT_EXTERN) | VT_STATIC; > > - /* If the definition has no visibility use the > - one from prototype. */ > - if (! (type.t & VT_VIS_MASK)) > - type.t |= sym->type.t & VT_VIS_MASK; > + /* If the definition has no visibility use the > + one from prototype. */ > + if (! (type.t & VT_VIS_MASK)) > + type.t |= sym->type.t & VT_VIS_MASK; > > if (!is_compatible_types(&sym->type, &type)) { > func_error1: Likewise. Jiang, you need to check your patch before sending them. I don't expect a perfect patch but at least the style should be ok (you forgot lots of spaces here and there) and there should not be whitespace only changes. Also I don't want to see any trailing whitespace. > diff --git a/tests/tests2/03_struct.c b/tests/tests2/03_struct.c > index c5d48c5..3b99e30 100644 > --- a/tests/tests2/03_struct.c > +++ b/tests/tests2/03_struct.c > @@ -1,5 +1,18 @@ > #include <stdio.h> > > +truct buffered_lines_head { > + int *slh_first; > +}; > + > +static int eval_rept() > +{ > + /* Test locally defined */ > + struct buffered_lines_head { > + int *slh_first; > + } lines; > + return 0; > +} > + I'm pretty sure you took this code from yasm without rewriting anything. If the license of yasm is the same of tinycc (MIT) that's ok but otherwise it'd be better if you can change the names. Eval_rept could be just fct. > struct fred > { > int boris; > @@ -8,24 +21,27 @@ struct fred > > int main() > { > - struct fred bloggs; > + struct fred bloggs; > + struct buffered_lines_head tst; > + /* Test locally defined */ > + tst.slh_first = 0; > > - bloggs.boris = 12; > - bloggs.natasha = 34; > + bloggs.boris = 12; > + bloggs.natasha = 34; > > - printf("%d\n", bloggs.boris); > - printf("%d\n", bloggs.natasha); > + printf("%d\n", bloggs.boris); > + printf("%d\n", bloggs.natasha); > > - struct fred jones[2]; > - jones[0].boris = 12; > - jones[0].natasha = 34; > - jones[1].boris = 56; > - jones[1].natasha = 78; > + struct fred jones[2]; > + jones[0].boris = 12; > + jones[0].natasha = 34; > + jones[1].boris = 56; > + jones[1].natasha = 78; > > - printf("%d\n", jones[0].boris); > - printf("%d\n", jones[0].natasha); > - printf("%d\n", jones[1].boris); > - printf("%d\n", jones[1].natasha); > + printf("%d\n", jones[0].boris); > + printf("%d\n", jones[0].natasha); > + printf("%d\n", jones[1].boris); > + printf("%d\n", jones[1].natasha); > > return 0; > } > > fixsym.patch > Make the following code to compile > int foo(); > int main() > { > int foo(); > return 0; > } I didn't understand your private email that followed this one. Did you discover a bug in this patch that you want to fix before sending it again? > > fixbitfields.patch > Nothing bug Ok, I'll take a look at this one later this week. > > Patch above, I request to join mob No need to ask for this, I assume that if you post code here you want it commited to mob. By the way, can you post your patch inline as git format- patch would do? This way I can comment directly in the reply otherwise I need to copy/paste and it's difficult to distinguish the code from my comment or use sed to add the "> " prefix to each line. > > pass test: gnumake yasm > I like the fact that you say on what you tested your patch. Good idea, please continue. Best regards, Thomas
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ Tinycc-devel mailing list Tinycc-devel@nongnu.org https://lists.nongnu.org/mailman/listinfo/tinycc-devel