Re: [Tinycc-devel] Re: Please comment this patch, if you have the time.

[Rob Landley]

On Sunday 30 September 2007 2:13:37 pm Jakob Eriksson wrote:
> > Anybody else understand what this is doing?
>
> By the way, this is (almost) the patch Rob was referring to.
>
> regards,
> Jakob

And here's what I told him at the time:

On Tuesday 25 September 2007 3:54:23 pm Jakob Eriksson wrote:
> diff -r a62ad123624a arm-gen.c
> --- a/arm-gen.c Sat Sep 22 04:39:52 2007 -0500
> +++ b/arm-gen.c Tue Sep 25 22:40:44 2007 +0200
> @@ -1695,6 +1695,37 @@ void ggoto(void)
>    vtop--;
>  }
>  
> +int gen_0x850f(int ind)
> +{
> +    o(0x1A000000 | encbranch(ind, 0, 1));
> +
> +    return ind;
> +}

What's 0x850f?  Seems a somewhat non-informative name for a function.

> +int target_len(void)
> +{
> +    return
> +#ifdef TCC_ARM_EABI
> +        8
> +#else
> +        4
> +#endif
> +        ;
> +}

Oh wow that's ugly.  Why have a function for this instead of a global variable 
or a #define?

> +int tcc_define_symbols_platform(TCCState *s)
> +{
> +    tcc_define_symbol(s, "__ARM_ARCH_4__", NULL);
> +    tcc_define_symbol(s, "__arm_elf__", NULL);
> +    tcc_define_symbol(s, "__arm_elf", NULL);
> +    tcc_define_symbol(s, "arm_elf", NULL);
> +    tcc_define_symbol(s, "__arm__", NULL);
> +    tcc_define_symbol(s, "__arm", NULL);
> +    tcc_define_symbol(s, "arm", NULL);
> +    tcc_define_symbol(s, "__APCS_32__", NULL);
> +}

Hmmm, not a bad idea.  Might be better to have a structure we can iterate over 
in a loop instead.  (Are any of these platform symbols ever defined to 
something other than NULL?)

>  /* end of ARM code generator */
>  /*************************************************************/
>  
> diff -r a62ad123624a i386/i386-asm.c
> --- a/i386/i386-asm.c   Sat Sep 22 04:39:52 2007 -0500
> +++ b/i386/i386-asm.c   Tue Sep 25 22:42:02 2007 +0200
> @@ -1207,3 +1207,9 @@ static void asm_clobber(uint8_t *clobber
>      }
>      clobber_regs[reg] = 1;
>  }
> +
> +int gen_0x850f(int ind)
> +{
> +    return psym(0x850f, 0);
> +}

Again, is there a better name for this?

> diff -r a62ad123624a i386/i386-gen.c
> --- a/i386/i386-gen.c   Sat Sep 22 04:39:52 2007 -0500
> +++ b/i386/i386-gen.c   Tue Sep 25 22:42:09 2007 +0200
> @@ -1018,6 +1018,24 @@ void gen_bounded_ptr_deref(void)
>          put_extern_sym(sym, NULL, 0, 0);
>      rel->r_info = ELF32_R_INFO(sym->c, ELF32_R_TYPE(rel->r_info));
>  }
> +
> +void inline pop_fp_r (int r)
> +{
> +    if (TREG_ST0 == r) {
> +        o(0xd9dd); /* fstp %st(1) */
> +    }
> +}

*shrug* ok...

> +int target_len(void)
> +{
> +    return 4;
> +}

Could still be a global variable.

> +int tcc_define_symbols_platform(TCCState *s)
> +{
> +    tcc_define_symbol(s, "__i386__", NULL);
> +}
> +
>  #endif

So arm has 8 symbols, i386 has one.  Ok...

>  /* end of X86 code generator */
> diff -r a62ad123624a tcc.c
> --- a/tcc.c     Sat Sep 22 04:39:52 2007 -0500
> +++ b/tcc.c     Tue Sep 25 22:43:29 2007 +0200
> @@ -3756,12 +3756,9 @@ void save_reg(int r)
>                  sv.r = VT_LOCAL | VT_LVAL;
>                  sv.c.ul = loc;
>                  store(r, &sv);
> -#ifdef TCC_TARGET_I386
> -                /* x86 specific: need to pop fp register ST0 if saved */
> -                if (r == TREG_ST0) {
> -                    o(0xd9dd); /* fstp %st(1) */
> -                }
> -#endif
> +
> +                pop_fp_r (r);
> +

