Re: [PATCH] fix for #1689 (argc check with -O0)

[Peter Bex]

On Tue, Apr 07, 2020 at 12:48:12PM +0200, address@hidden wrote:
> Attached is a patch to address #1689 by moving the argc check for a known
> procedure call into the analysis phase (on first analysis) so that even with 
> -O0
> this check is done, as runtime-checks are disabled for such calls in general.

This patch has revealed two buggy eggs: Spock and sxml-serializer:

https://salmonella-freebsd-x86-64.call-cc.org/master/clang/freebsd/x86-64/2020/04/13/yesterday-diff/log2/install/spock.html
https://salmonella-freebsd-x86-64.call-cc.org/master/clang/freebsd/x86-64/2020/04/13/yesterday-diff/log2/install/sxml-serializer.html

Now, Spock seems to be a very straightforward bug: (usage) is called
without arguments while it has one required argument.

But sxml-serializer is a bit trickier.  sxml-serializer.scm is the
CHICKEN module which includes serializer.scm; a clean, unpatched
version of the upstream code.

The CHICKEN module "patches" it by redefining two procedures,
srl:construct-start-end-tags and srl:name->qname-components.
The former is the only procedure which calls the latter, also
in the original code.

In the new code, the patch takes care of a problem with the context
in which the name must be serialized.  To do so, it adds one extra
argument to srl:name->qname-components, the "attributes?" argument.
This changes its arity.  However, like I said the only place which
calls it is srl:construct-start-end-tags, which is also redefined and
all the call sites now include the extra argument.

Essentially, a minimal test case should be something like:

(define (foo x) (display x) (newline))
(define (bar) (foo 1))

;; Redefine both
(define (foo) (display "hello, world!\n"))
(define (bar) (foo))

(bar)

Unfortunately, this test case doesn't trigger an error because the
procedures are so small they get inlined.

Fixing sxml-serializer should be trivial, so that's not the
issue here.  I'm just wondering if the compiler should allow this or
not.  To me, it seems like something Scheme allows, even if it's
somewhat questionable to do so.

It *looks* like the old versions of both procedures still get compiled.
I'm not sure exactly when the error gets triggered (there's no line
number info), if it's in the old or the new procedure.  But if it's
the old procedure, why is that code even getting compiled if it's
not used?

Cheers,
Peter

signature.asc
Description: PGP signature