[llvm-ocaml] Add binding for constructing opaque pointers

[alan]

[llvm-ocaml] Add binding for llvm::PointerType::isOpaque

[alan]

Hi, I am a first-time contributor to LLVM. I just requested a review from the last two people to make commits to the OCaml LLVM bindings, because I wasn't sure who the code owner was. For context, see https://discourse.llvm.org/t/guidance-for-doing-my-first-contribution/65590/3. Please let me know if I'm doing anything wrong!

[nikic]

I think your update overwrite your previous changes, which added pointer_type_in_context? Now you're just adding pointer_type_is_opaque (but the tests still use the other function as well).

[alan]

In D134916#3826248, @nikic wrote:

I think your update overwrite your previous changes, which added pointer_type_in_context? Now you're just adding pointer_type_is_opaque (but the tests still use the other function as well).

Hi, thanks for taking the time to look at my patch! Unfortunately, I'm new to Phabricator and Arcanist and the documentation confused me. I'm used to the Git workflow of making a branch and committing and pushing changes. I tried to "update" my patch with arc diff --update, but it seems that this didn't do what I wanted. What should I do to "squash" my two commits into one patch? Thank you for your time and patience!

[alan]

[llvm-ocaml] Add binding for constructing opaque pointers

[alan]

I've updated the patch by making a new feature branch and squash merging the old one into the new one. Now, both my commits are included in the diff. Was there a better way for me to have done this?

[nikic]

In D134916#3827035, @alan wrote:

I've updated the patch by making a new feature branch and squash merging the old one into the new one. Now, both my commits are included in the diff. Was there a better way for me to have done this?

Sounds like the right thing to do in this case. Phabricator doesn't do multi-commit reviews, you need one review per commit (though they can be dependent).

llvm/test/Bindings/OCaml/core.ml

47

Hm, does this test really pass? It's not possible to mix opaque and typed pointers in the same context. The second call should be creating an opaque pointer as well.

[aeubanks]

do we even need pointer_type_is_opaque? at this point users should assume that all pointers are opaque

[jberdine]

llvm/bindings/ocaml/llvm/llvm.mli

730

[alan]

This comment was removed by alan.

[alan]

[llvm-ocaml] Add binding for constructing opaque pointers

[alan]

It turns out that @nikic was correct to be fishy by pointing out that a "test" I added should not have passed! I figured out that I added the test function, but I had not added it to the list of tests at the bottom of the file, so it never ran. When I did run it, the test did indeed fail. I've updated the diff to fix this, as well as to add the missing word "space" in a doc comment that was pointed out.

llvm/test/Bindings/OCaml/core.ml

47

It turns out that test should not have passed! It turned out I wasn't actually running the test I added because I neglected to add it to the list of tests at the bottom. When I added it, the test indeed failed. I've updated the patch to fix the test. Your expectation is right; both explicitly created opaque pointer and the pointer created from an i8 type are opaque.

[alan]

[llvm-ocaml] Add binding for constructing opaque pointers

[alan]

Also, as I update this patch using the "make a new branch and squash merge" method, my reviewers' inline comments still appear, but are shown on the "new" version of the patch where I fix the issue, which creates a confusing context. I want to know if I'm doing the Phabricator workflow correctly, or if there is something else I should be doing.

[alan]

[llvm-ocaml] Add binding for constructing opaque pointers

[alan]

I just updated the diff to remove the PointerType::isOpaque binding I added. The only reason I added that was to write a test case checking if a pointer was opaque, which I had *misunderstood* and written incorrectly, as @nikic pointed out. As @aeubanks pointed out, users should assume that all pointers are opaque now (as LLVM is migrating to opaque pointers), so the isOpaque binding perhaps should not be added.

I just want to merge a patch into the main branch to add OCaml bindings for constructing opaque pointers without passing a pointee type. If one were to decide that isOpaque also needs bindings, I think that can be a separate patch after this one is finished.

[alan]

[llvm-ocaml] Add binding for constructing opaque pointers

[alan]

Just rearranged the test order at the bottom to match the order of test definition...

[alan]

Thank you for the approval! I do not believe I have permissions to merge, as I just joined Phabricator to make this patch. According to this: https://www.llvm.org/docs/Phabricator.html#committing-a-change

Once a patch has been reviewed and approved on Phabricator it can then be committed to trunk. If you do not have commit access, someone has to commit the change for you (with attribution). It is sufficient to add a comment to the approved review indicating you cannot commit the patch yourself.

So I assume someone with the right permissions is going to merge this?

Also, will this appear in a minor or patch release of LLVM 15, which adds opaque pointers, or will this only appear in the next major release?

[aeubanks]

In D134916#3829843, @alan wrote:

Thank you for the approval! I do not believe I have permissions to merge, as I just joined Phabricator to make this patch. According to this: https://www.llvm.org/docs/Phabricator.html#committing-a-change

Once a patch has been reviewed and approved on Phabricator it can then be committed to trunk. If you do not have commit access, someone has to commit the change for you (with attribution). It is sufficient to add a comment to the approved review indicating you cannot commit the patch yourself.

So I assume someone with the right permissions is going to merge this?

Merged.

Also, will this appear in a minor or patch release of LLVM 15, which adds opaque pointers, or will this only appear in the next major release?

I don't think anybody has plans to backport this, so the next major release.

llvm/bindings/ocaml/llvm/llvm.ml

 
 (*--... Operations on pointer, vector, and array types .....................--*)
 
 external subtypes : lltype -> lltype array = "llvm_subtypes"
 external array_type : lltype -> int -> lltype = "llvm_array_type"
 external pointer_type : lltype -> lltype = "llvm_pointer_type"
 external qualified_pointer_type : lltype -> int -> lltype
                                 = "llvm_qualified_pointer_type"
+external pointer_type_in_context : llcontext -> int -> lltype
+                                 = "llvm_pointer_type_in_context"
+external pointer_type_is_opaque : lltype -> bool
+                                = "llvm_pointer_type_is_opaque"
 external vector_type : lltype -> int -> lltype = "llvm_vector_type"
 
 external element_type : lltype -> lltype = "LLVMGetElementType"
 external array_length : lltype -> int = "llvm_array_length"
 external address_space : lltype -> int = "llvm_address_space"
 external vector_size : lltype -> int = "llvm_vector_size"
 
 (*--... Operations on other types ..........................................--*)

llvm/bindings/ocaml/llvm/llvm.mli

(** [pointer_type ty] returns the pointer type referencing objects of type
     See the method [llvm::PointerType::getUnqual]. *)
 val pointer_type : lltype -> lltype
 
 (** [qualified_pointer_type ty as] returns the pointer type referencing objects
     of type [ty] in address space [as].
     See the method [llvm::PointerType::get]. *)
 val qualified_pointer_type : lltype -> int -> lltype
 
+(** [pointer_type_in_context context as] returns the opaque pointer type
+    referencing objects in address [as].
+    See the method [llvm::PointerType::get]. *)
+val pointer_type_in_context : llcontext -> int -> lltype
+
+(** [pointer_type_is_opaque pty] returns whether the pointer type [pty]
+    is opaque. See the method [llvm::PointerType::isOpaque]. *)
+val pointer_type_is_opaque : lltype -> bool
+
 (** [vector_type ty n] returns the array type containing [n] elements of the
     primitive type [ty]. See the method [llvm::ArrayType::get]. *)
 val vector_type : lltype -> int -> lltype
 
 (** [element_type ty] returns the element type of the pointer, vector, or array
     type [ty]. See the method [llvm::SequentialType::get]. *)
 val element_type : lltype -> lltype
 

llvm/bindings/ocaml/llvm/llvm_ocaml.c

 }
 
 /* lltype -> int -> lltype */
 LLVMTypeRef llvm_qualified_pointer_type(LLVMTypeRef ElementTy,
                                         value AddressSpace) {
   return LLVMPointerType(ElementTy, Int_val(AddressSpace));
 }
 
+/* llcontext -> int -> lltype */
+LLVMTypeRef llvm_pointer_type_in_context(LLVMContextRef C, value AddressSpace) {
+  return LLVMPointerTypeInContext(C, Int_val(AddressSpace));
+}
+
+/* lltype -> bool */
+value llvm_pointer_type_is_opaque(LLVMTypeRef PtrTy) {
+  return Val_bool(LLVMPointerTypeIsOpaque(PtrTy));
+}
+
 /* lltype -> int -> lltype */
 LLVMTypeRef llvm_vector_type(LLVMTypeRef ElementTy, value Count) {
   return LLVMVectorType(ElementTy, Int_val(Count));
 }
 
 /* lltype -> int */
 value llvm_array_length(LLVMTypeRef ArrayTy) {
   return Val_int(LLVMGetArrayLength(ArrayTy));

llvm/test/Bindings/OCaml/core.ml

 
 (*===-- Contained types  --------------------------------------------------===*)
 
 let test_contained_types () =
   let ar = struct_type context [| i32_type; i8_type |] in
   insist (i32_type = (Array.get (subtypes ar)) 0);
   insist (i8_type = (Array.get (subtypes ar)) 1)
 
+(*===-- Pointer types  ----------------------------------------------------===*)
+let test_pointer_types () =
+  insist (pointer_type_is_opaque (pointer_type_in_context context 0));
+  insist (not (pointer_type_is_opaque (pointer_type i8_type)))
 
 (*===-- Conversion --------------------------------------------------------===*)
 
 let test_conversion () =
   insist ("i32" = (string_of_lltype i32_type));
   let c = const_int i32_type 42 in
   insist ("i32 42" = (string_of_llvalue c))