I didn't see a stub for pop_fp_r() in the arm patch, nor does this call seem 
to be inside the bounds checker only...

>                  /* special long long case */
>                  if ((type->t & VT_BTYPE) == VT_LLONG) {
>                      sv.c.ul += 4;
> @@ -4197,12 +4194,9 @@ void vpop(void)
>  {
>      int v;
>      v = vtop->r & VT_VALMASK;
> -#ifdef TCC_TARGET_I386
> -    /* for x86, we need to pop the FP stack */
> -    if (v == TREG_ST0 && !nocode_wanted) {
> -        o(0xd9dd); /* fstp %st(1) */
> -    } else
> -#endif
> +    if (!nocode_wanted) {
> +        pop_fp_r (v);
> +    }  

You dropped an "else".

Also, the other pop_fp_r call wasn't nocode_wanted?  (Not a complaint, I see 
that you're leaving the current context alone.  I'm trying to figure out 
where the nocode_wanted stuff kicks in, actually.  There's declaration of 
symbols, there's evaluation of constants, and there's generation of code.  
Possibly some of this could be reorganized at a higher level...)

>      if (v == VT_JMP || v == VT_JMPI) {
>          /* need to put correct jump if && or || without test */
>          gsym(vtop->c.ul);
> @@ -4449,16 +4443,7 @@ void gen_opl(int op)
>              if (a == 0) {
>                  b = gtst(0, 0);
>              } else {
> -#if defined(TCC_TARGET_I386)
> -                b = psym(0x850f, 0);
> -#elif defined(TCC_TARGET_ARM)
> -                b = ind;
> -                o(0x1A000000 | encbranch(ind, 0, 1));
> -#elif defined(TCC_TARGET_C67)
> -                error("not implemented");
> -#else
> -#error not supported
> -#endif
> +                b = gen_0x850f (ind);

I take it c67 won't build until it gets a stub for this?

>              }
>          }
>          /* compare low. Always unsigned */
> @@ -5141,17 +5126,7 @@ static int type_size(CType *type, int *a
>          *a = LDOUBLE_ALIGN;
>          return LDOUBLE_SIZE;
>      } else if (bt == VT_DOUBLE || bt == VT_LLONG) {
> -#ifdef TCC_TARGET_I386
> -        *a = 4;
> -#elif defined(TCC_TARGET_ARM)
> -#ifdef TCC_ARM_EABI
> -        *a = 8;
> -#else
> -        *a = 4;
> -#endif
> -#else
> -        *a = 8;
> -#endif
> +        *a = target_len ();

Ok, so the name of this function should actually be double_align and it can 
still be a variable or a #define, right?  (Variable if you don't want to add 
a 386-asm.h file.)

>          return 8;
>      } else if (bt == VT_INT || bt == VT_ENUM || bt == VT_FLOAT) {
>          *a = 4;
> @@ -8842,19 +8817,7 @@ TCCState *tcc_new(void)
>      /* standard defines */
>      tcc_define_symbol(s, "__STDC__", NULL);
>      tcc_define_symbol(s, "__STDC_VERSION__", "199901L");
> -#if defined(TCC_TARGET_I386)
> -    tcc_define_symbol(s, "__i386__", NULL);
> -#endif
> -#if defined(TCC_TARGET_ARM)
> -    tcc_define_symbol(s, "__ARM_ARCH_4__", NULL);
> -    tcc_define_symbol(s, "__arm_elf__", NULL);
> -    tcc_define_symbol(s, "__arm_elf", NULL);
> -    tcc_define_symbol(s, "arm_elf", NULL);
> -    tcc_define_symbol(s, "__arm__", NULL);
> -    tcc_define_symbol(s, "__arm", NULL);
> -    tcc_define_symbol(s, "arm", NULL);
> -    tcc_define_symbol(s, "__APCS_32__", NULL);
> -#endif
> +    tcc_define_symbols_platform(s);
>  #if defined(linux)
>      tcc_define_symbol(s, "__linux__", NULL);
>      tcc_define_symbol(s, "linux", NULL);
> @@ -9842,3 +9805,9 @@ the_end:
>  }

I suspect that whole mess could be turned into a for loop over a structure, 
but that might be a separate cleanup.

>  #endif
> +/**
> + * Local Variables:
> + *  indent-tabs-mode: nil
> + *  tab-width: 4
> + * End:
> + */

Please don't put emacs stuff at the end of the file.

This looks pretty close to mergeable...

Rob
-- 
"One of my most productive days was throwing away 1000 lines of code."
  - Ken Thompson.