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

[Kirill Smelkov]

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);
+}
-- 
1.8.0.337.g4cdca5d