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

Fixes for DWARF CFI when switching between stacks #270

Closed
wants to merge 2 commits into from

Conversation

sadiqj
Copy link
Collaborator

@sadiqj sadiqj commented Dec 27, 2019

No description provided.

@@ -615,15 +615,21 @@ let emit_instr fallthrough i =
end;
cfi_remember_state ();
I.mov (domain_field Domainstate.Domain_c_stack) rsp;
(* FIXME: CFI *)
(* cfi_def_cfa_offset 8; *)
cfi_def_cfa_offset 8;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the offset 8 here? Do you need this? 8 doesn't look right.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since here the address will be determined by the value in memory, I think we should load the value into a register use cfi_def_cfa to set stack pointer to the register value

@kayceesrk
Copy link
Contributor

kayceesrk commented Jan 13, 2020

Can you explain what was the original problem and how the PR fixes it (for posterity)?

And, since we are pushing the OCaml stack pointer information into the C stack, is it necessary to still use rbp at all? Otherwise, can we use rbp, but use other DWARF info to avoid pushing into the stack? I am not comfortable adding 4 memory operations for every fast external call in order to support debugging information. Perhaps this doesn't matter in practice?

@mshinwell
Copy link
Contributor

One general point that it's worth bearing in mind, apologies if this is known already: CFI information isn't just for debugging. It is required to be correct by some language runtimes (Objective-C on macOS is one such) in order to unwind the stack for exception handling.

@kayceesrk
Copy link
Contributor

Agreed. I was a bit sloppy in my use of the term debugging.

@anmolsahoo25
Copy link
Contributor

The following is enough to generate the back-trace correctly -

        I.mov rsp rbp;
        if Config.stats then begin
          I.inc (domain_field Domainstate.Domain_extcall_noalloc);
        end;
        I.mov (domain_field Domainstate.Domain_c_stack) rsp;
        cfi_def_cfa ();
        I.sub (int 8) rsp;
        cfi_def_cfa_offset 8;
        emit_call func;
        I.mov rbp rsp;

Tested it with an external call, seems to work. Also, can remove the space-time related stuff, because we are assuming this function does not alloc.

@kayceesrk
Copy link
Contributor

kayceesrk commented Jan 13, 2020 via email

@sadiqj
Copy link
Collaborator Author

sadiqj commented Jan 13, 2020

This is superseded by the implementation in: #272

@sadiqj sadiqj closed this Jan 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants