Buggy commit "Rework expr_landor"

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Buggy commit "Rework expr_landor"

Christian Jullien-3

Hi Michael,

 

Your last commit breaks something in my bignum library used by OpenLisp.

This library does a lot of logical and bit operations on integers.

Only this last commit generates a core dump as tested on Linux x64 (tcc compiled by gcc 9.2.1) and Windows x64 (tcc compiled by boostrapped tcc).

 

It is hard to currently investigate where it exactly happens.

 

Christian.

 

 


_______________________________________________
Tinycc-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/tinycc-devel
Reply | Threaded
Open this post in threaded view
|

Re: Buggy commit "Rework expr_landor"

grischka
Christian Jullien wrote:

> Hi Michael,
>
> Your last commit breaks something in my bignum library used by OpenLisp.
>
> This library does a lot of logical and bit operations on integers.
>
> Only this last commit generates a core dump as tested on Linux x64 (tcc
> compiled by gcc 9.2.1) and Windows x64 (tcc compiled by boostrapped tcc).
>
> It is hard to currently investigate where it exactly happens.
Hi, well, here too, exactly what Christian wrote: strange failures,
hard to impossible to say more.  But I found another test that might
give a hint (attached).

About the precedence parser, great to see this, how it can really
work and all.

Still, hope you don't mind, I'd vote against it, for plain ordinary
nostalgic reasons, in the first place:

Basically those small functions making the top-down parser, still
almost unchanged from how Fabrice wrote them,  are a just wonderful
demonstration how simple C is, at its basics.  I figure everybody
seeing this wants to start working on her own compiler immediately.

We can't let that go away, can we?

About expr_landor,  I didn't feel quite happy with how it looks,
either.  Why for example

          if (c < 0)
              t = gvtst(i, t);
          else
              vpop();

and not just

          t = gvtst(i, t);

and let the "automatic code suppression" do it?  (Guess it was
const expressions or something.)

Obviously you tried to move these 'non-obviousnesses' to the
gvtest_set stuff.  I'm not sure that's a good idea.  At least
that part I was still able to understand.  Now, for example,
what means (t = gvtest()) == NULL?

          int t = gvtst(inv, 0);
          if (t) /* in case gvtst only needed to do a gsym */
           ...

Before that was because of nocode_wanted.  And that was the
reason why I did avoid to have any 'if' with it anywhere.

Anywho, there is less stack usage indeed, from compiling tcc.
On the other hand 6 or 7 kB (on i386) aren't that much either.

The patch to make those traces is attached too. Usage is
     $ tcc.exe -bt1000 -d9 tcc.c -o tcc1.exe
     $ tcc1.exe tcc.c -bench -d7 > tcc.log 2>&1

(1) with your patches:
       ../lib/bt-log.c:61: at test_stack: 6256
       ../tccpp.c:2772: parsing at 'next_nomacro1'
       ../tccgen.c:2223: by gen_opic
       ../tccgen.c:2683: by gen_op
       ...
       ../tcc.c:346: by main
       [60 levels]
       code: 220275 bytes, suppressed 327 bytes

(0) before:
       ../lib/bt-log.c:61: at test_stack: 7008
       ../tccpp.c:2772: parsing at 'next_nomacro1'
       ../tccgen.c:2214: by gen_opic
       ../tccgen.c:2674: by gen_op
       ...
       ../tcc.c:346: by main
       [136 levels]
       code: 220284 bytes, suppressed 327 bytes

--- grischka

>
> Christian.


>From 7b627bc4139ea609c5aeecbf8b85f96c3c1bfc3b Mon Sep 17 00:00:00 2001
From: grischka <grischka>
Date: Mon, 20 Jan 2020 08:46:39 +0100
Subject: stack test


diff --git a/i386-gen.c b/i386-gen.c
index 8016af94..cc601443 100644
--- a/i386-gen.c
+++ b/i386-gen.c
@@ -105,8 +105,14 @@ static void gen_bounds_epilog(void);
 ST_FUNC void g(int c)
 {
     int ind1;
-    if (nocode_wanted)
+
+    extern int cc, nc;
+    if (nocode_wanted) {
+        ++nc;
         return;
+    }
+    ++cc;
+
     ind1 = ind + 1;
     if (ind1 > cur_text_section->data_allocated)
         section_realloc(cur_text_section, ind1);
diff --git a/lib/bt-log.c b/lib/bt-log.c
index 73e0067f..87c4b7e2 100644
--- a/lib/bt-log.c
+++ b/lib/bt-log.c
@@ -34,3 +34,31 @@ DLL_EXPORT int tcc_backtrace(const char *fmt, ...)
     }
     return ret;
 }
+
+/* a stack depth test */
+char *main_stack;
+const char *ppfn;
+int *ppln;
+const char **ppfu;
+static unsigned xx;
+
+void test_stack()
+{
+    unsigned x;
+    const char *file, *func;
+    int line, level;
+
+    x = main_stack ? main_stack - (char*)&x : 0;
+    if (x <= xx)
+        return;
+    xx = x;
+    file = ppfn ? ppfn : 0;
+    line = ppln ? *ppln : 0;
+    func = ppfu ? *ppfu : 0;
+    if (func)
+        level = tcc_backtrace("%d\n%s:%d: parsing at '%s'", x, file, line, func);
+    else
+        level = tcc_backtrace("%d (not yet parsing)", x);
+
+    fprintf(stderr, "[%d levels]\n\n", level);
+}
diff --git a/libtcc.c b/libtcc.c
index fdbe2e7f..3bab0a8f 100644
--- a/libtcc.c
+++ b/libtcc.c
@@ -2084,8 +2084,15 @@ LIBTCCAPI void tcc_set_options(TCCState *s, const char *r)
     dynarray_reset(&argv, &argc);
 }

+int cc, nc;
+
 PUB_FUNC void tcc_print_stats(TCCState *s1, unsigned total_time)
 {
+    if (s1->g_debug == 7) {
+        fprintf(stderr, "code: %d bytes, suppressed %d bytes\n", cc, nc);
+        return;
+    }
+
     if (total_time < 1)
         total_time = 1;
     if (total_bytes < 1)
diff --git a/tcc.c b/tcc.c
index 34bfa004..009839e1 100644
--- a/tcc.c
+++ b/tcc.c
@@ -248,6 +248,9 @@ static unsigned getclock_ms(void)
 #endif
 }

+char* main_stack;
+int cc, nc;
+
 int main(int argc0, char **argv0)
 {
     TCCState *s, *s1;
@@ -257,6 +260,8 @@ int main(int argc0, char **argv0)
     int argc; char **argv;
     FILE *ppfp = stdout;

+    main_stack = (char*)&argc0;
+
 redo:
     argc = argc0, argv = argv0;
     s = s1 = tcc_new();
diff --git a/tccgen.c b/tccgen.c
index 047332b8..e4f047a9 100644
--- a/tccgen.c
+++ b/tccgen.c
@@ -7601,6 +7601,15 @@ static void decl_initializer_alloc(CType *type, AttributeDef *ad, int r,
     nocode_wanted = saved_nocode_wanted;
 }

+char *ppfn, *ppln, *ppfu;
+
+static void insert_func(const char *fn)
+{
+    int v = tok_alloc(fn, strlen(fn))->tok;
+    vpush_global_sym(&func_old_type, v);
+    gfunc_call(0);
+}
+
 /* parse a function defined by symbol 'sym' and generate its code in
    'cur_text_section' */
 static void gen_function(Sym *sym)
@@ -7633,7 +7642,15 @@ static void gen_function(Sym *sym)
     local_scope = 0;
     rsym = 0;
     clear_temp_local_var_list();
+
+    ppfn = (char*)&file->filename;
+    ppln = (char*)&file->line_num;
+    ppfu = (char*)&funcname;
+    if (tcc_state->g_debug == 9)
+        insert_func("test_stack");
     block(0);
+    ppfu = NULL;
+
     gsym(rsym);
     nocode_wanted = 0;
     /* reset local stack */
diff --git a/tccrun.c b/tccrun.c
index 4bf709d6..ae8001a0 100644
--- a/tccrun.c
+++ b/tccrun.c
@@ -564,7 +564,7 @@ static int _rt_error(void *fp, void *ip, const char *fmt, va_list ap)
     }

     rc->ip = rc->fp = 0;
-    return 0;
+    return level;
 }

 /* emit a run time error at position 'pc' */
diff --git a/x86_64-gen.c b/x86_64-gen.c
index 44c34ab0..f095fc26 100644
--- a/x86_64-gen.c
+++ b/x86_64-gen.c
@@ -150,8 +150,14 @@ static int func_ret_sub;
 ST_FUNC void g(int c)
 {
     int ind1;
-    if (nocode_wanted)
+
+    extern int cc, nc;
+    if (nocode_wanted) {
+        ++nc;
         return;
+    }
+    ++cc;
+
     ind1 = ind + 1;
     if (ind1 > cur_text_section->data_allocated)
         section_realloc(cur_text_section, ind1);



#include <stdio.h>
#include <string.h>

int main(int argc, char **argv)
{
    char buf[40];
    double a, b, n;
    int i = 0;

#define TEST_1(a,op,b) (a op b), ((a op b) && 1), !(a op b), (!(a op b) && 1)
#define TEST(op, ref) \
    sprintf(buf, "%d%d%d%d%d%d%d%d%d%d%d%d" \
        ,TEST_1(a, op, b) \
        ,TEST_1(b, op, a) \
        ,TEST_1(a, op, n) \
        ); \
    printf("%d: %-2s %s %s\n", ++i, #op, buf, strcmp(buf, ref) ? ref : "ok");

    a = 1, b = 1, n = 0, n = n/n;
    TEST(==, "110011000011");
    TEST(!=, "001100111100");
    a = -1;
    TEST(==, "001100110011");
    TEST(!=, "110011001100");
    TEST(<,  "110000110011");
    TEST(>,  "001111000011");
    return 0;
}



_______________________________________________
Tinycc-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/tinycc-devel
Reply | Threaded
Open this post in threaded view
|

Re: Buggy commit "Rework expr_landor"

grischka
... Found something:

1) in vcheck_cmp():
     if (vtop->r == VT_CMP && !nocode_wanted --> && vtop->cmp_op > 1)
doesn't work, see the test I sent earlier.

2) in expr_landor():
     if (f) {
         vpop();
         vpushi(!i);
         nocode_wanted -= f;
     }
     gvtst_set(i, t);

This doesn't work as is with for example
     c = a && 0 ? 2 : 3;
because expr_cond() would turn off code at its first gvtst()
and never on again.

option 1:
     if (f || condition_3way() == !i) {
         vpop();
         vpushi(!i);
         gsym(t);
         nocode_wanted -= f;
     } else
         gvtst_set(i, t);

This appears to generate byte-identical code to before, with my tests.

option 2:
     moving up 'ncw_prev = nocode_wanted;'.
This appears to work too but generates more code.

It's all somehow brittle, with the mix of ++/-- with |=/&= nocode_wanted.

Sigh,

--- grischka


_______________________________________________
Tinycc-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/tinycc-devel
Reply | Threaded
Open this post in threaded view
|

Re: Buggy commit "Rework expr_landor"

Christian Jullien-3
Hi Grischka,

I can't certify that your change fixes all parsing issues but, at least, it fixes my bignum library and all my huge non-reg tests suite pass again w.o. issue.

A very big thanks

C.

-----Original Message-----
From: Tinycc-devel [mailto:tinycc-devel-bounces+eligis=[hidden email]] On Behalf Of grischka
Sent: Tuesday, January 21, 2020 00:45
To: [hidden email]
Cc: [hidden email]; Michael Matz
Subject: Re: [Tinycc-devel] Buggy commit "Rework expr_landor"

... Found something:

1) in vcheck_cmp():
     if (vtop->r == VT_CMP && !nocode_wanted --> && vtop->cmp_op > 1)
doesn't work, see the test I sent earlier.

2) in expr_landor():
     if (f) {
         vpop();
         vpushi(!i);
         nocode_wanted -= f;
     }
     gvtst_set(i, t);

This doesn't work as is with for example
     c = a && 0 ? 2 : 3;
because expr_cond() would turn off code at its first gvtst()
and never on again.

option 1:
     if (f || condition_3way() == !i) {
         vpop();
         vpushi(!i);
         gsym(t);
         nocode_wanted -= f;
     } else
         gvtst_set(i, t);

This appears to generate byte-identical code to before, with my tests.

option 2:
     moving up 'ncw_prev = nocode_wanted;'.
This appears to work too but generates more code.

It's all somehow brittle, with the mix of ++/-- with |=/&= nocode_wanted.

Sigh,

--- grischka


_______________________________________________
Tinycc-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/tinycc-devel


_______________________________________________
Tinycc-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/tinycc-devel
Reply | Threaded
Open this post in threaded view
|

Re: Buggy commit "Rework expr_landor"

Michael Matz-4
In reply to this post by grischka
Hello,

On Mon, 20 Jan 2020, grischka wrote:

