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

Missing renaming during inlining? #7018

Closed
vicuna opened this issue Oct 13, 2015 · 3 comments
Closed

Missing renaming during inlining? #7018

vicuna opened this issue Oct 13, 2015 · 3 comments
Assignees

Comments

@vicuna
Copy link

vicuna commented Oct 13, 2015

Original bug ID: 7018
Reporter: @alainfrisch
Assigned to: @alainfrisch
Status: closed (set by @xavierleroy on 2017-02-16T14:14:53Z)
Resolution: fixed
Priority: normal
Severity: minor
Category: back end (clambda to assembly)
Monitored by: @diml @jmeber @hcarty

Bug description

The function 'substitute' in closure.ml is responsible for applying alpha renaming to avoid clash of ids. While all other binding forms rename their bound identifiers, this is not the case for Ucatch. I attach a patch that fixes that.

we have observed on a large code base a weird segfault caused by the addition of an unused type in a compilation unit. The segfault is very fragile, and appear/disappear if other useless types are added. Our only explanation is that the problem is related to clashes of unique ids related to inlining, as adding/removing those useless types only affects the unique id generator (the generated assembly code is unchanged modulo changes of these unique ids). The patch seems to fix the problem, although it's hard to be positive that it doesn't just move it around.

File attachments

@vicuna
Copy link
Author

vicuna commented Oct 14, 2015

Comment author: @xavierleroy

That's clearly an oversight in Closure.substitute. I'm not sure the patch is right, though. In "Ucatch(nfail, ids, body, handler)", isn't it the case that "ids" scope over "handler" but not over "body"? In which case the first recursive call to "substitute" should be with the original substitution "sb", and the second with the extended substitution "sb'".

@vicuna
Copy link
Author

vicuna commented Oct 14, 2015

Comment author: @alainfrisch

Thanks Xavier. You're right, of course. I'll commit the patch modified as you suggest. (Strictly speaking, I think the attached patch was still safe, since the identifiers ids cannot be referenced or bound in body -- except if there is another bug elsewhere.)

@vicuna
Copy link
Author

vicuna commented Oct 14, 2015

Comment author: @alainfrisch

Committed to trunk, rev 16499.

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

2 participants