Missing renaming during inlining? #7018
Original bug ID: 7018
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.
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'".
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.)