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

Add Lprologue #2055

Merged
merged 4 commits into from Sep 24, 2018

Conversation

Projects
None yet
3 participants
@mshinwell
Copy link
Contributor

commented Sep 21, 2018

The forthcoming gdb support needs to be able to insert items in the linearised instruction stream before the function prologue. This patch, which consists almost entirely of code moves and should be a no-op, adds a new Lprologue instruction which causes emission of the prologue. This enables other instructions (which will only be used for debugging information emission) to be placed before it. The size calculations for the prologue on ARM, AArch64 and POWER platforms are probably not strictly required but have been included for completeness.

@mshinwell mshinwell added this to the 4.08 milestone Sep 21, 2018

@mshinwell mshinwell requested a review from chambart Sep 21, 2018

@chambart
Copy link
Contributor

left a comment

This patch should effectively be a noop.

What do you consider adding before the prologue ? Isn't that better handled by adding some more field to fundecl ?

@@ -419,11 +419,16 @@ module BR = Branch_relaxation.Make (struct

let offset_pc_at_branch = 0

let prologue_size () =
(if frame_size () > 0 then 2 else 0)

This comment has been minimized.

Copy link
@chambart

chambart Sep 21, 2018

Contributor

This is a slight over-approximation, but this shouldn't matter

This comment has been minimized.

Copy link
@mshinwell

mshinwell Sep 21, 2018

Author Contributor

Indeed -- I followed the existing size calculation code though, which uses this value.

@@ -911,21 +926,6 @@ let fundecl fundecl =
D.label (emit_symbol fundecl.fun_name);
emit_debug_info fundecl.fun_dbg;
cfi_startproc ();

This comment has been minimized.

Copy link
@chambart

chambart Sep 21, 2018

Contributor

Wouldn't it make sense for cfi_startproc to be part of what is emitted by Lprelude. Of course this depends on what kind of things you consider adding before the prelude.

This comment has been minimized.

Copy link
@mshinwell

mshinwell Sep 21, 2018

Author Contributor

I will check this.

This comment has been minimized.

Copy link
@mshinwell

mshinwell Sep 24, 2018

Author Contributor

I think this is ok as is, having thought about it this morning. It matches up nicely with cfi_endproc at the moment; and it also seems in some sense correct that it's right at the top in the assembly output.

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Sep 21, 2018

As far as I recall the reason for needing this relates to the fact that the DWARF available range information is emitted based on labels in the linearised code (i.e. values of type Linearize.t). The address of the earliest possible such label will be after the prologue if you don't have this change. The consequence is that when you arrive at the top of a function in the debugger, you won't see your parameters...

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2018

@chambart has approved and all comments addressed without needing code changes, so merging.

@mshinwell mshinwell merged commit 2a072d8 into ocaml:trunk Sep 24, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

damiendoligez added a commit to damiendoligez/ocaml that referenced this pull request Nov 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.