Re: [Tinycc-devel] tcc grammar problems

[Thomas Preud'homme]

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 <address@hidden>
> 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.