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
Fix #8769 -- "index out of bounds" when compiling packed module #8770
Conversation
I'll add the failing code as a test tomorrow |
There is another suspicious usage of (Maybe we should add to |
bytecomp/translmod.ml
Outdated
@@ -91,7 +91,10 @@ let rec apply_coercion loc strict restr arg = | |||
(fun _ -> apply_coercion loc Alias cc lam) | |||
|
|||
and apply_coercion_field loc get_field (pos, cc) = | |||
apply_coercion loc Alias cc (get_field pos) | |||
if pos >= 0 then | |||
apply_coercion loc Alias cc (get_field pos) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to add assert (pos >= 0)
in the definition of get_field
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've now moved the check itself into get_field
which is slightly cleaner and more consistent with the other get_field
function that appears in this file.
Here is a reference to the original PR that revealed the bug: #1610. I find this part of the codebase confusing to read, and maybe a few comments would help. Some questions below. I understand that some module components are "erased" and others are "runtime". Only runtime components have a valid position. The One would thus expect the runtime representation of modules to be a block whose size is the number of runtime components. This is what the definitions of Is the intent that erased components still get a slot in the runtime value, but their value is just |
In the |
The So a The issue comes from the coercions Since #1610 the use of |
Yes, I agree with the reasoning of having a cleaner refactor in Looking at the code more: instead of changing With your version, I cannot convince myself that |
I had the same observation and pushed the same change just a few of minutes before your comment went up. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to go (but it should have a Changes entry).
I've added #8769 as a regression test. For the changes entry, should I add a new section for 4.08.1 or should I just rebase this over onto the 4.09 branch? Basically, are we going to make a 4.08.1? |
4.08 already has a post-release Changes section, and I think it could go there. (I would copy this section in the Changes file of the 4.09 branch, placed before the 4.09 part.) I don't know whether we will have a 4.08.1. My guess is that we will find out depending on the bugs reported -- this one for example. |
Ah, my PR was against an old version of the 4.08 branch that did not yet have the post-release Changes section. I've rebased it and added a Changes entry. |
I think it would be nice to also credit the bug reporter in the Changes entry.
|
Good point. Done. |
I think you should feel free to merge when the CI is happy again. Can you take care of the cherry-picking as well? (And: thanks for the fix!) |
CI is broken on all platforms:
|
Yeah, cherry-picking accident. Fix incoming. |
When compiling alias coercions we generated invalid
Pfield
primitives. This didn't matter normally because they were always just removed bySimplif
. However,Simplif
was not called when creating a-pack
. This PR stops generating invalidPfield
primitives, and starts callingSimplif
when creating a-pack
.Fixes #8769