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

MPR#7423: `void caml_{failwith,invalid_argument}_value(value msg)` #946

Merged
merged 2 commits into from Dec 29, 2016

Conversation

Projects
None yet
2 participants
@gasche
Member

gasche commented Dec 3, 2016

Fixes MPR#7423:

caml_invalid_argument(str) is noreturn and does not free its
argument. So calling it with a string constructed dynamically means that it will never get freed.

@@ -121,11 +121,21 @@ void caml_failwith (char const *msg)
caml_raise_with_string((value) caml_exn_Failure, msg);
}
void caml_failwith_value (value msg)

This comment has been minimized.

@gasche

gasche Dec 3, 2016

Member

It looks fishy to me that an OCaml-value-taking primitive would have no CAMLfoo ceremony, but I followed the style of the existing void caml_raise_sys_error(value msg) declaration a few lines below:

 void caml_raise_end_of_file(void)
 {
   caml_raise_constant((value) caml_exn_End_of_file);
}
@gasche

gasche Dec 3, 2016

Member

It looks fishy to me that an OCaml-value-taking primitive would have no CAMLfoo ceremony, but I followed the style of the existing void caml_raise_sys_error(value msg) declaration a few lines below:

 void caml_raise_end_of_file(void)
 {
   caml_raise_constant((value) caml_exn_End_of_file);
}

This comment has been minimized.

@mshinwell

mshinwell Dec 27, 2016

Contributor

I don't think any CAML* annotation is required. As far as I know, CAMLexport is now redundant, and CAMLprim is only used to mark primitive functions called from OCaml for bytecode compilation. The function here is just a "normal" function to be called from C.

@mshinwell

mshinwell Dec 27, 2016

Contributor

I don't think any CAML* annotation is required. As far as I know, CAMLexport is now redundant, and CAMLprim is only used to mark primitive functions called from OCaml for bytecode compilation. The function here is just a "normal" function to be called from C.

@@ -121,11 +121,21 @@ void caml_failwith (char const *msg)
caml_raise_with_string((value) caml_exn_Failure, msg);
}
void caml_failwith_value (value msg)

This comment has been minimized.

@mshinwell

mshinwell Dec 27, 2016

Contributor

I don't think any CAML* annotation is required. As far as I know, CAMLexport is now redundant, and CAMLprim is only used to mark primitive functions called from OCaml for bytecode compilation. The function here is just a "normal" function to be called from C.

@mshinwell

mshinwell Dec 27, 2016

Contributor

I don't think any CAML* annotation is required. As far as I know, CAMLexport is now redundant, and CAMLprim is only used to mark primitive functions called from OCaml for bytecode compilation. The function here is just a "normal" function to be called from C.

Show outdated Hide outdated byterun/fail.c
CAMLexport void caml_failwith_value (value msg)
{
CAMLparam1(msg);
caml_raise_with_arg(caml_get_failwith_tag(String_val(msg)), msg);

This comment has been minimized.

@mshinwell

mshinwell Dec 27, 2016

Contributor

I recommend removing CAMLparam1 (and finding something else to say "no return"). It gives a false sense of security. At the moment the presence of this declaration implies that it's guarding against the risk of a GC, which would presumably happen inside caml_get_failwith_tag, if it were changed in the future. However if you expect a side effect from that function, then the code is implicitly relying on the order of evaluation of the arguments of caml_raise_with_arg, which I think is undefined behaviour. (If the second argument was evaluated first and that function did perform a GC then it's wrong.)

@mshinwell

mshinwell Dec 27, 2016

Contributor

I recommend removing CAMLparam1 (and finding something else to say "no return"). It gives a false sense of security. At the moment the presence of this declaration implies that it's guarding against the risk of a GC, which would presumably happen inside caml_get_failwith_tag, if it were changed in the future. However if you expect a side effect from that function, then the code is implicitly relying on the order of evaluation of the arguments of caml_raise_with_arg, which I think is undefined behaviour. (If the second argument was evaluated first and that function did perform a GC then it's wrong.)

This comment has been minimized.

@mshinwell

mshinwell Dec 27, 2016

Contributor

(Alternatively add a binding for the tag, then call caml_raise_with_arg.)

@mshinwell

mshinwell Dec 27, 2016

Contributor

(Alternatively add a binding for the tag, then call caml_raise_with_arg.)

Show outdated Hide outdated byterun/fail.c
CAMLexport void caml_invalid_argument_value (value msg)
{
CAMLparam1(msg);
caml_raise_with_arg(caml_get_invalid_argument_tag(String_val(msg)), msg);

This comment has been minimized.

@mshinwell

mshinwell Dec 27, 2016

Contributor

Same comment as above.

@mshinwell

mshinwell Dec 27, 2016

Contributor

Same comment as above.

@@ -0,0 +1,11 @@
external failwith_from_ocaml : string -> 'a = "caml_failwith_value"

This comment has been minimized.

@mshinwell

mshinwell Dec 27, 2016

Contributor

This is maybe a bit naughty, because I don't think that function is supposed to be called from OCaml. However I imagine it's probably ok in a test case.

@mshinwell

mshinwell Dec 27, 2016

Contributor

This is maybe a bit naughty, because I don't think that function is supposed to be called from OCaml. However I imagine it's probably ok in a test case.

This comment has been minimized.

@gasche

gasche Dec 28, 2016

Member

Yeah, I think it is fine.

@gasche

gasche Dec 28, 2016

Member

Yeah, I think it is fine.

Show outdated Hide outdated testsuite/tests/runtime-C-exceptions/stub_test.c
return strdup("bar");
}
CAMLprim value caml_dynamic_invalid_argument(value unit)

This comment has been minimized.

@mshinwell

mshinwell Dec 27, 2016

Contributor

I would remove the "caml_" name, since this function isn't part of the runtime.

@mshinwell

mshinwell Dec 27, 2016

Contributor

I would remove the "caml_" name, since this function isn't part of the runtime.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Dec 28, 2016

Member

There was a non-trivial merge because of the conflicts with #947, but I did it -- and I split the change in two commits to make it easier to review (the one-commit diff was very confusing). @mshinwell, would you mind having a second look to check that it is as you expected?

Member

gasche commented Dec 28, 2016

There was a non-trivial merge because of the conflicts with #947, but I did it -- and I split the change in two commits to make it easier to review (the one-commit diff was very confusing). @mshinwell, would you mind having a second look to check that it is as you expected?

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Dec 29, 2016

Contributor

All OK.

Contributor

mshinwell commented Dec 29, 2016

All OK.

@mshinwell mshinwell added the approved label Dec 29, 2016

@gasche gasche merged commit bb0ed08 into ocaml:trunk Dec 29, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Dec 29, 2016

Member

Thanks, merged.

Member

gasche commented Dec 29, 2016

Thanks, merged.

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

Merge pull request #946 from gasche/raise_value
MPR#7423: `void caml_{failwith,invalid_argument}_value(value msg)`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment