Re: [Tinycc-devel] Buggy commit "Rework expr_landor"

[Michael Matz]

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.

tcc-landor-bla.diff
Description: Text Data