[PATCH] D99393: [OCaml] Fix a possible crash in llvm_struct_name

[Josh Berdine via Phabricator via llvm-commits]

jberdine added inline comments.


================
Comment at: llvm/bindings/ocaml/llvm/llvm_ocaml.c:887
   unsigned Len;
-
-  if ((S = LLVMGetMDString(V, &Len))) {
-    Str = caml_alloc_string(Len);
-    memcpy((char *)String_val(Str), S, Len);
-    Option = alloc(1,0);
-    Store_field(Option, 0, Str);
-    CAMLreturn(Option);
-  }
-  CAMLreturn(Val_int(0));
+  return cstr_to_string_option(LLVMGetMDString(V, &Len), Len);
 }
----------------
vaivaswatha wrote:
> Given that the order of argument evaluation is unspecified in C, if the second argument `Len` to `cstr_to_string_option` is evaluated first (at which point it is uninitialized), then an incorrect value gets passed. 
Gah, yes, of course, thanks!


================
Comment at: llvm/bindings/ocaml/llvm/llvm_ocaml.c:1047
   } else {
-    CAMLreturn(Val_int(0));
+    return Val_int(0);
   }
----------------
vaivaswatha wrote:
> Would it be a good idea to define a macro `#define OCAML_OPTION_NONE Val_Int(0)` to use in such places? I'm not saying we should do it in this patch (if you think it's a good idea to do it at all), but may be in a later one that changes it at all places uniformly.
It might help, but I am also slightly concerned that `Val_int(0)` is the representation of many other OCaml values too, like `[]`. So it would be a leaky abstraction. It seems that choosing to name the macro after `None` is arbitrary, and potentially confusing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99393/new/

https://reviews.llvm.org/D99393