> > Your last commit breaks something in my bignum library used by OpenLisp.
> >
> > This library does a lot of logical and bit operations on integers.
> >
> > Only this last commit generates a core dump as tested on Linux x64 (tcc
> > compiled by gcc 9.2.1) and Windows x64 (tcc compiled by boostrapped tcc).
> >
> > It is hard to currently investigate where it exactly happens.
>
> Hi, well, here too, exactly what Christian wrote: strange failures,
> hard to impossible to say more.
Hrmpf, I've used that patch since some weeks and never hit that problem
:-/  Obviously not enough testing coverage.

> But I found another test that might give a hint (attached).

Indeed, and thanks for fixing it ;-)

> About the precedence parser, great to see this, how it can really work
> and all.
>
> Still, hope you don't mind, I'd vote against it, for plain ordinary
> nostalgic reasons, in the first place:
>
> Basically those small functions making the top-down parser, still
> almost unchanged from how Fabrice wrote them,  are a just wonderful
> demonstration how simple C is, at its basics.

Actually, what Fabrice originally wrote is a sort-of precedence parser
written in the slowest imaginable way (see commit a9f08655), though I
found out only later :-)

In principle I agree with you on the nostalgia part, though I find other
places in TCC more warty and unelegant than the precedence expression
parser (_actually_ I find it fairly elegant, even :) ).

My motivation for writing it in the first place were two-fold: to reduce
recursion depth, the precedence parser simply is faster and less deep; and
second to save code lines, I have some little code optimizer that needs
250 lines or so, and I thought I'd trade that with lines of code I remove
with other patches ;-)

> I figure everybody seeing this wants to start working on her own
> compiler immediately.
>
> We can't let that go away, can we?
>
> About expr_landor,  I didn't feel quite happy with how it looks,
> either.

Don't tell me.  I spent literally _days_ on making expr_landor
somewhat more "natural" and failed.  Multiple times, I went back and forth
with the gvtst helpers, either leaving things on the stack (a label
reference) plus a gen_op helper that would combine them, or what's there
now.  I thought that my expr_landor variant was a bit nicer, but obviously
it also was wronger ;)

> Why for example
>
>          if (c < 0)
>              t = gvtst(i, t);
>          else
>              vpop();
>
> and not just
>
>          t = gvtst(i, t);
>
> and let the "automatic code suppression" do it?  (Guess it was
> const expressions or something.)
This is actually because gjmp() (used by gvtst) unconditionally does a
CODE_OFF, but gsym(t) only does a CODE_ON when if(t).  The attached
patch (on top of mob) works, but it introduces an explicit check on
nocode_wanted.  That could be replaced by

  static int gjmp_acs(int t) { t = gjmp(t); if (t) CODE_OFF(); return t; }

But that doesn't catch the case when a jump chain already was established
and we are now newly in a nocode_wanted region, that would need to check
t_after_gjmp != t_before_gjmp, but that's exactly equivalent to a
nocode_wanted check again.

> Obviously you tried to move these 'non-obviousnesses' to the
> gvtest_set stuff.  I'm not sure that's a good idea.  At least
> that part I was still able to understand.  Now, for example,
> what means (t = gvtest()) == NULL?
>
>          int t = gvtst(inv, 0);
>          if (t) /* in case gvtst only needed to do a gsym */
>   ...
>
> Before that was because of nocode_wanted.  And that was the
> reason why I did avoid to have any 'if' with it anywhere.
Yeah, so the above was not for correctness, but for generating the 100%
same code as without the expr_landor changes, nothing directly to do with
nocode_wanted (though the changed code happened on the border of
nocode_wanted changes, and only with my changes to gvtst+friends).  The
thing is, gvtst doesn't always emit a jump (when the inverse setting and
the jtrue/jfalse entries are right it only does a gsym(foo)), but the
unconditional vseti(VT_JMP+inv, ...) makes it look to the compiler as if a
jump was emitted that needs to be resolved when evaluating for value.  
The individual backends could of course handle the case when .c.i is zero
for VT_JMP[I], but I thought why bother, the generic code could just as
well handle that :)

> Anywho, there is less stack usage indeed, from compiling tcc.
> On the other hand 6 or 7 kB (on i386) aren't that much either.
>
> The patch to make those traces is attached too. Usage is
>     $ tcc.exe -bt1000 -d9 tcc.c -o tcc1.exe
>     $ tcc1.exe tcc.c -bench -d7 > tcc.log 2>&1

Ah, cool, thanks, I was already curious how you measured that :)


Ciao,
Michael.
_______________________________________________
Tinycc-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/tinycc-devel

tcc-landor-bla.diff (1K) Download Attachment