justinethier / cyclone

:cyclone: A brand-new compiler that allows practical application development using R7RS Scheme. We provide modern features and a stable system capable of generating fast native binaries.
http://justinethier.github.io/cyclone/
MIT License
823 stars 42 forks source link

Segmentation Fault #484

Closed amirouche closed 2 years ago

amirouche commented 2 years ago

You can reproduce with the following cli dance:

git clone --branch=amirouche-json-with-cyclone https://github.com/scheme-live/live
cd live
cyclone tests/check-0002-json-unstable.scm 
./tests/check-0002-json-unstable
justinethier commented 2 years ago

The core problem here is and program exporting primitives.

For example, compiling prog.scm and running ./prog will segfault in the same way:

justin@justin-OptiPlex-790 ~/Documents/justinethier-misc/tmp $ cat prog.scm
(import
  (scheme write)
  (lib))

(write (port? 1))
justin@justin-OptiPlex-790 ~/Documents/justinethier-misc/tmp $ cat lib.sld
(define-library (lib)
  (export port? )
  (begin
    (define port? port?)))
justinethier commented 2 years ago

Its also rather unfortunate (define port? port?) is even required to compile the library as built-in primitives are always implicitly available...

justinethier commented 2 years ago

Put in quick fixes to prevent a segmentation fault and to no longer require define when exporting a primitive. These fixes are on the master branch.

With this change check-0002 is no longer segfaulting. However the program seems to terminate after running out of memory. Any thoughts on your end there?

amirouche commented 2 years ago

What we did for other Scheme, so far, is that we disabled tests that were failing, as far as I remember, those were mostly unicode or textual representation that were different (I need to make a second pass on this).

Anyway, with cyclone the situation is different, some test files lead the runner to hang, at least the first lead to a memory leak. Here is the list of test files I need to remove, so that the test runner can finish its work:

    live/json/data/n-object-lone-continuation-byte-in-key-and-trailing-comma/input.json
    live/json/data/n-object-unterminated-value/input.json
    live/json/data/n-string-escaped-backslash-bad/input.json
    live/json/data/n-string-incomplete-escape/input.json
    live/json/data/n-string-single-doublequote/input.json
    live/json/data/n-structure-array-with-unclosed-string/input.json
    live/json/data/n-structure-open-array-open-string/input.json
        live/json/data/n-structure-open-object-open-string/input.json

When those test files are disabled via data-index.scm, the test suite pass green. I added a script the repository called ./local/bin/json2scm.scm so that you can do something like:

cyclone ./local/bin/json2scm.scm
cat live/json/data/n-structure-open-object-open-string/input.json | ./local/bin/json2scm

It seems the same bug for all those files. Do not forget to git pull the branch amirouche-json-with-cyclone.

justinethier commented 2 years ago

Consider the contents of live/json/data/n-structure-open-object-open-string/input.json:

{"a

Maybe I am missing something but how is the parser expected to terminate if the check for EOF is commented out?

https://github.com/scheme-live/live/blob/hello-schemer/live/json/body.scm#L78

amirouche commented 2 years ago

json-read is guarded here:

https://github.com/scheme-live/live/blob/9ff4eae00f18d1aa9ec2228524591ccb838101b2/live/json/body.scm#L273-L274

At this time, there is no precise EOF check, to speed things a little, but I could bring it back, if that is the origin of the problem

justinethier commented 2 years ago

Hmm. Does that work with other Schemes such as Chibi and Chicken? I would think they would run out of memory, too. Or am I missing something?

(import (scheme base) (scheme write))                                            

(define (loop lis)                                                               
  (loop (cons 1 lis)))                                                           

(guard (ex (else (write `(exception ex))))                                       
  (loop 1))  

My preference is for live to check for EOF and handle appropriately.

amirouche commented 2 years ago

I will bring back the eof checks.

amirouche commented 2 years ago

The main branch is called hello-schemer and in that branch there is the json test suite: https://github.com/scheme-live/live/actions?query=branch%3Ahello-schemer

justinethier commented 2 years ago

@amirouche Thanks! Is Cyclone passing all of your tests now?

amirouche commented 2 years ago

After bringing back the eof checks it runs smoothly. I will need to update cyclone inside the docker image used in the CI.

Thanks for the help.

ref: https://github.com/scheme-live/live/pull/57/commits/6cfde5658015b6c6cccff34a0fd36cc1d9848c01