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

Position [Lprologue] correctly #2292

Merged
merged 3 commits into from Mar 29, 2019

Conversation

@mshinwell
Copy link
Contributor

commented Mar 6, 2019

It is necessary that the Lprologue instruction in linearized code is positioned correctly with respect to the Iname_for_debugger pseudooperation that assigns variable names to registers. If this does not happen, then the user may not see values of variables that are expected to be seen (function parameters in particular...), when stepping amongst the prologue. This produces a poor user experience.

This patch ensures that the prologue instruction goes in the correct place. It also elides the Lprologue instruction entirely when it is going to expand to nothing. The reason for this is documented in the code. The comment refers to a pass that has not yet been presented upstream, Coalesce_labels, but all that is necessary to know about that pass is the following: it ensures that given two labels in the linearized code that do not compare equal, those labels do not point at the same code address.

The reason for moving the creation of the tail recursion label into Linearize (apart from the fact that it seems better to represent such things explicitly in intermediate languages) is because otherwise, if we elide the Lprologue instruction, we would fail to define the label.

@lthls Are you qualified to review this? For the prologue_required calculations, the thing to look at is what the Lprologue clause does in the instruction emitter, and observe that the stack_offset variables will always be zero at that point.

@lthls

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2019

I'll review the implementation.

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

(We should send this to precheck before merging.)

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

One thing to note is that this dovetails nicely with #2090, which moves some stack-related calculations from the emitters into Proc. The approach in #2090 has been shown to work well for the gdb work, and I am planning to complete that GPR after this one has been dealt with.

@lthls

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

I noticed that you assume that if Clflags.gprofile is set to true, a prologue is always emitted (with the exception of s390x). As far as I can tell, a number of architectures may not output any profiling instructions even when profiling is on, depending on the value of Config.system. For instance, no profiling is emitted on Windows.
Unless there is some logic to ensure that Clflags.gprofile cannot be set on systems that do not support it, you risk having empty prologues despite prologue_required returning true in some corner cases.

The rest of the code looks correct.

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

Thanks for the review. I have started a discussion on the core developers' list raising the possibility that maybe we could remove the gprof support at this stage.

@mshinwell mshinwell referenced this pull request Mar 12, 2019

@mshinwell mshinwell force-pushed the mshinwell:linearize_prologue branch from d9ce337 to 697679d Mar 18, 2019

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

@lthls I've updated this to deal with the removal of gprof support.

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

I have asked @dra27 to restart AppVeyor on this one.

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

I don't know what's going on with Appveyor, so I've sent this to precheck (#226). It's probably a good idea anyway to precheck this one.

@mshinwell mshinwell force-pushed the mshinwell:linearize_prologue branch from 697679d to b1e3b41 Mar 27, 2019

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2019

@lthls Please approve, I have rebased this. There seems to be something wrong with precheck at the moment unfortunately -- I am going to ask if that can be fixed, then try again with this one.

@lthls

lthls approved these changes Mar 27, 2019

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

The issues with precheck are apparently ongoing. I've looked again at precheck#226 for this one and I think the failures were probably CI issues, so I'm going to merge this now. I will keep an eye on the CI.

@mshinwell mshinwell merged commit 4334b2d into ocaml:trunk Mar 29, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.