Re: [Chicken-hackers] [PATCH] Add aggressive debugging checks to squash those damn bugs

[Peter Bex]

On Sun, Sep 29, 2013 at 10:46:25PM +0200, Peter Bex wrote:
> The first patch adds assertions to all the type accessors like C_unfix,
> C_block_{header,item}, C_character_code etc.  It does this through a
> "check" macro which contains a horrible, evil hack which allows LET-like
> semantics in C macros, in a bit of a manual way.
> If you'd like more info, let me know and I'll try to clarify.  Also, if
> anyone knows of a way to do this in a less verbose, ugly way, please let
> me know!
> 
> The patch also cleans up a few unused macros and ensures all macros
> nicely use the C_(set)_block_item, C_block_header and C_data_pointer to
> access the C_SCHEME_BLOCK objects.  This cleans up things a bit (and in
> this case, ensures the assertions are triggered when things are wrong).

I ran into a slight setback with this patch: the fx... procedures are all
unsafe and marked as clean (ie, non-mutating), which is correct.
Unfortunately, this means that the optimizer will attempt to
constant-fold them, even in situations like irregex's flag-join.
You see this happen when you compile a new DEBUGBUILD CHICKEN with
itself; it'll crash when compiling irregex.scm.

Here's a simple test program that reproduces the slightly more complex
situation in irregex-core:

(define (flag-join a b)
  (if b (fxior a b) a))

(print (flag-join 1 #f))

This will crash the compiler with a panic due to an assertion failure:
the constant-folder is invoking (fxior 1 #f) because they're all
constants.  Normally, such a bogus thing will raise an exception and
the constant folder will stop, but these procedures are unsafe and
unchecked.  This won't cause an issue in practice because the
constant-folded expression is never reached and may even be eliminated
in a later optimisation step, but the damage is done already.

So to avoid these issues I've decided to apply the checks less strictly
and only on the actual unboxing operations that shouldn't be accessed
straight from Scheme anyway (but it may get hit anyway if I'm
overlooking anything).  Unfortunately, there's still one use of CHECK2
left in the code, and it's the most important one so I had to keep it.

I've also removed the trailing whitespace in patch 0003, which was
already there in library.scm, but git complains when applying it.

So here are all 4 patches again, with 1 and 3 different from last time.

Cheers,
Peter
-- 
http://www.more-magic.net

0001-In-DEBUGBUILDs-add-sanity-assertions-to-most-importa.patch
Description: Text document

0002-Fix-CHICKEN_initialize-s-call-to-C_randomize-which-e.patch
Description: Text document

0003-Fix-several-subtle-problems-in-the-reader-caused-by-.patch
Description: Text document

0004-Fix-read-string-behaviour-after-peeking-at-EOF-add-r.patch
Description: Text document