Skip to content

Detection from C as to whether our caller was OCaml#662

Closed
mshinwell wants to merge 1 commit intoocaml:trunkfrom
mshinwell:directly_called_from_ocaml
Closed

Detection from C as to whether our caller was OCaml#662
mshinwell wants to merge 1 commit intoocaml:trunkfrom
mshinwell:directly_called_from_ocaml

Conversation

@mshinwell
Copy link
Copy Markdown
Contributor

This patch adds a macro that can be used in C functions to determine whether they were directly called from OCaml code. This is needed for Spacetime instrumentation.

I am pretty sure this is correct for Unix, but I am unsure about Windows. @alainfrisch : can you comment? Should we be using the gcc builtin for SYS_cygwin?

@mshinwell mshinwell added this to the 4.04 milestone Jul 6, 2016
@lefessan
Copy link
Copy Markdown
Contributor

lefessan commented Jul 6, 2016

Wouldn't it be better to make it explicit it relies on caml_last_return_address, i.e. use DIRECTLY_CALLED_FROM_OCAML(caml_last_return_address), or at least, it should be something close to a function call (DIRECTLY_CALLED_FROM_OCAML()) ?

@mshinwell
Copy link
Copy Markdown
Contributor Author

I don't see why. (You could imagine a version of this that doesn't use that variable, for example.)

@mshinwell
Copy link
Copy Markdown
Contributor Author

Also, there's a good reason why this should not be thought of as a function call: it must be fully independent of any inlining decisions made by the C compiler.

@lefessan
Copy link
Copy Markdown
Contributor

lefessan commented Jul 6, 2016

I am not saying it should be a function, I say it should "look like a function" (so, the parens after the macro), so that the programmer knows it is dynamically determined (by opposition to macros that are statically determined).

@xavierleroy
Copy link
Copy Markdown
Contributor

Not only this macro is not portable across C compilers, but it is wrong for arm, arm64, powerpc, s390x, and sparc. (Hint: in these architectures, the C function returns to glue code in caml_c_call, which then returns to the OCaml code at address caml_last_return_address.)

Could you please rethink the need for such a test? It's going to be a portability nightmare.

@mshinwell
Copy link
Copy Markdown
Contributor Author

This is only needed as an optimisation. Let me propose another means by which it could be accomplished in a different PR.

@mshinwell
Copy link
Copy Markdown
Contributor Author

Please see #664

@mshinwell mshinwell closed this Jul 6, 2016
lukemaurer pushed a commit to lukemaurer/ocaml that referenced this pull request Jul 19, 2022
stedolan added a commit to stedolan/ocaml that referenced this pull request Sep 21, 2022
stedolan pushed a commit to stedolan/ocaml that referenced this pull request Sep 21, 2022
ce76e02 flambda-backend: Bugfix for type_application (ocaml#746)
44f3afb flambda-backend: PR580 for main branch (ocaml#743)
b851eaa flambda-backend: Backport first part of ocaml/ocaml PR10498 (ocaml#737)
fafb4bd flambda-backend: Fix return mode for eta-expanded function in type_argument (ocaml#735)
c31f6c3 flambda-backend: Fix treatment of functions called [@nontail] (ocaml#725)
847781e flambda-backend: Fix build_upstream post-PR703 (ocaml#712)
bfcbbf8 flambda-backend: Extend Pblock value kind to handle variants (ocaml#703)
b2cab95 flambda-backend: Merge ocaml-jst
a6d6e0e flambda-backend: Merge ocaml-jst
88a4f63 flambda-backend: Use Pmakearray for immutable arrays (ocaml#699)
eeaa44b flambda-backend: Install an ocamldoc binary (ocaml#695)
48d322b flambda-backend: Ensure that GC is not invoked from bounds check failures (ocaml#681)
4370fa1 flambda-backend: Review changes of term directory (ocaml#602)
65a4566 flambda-backend: Add code coverage using bisect_ppx (ocaml#352)
63ab65f flambda-backend: Bugfix for primitive inclusion (ocaml#662)
7e3e0c8 flambda-backend: Fix inclusion checks for primitives (ocaml#661)
96c68f9 flambda-backend: Speed up linking by changing cmxa format (ocaml#607)
1829150 flambda-backend: Bugfix for Translmod.all_idents (ocaml#659)

git-subtree-dir: ocaml
git-subtree-split: ce76e02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants