Re: [Tinycc-devel] Please test repaired bounds-checking mode, especially on X86_64

[Kirill Smelkov]

On Thu, Nov 15, 2012 at 03:50:28AM +0400, Kirill Smelkov wrote:
> On Wed, Nov 14, 2012 at 04:28:40PM +0100, Michael Matz wrote:
> > Hi,
> > 
> > On Wed, 14 Nov 2012, Kirill Smelkov wrote:
> > 
> > > I've fixed bounds checking mode and now btest passes on i386 and gcc-4.7
> > > for me. The fixup involved touching X86_64 code though, which I have no
> > > hardware to test, and also it would  be good to know whether bcheck now
> > > works for other people, other OSes, etc...
> > 
> > Your changes break compiling bcheck.c with tcc itself, which supports only 
> > calling builtin_frame_address with a zero argument.  bcheck is compiled 
> > with tcc when building the win32 libtcc1 ("make lib/i386-win32/libtcc1.a", 
> > you might have to add a hack to arm-gen.c to get through a compile error 
> > there).
> 
> Oops, sorry about that. I forget about cross-compilation and that tcc
> should be able to compile itself (actually a good critera).
> 
> > In any case, bcheck.o needs to be compilable by gcc and tcc, so your 
> > approach doesn't work.  I'd suggest simply building that file with -O0 
> > when compiled with gcc, then it will generate a frame for all functions.
> 
> Using -O0 for runtime which is a hot point seems to be not a good idea
> to me.
> 
> > Even better would be to change the bounds checker interface by simply 
> > explicitely passing the frame of the caller of 
> > __bound_local_new/__bound_local_delete, instead of relying on rbp/ebp.  
> > I.e. change the call sequence of i386-gen.c.
> 
> That would be more code in all callers, right? And also why use
> additional registers or stack if we already have it in %ebp?
> 
> > Your change for x86-64 would be correct if we would need to care for only 
> > gcc.  But it would also be useless because no target except i386 currently 
> > emits calls to the bounds checking functions.
> 
> I can't say about whether it is useless or not, but how about the
> following patch? With it `make lib/i386-win32/libtcc1.a` compiles fine.
> Is everything ok now?
> 
> Thanks,
> Kirill
> 
> ---- 8< ----
> From: Kirill Smelkov <address@hidden>
> Date: Thu, 15 Nov 2012 03:31:49 +0400
> Subject: [PATCH] Add support for __builtin_frame_address(level)
> 
> Continuing d6072d37 (Add __builtin_frame_address(0)) implement
> __builtin_frame_address for levels greater than zero, in order for
> tinycc to be able to compile its own lib/bcheck.c after
> cffb7af9 (lib/bcheck: Prevent __bound_local_new / __bound_local_delete
> from being miscompiled).
> 
> I'm new to the internals, and used the most simple way to do it.
> Generated code is not very good for levels >= 2, compare
> 
>                 gcc                         tcc
> 
>     level=0     mov    %ebp,%eax            lea    0x0(%ebp),%eax
> 
>     level=1     mov    0x0(%ebp),%eax       mov    0x0(%ebp),%eax
> 
>     level=2     mov    0x0(%ebp),%eax       mov    0x0(%ebp),%eax
>                 mov    (%eax),%eax          mov    %eax,-0x10(%ebp)
>                                             mov    -0x10(%ebp),%eax
>                                             mov    (%eax),%eax
> 
>     level=3     mov    0x0(%ebp),%eax       mov    0x0(%ebp),%eax
>                 mov    (%eax),%eax          mov    (%eax),%ecx
>                 mov    (%eax),%eax          mov    (%ecx),%eax
> 
> But this is still an improvement and for bcheck we need level=1 for
> which the code is good.
> 
> For the tests I had to force gcc use -O0 to not inline the functions.
> And -fno-omit-frame-pointer just in case.
> 
> If someone knows how to improve the generated code - help is
> appreciated.
> 
> Thanks,
> Kirill
> 
> Cc: Michael Matz <address@hidden>
> Cc: Shinichiro Hamaji <address@hidden>
> ---
>  tccgen.c        | 15 +++++++++------
>  tests/Makefile  |  2 +-
>  tests/tcctest.c | 27 +++++++++++++++++++++++++++
>  3 files changed, 37 insertions(+), 7 deletions(-)
> 
> diff --git a/tccgen.c b/tccgen.c
> index f183913..f79da36 100644
> --- a/tccgen.c
> +++ b/tccgen.c
> @@ -3665,20 +3665,23 @@ ST_FUNC void unary(void)
>          break;
>      case TOK_builtin_frame_address:
>          {
> +            int level;
>              CType type;
>              next();
>              skip('(');
> -            if (tok != TOK_CINT) {
> -                tcc_error("__builtin_frame_address only takes integers");
> -            }
> -            if (tokc.i != 0) {
> -                tcc_error("TCC only supports __builtin_frame_address(0)");
> +            if (tok != TOK_CINT || tokc.i < 0) {
> +                tcc_error("__builtin_frame_address only takes positive 
> integers");
>              }
> +            level = tokc.i;
>              next();
>              skip(')');
>              type.t = VT_VOID;
>              mk_pointer(&type);
> -            vset(&type, VT_LOCAL, 0);
> +            vset(&type, VT_LOCAL, 0);       /* local frame */
> +            while (level--) {
> +                mk_pointer(&vtop->type);
> +                indir();                    /* -> parent frame */
> +            }
>          }
>          break;
>  #ifdef TCC_TARGET_X86_64
> diff --git a/tests/Makefile b/tests/Makefile
> index 2e7dd23..af1fdb8 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -73,7 +73,7 @@ libtcc_test$(EXESUF): libtcc_test.c ../$(LIBTCC)
>  # copy only tcclib.h so GCC's stddef and stdarg will be used
>  test.ref: tcctest.c
>       cp ../include/tcclib.h .
> -     $(CC) -o tcctest.gcc $< -I. $(CPPFLAGS) -w $(CFLAGS) $(NATIVE_DEFINES) 
> -std=gnu99 $(LDFLAGS)
> +     $(CC) -o tcctest.gcc $< -I. $(CPPFLAGS) -w $(CFLAGS) $(NATIVE_DEFINES) 
> -std=gnu99 -O0 -fno-omit-frame-pointer $(LDFLAGS)
>       ./tcctest.gcc > $@
>  
>  # auto test
> diff --git a/tests/tcctest.c b/tests/tcctest.c
> index 82762ea..4f9f2a3 100644
> --- a/tests/tcctest.c
> +++ b/tests/tcctest.c
> @@ -89,6 +89,7 @@ void global_data_test(void);
>  void cmp_comparison_test(void);
>  void math_cmp_test(void);
>  void callsave_test(void);
> +void builtin_frame_address_test(void);
>  
>  int fib(int n);
>  void num(int n);
> @@ -598,6 +599,7 @@ int main(int argc, char **argv)
>      cmp_comparison_test();
>      math_cmp_test();
>      callsave_test();
> +    builtin_frame_address_test();
>      return 0; 
>  }
>  
> @@ -2680,3 +2682,28 @@ void callsave_test(void)
>    printf ("%d\n", i);
>  #endif
>  }
> +
> +
> +void bfa3(ptrdiff_t str_offset)
> +{
> +    printf("bfa3: %s\n", (char *)__builtin_frame_address(3) + str_offset);
> +}
> +void bfa2(ptrdiff_t str_offset)
> +{
> +    printf("bfa2: %s\n", (char *)__builtin_frame_address(2) + str_offset);
> +    bfa3(str_offset);
> +}
> +void bfa1(ptrdiff_t str_offset)
> +{
> +    printf("bfa1: %s\n", (char *)__builtin_frame_address(1) + str_offset);
> +    bfa2(str_offset);
> +}
> +
> +void builtin_frame_address_test(void)
> +{
> +    char str[] = "__builtin_frame_address";
> +    char *fp0 = __builtin_frame_address(0);
> +
> +    printf("str: %s\n", str);
> +    bfa1(str-fp0);
> +}

I've pushed the patch to mob.