Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[patch] Matching.inline_lazy_force need eta expansion #6033

Closed
vicuna opened this issue Jun 5, 2013 · 6 comments
Closed

[patch] Matching.inline_lazy_force need eta expansion #6033

vicuna opened this issue Jun 5, 2013 · 6 comments

Comments

@vicuna
Copy link

vicuna commented Jun 5, 2013

Original bug ID: 6033
Reporter: @chambart
Status: closed (set by @xavierleroy on 2015-12-11T18:19:33Z)
Resolution: fixed
Priority: normal
Severity: major
Version: 4.00.1
Fixed in version: 4.01.0+dev
Category: back end (clambda to assembly)
Monitored by: @gasche

Bug description

The function used by inline_lazy_force depend on options in Clflags setted after the evaluation of the Matching module.

File attachments

@vicuna
Copy link
Author

vicuna commented Jun 5, 2013

Comment author: @alainfrisch

So, one of the branches (presumably the one intended for native code) was never used before?

@vicuna
Copy link
Author

vicuna commented Jun 5, 2013

Comment author: @chambart

Yes, unless the linking order between matching.cmo and optmain.cmo changed in the last 5 years.
It generates awfully slow code for lazy forcing (calling caml_obj_tag)

@vicuna
Copy link
Author

vicuna commented Jun 7, 2013

Comment author: @xavierleroy

Well spotted, thanks. Patch applied in trunk (r13758). Will be in 4.01.

@vicuna
Copy link
Author

vicuna commented Jun 9, 2013

Comment author: @gasche

I just bisected a problem with menhir compilation ("make all" segfaults), and this commit seems guilty. I guess that this unused code path was also untested and therefore faulty.

If you want to reproduce the fault:

wget http://gallium.inria.fr/~fpottier/menhir/menhir-20130116.tar.gz
tar -xzvf menhir-20130116.tar.gz
cd menhir-20130116
make PREFIX=/tmp/

@vicuna
Copy link
Author

vicuna commented Jun 9, 2013

Comment author: @gasche

After some inspection of the menhir compilation process, after this first make the segfault can be reproduced with the minimal command:

cd src/

./menhir.opt --dump --stdlib . fancy-parser.mly

(parser.mly works as well, but an empty grammar doesn't.)

@vicuna
Copy link
Author

vicuna commented Jun 10, 2013

Comment author: @xavierleroy

Indeed, the previously-not-exercised code path "inline_lazy_force_switch" was buggy. Minimal repro case appended to this PR. Quick fix by Luc Maranget and I in commit r13761.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant