Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
I agree that the caml_ prefix on caml_alloc_tuple_uninit is probably not a good idea.
Note that the intent of my comment about llvm_param_types was to show one concrete example of a pattern that occurs in several other functions, whose ml types return arrays, such as llvm_struct_element_types, llvm_subtypes, llvm_indices, etc. The added allocation seems unavoidable for the functions that accept an array and need to create an analogue where the lsbs have been cleared before passing it to an LLVM function. In a sequential setting, it would be possible to clear the lsbs of elements of the argument OCaml array in place, pass it to LLVM, and then re-set the lsbs. I am not suggesting to do that, it would be unsafely racy in a concurrent setting.
I don't know the history of what the FFI docs said when, but the OCaml bindings are *old*. The difference between caml_alloc_small and caml_alloc_shr is that the former allocates on the minor heap and the latter on the major heap. The FFI rules distinguish them since if a pointer to the/a minor heap is stored into the major heap, then the GC needs to be informed or else it may incorrectly consider the object in the minor heap to be dead. This is what caml_initialize and Store_field do. The key here is that in the case of the LLVM bindings, a crucial point of the design that sets the low bit of pointers to LLVM objects is that the memory allocated by either caml_alloc_small or caml_alloc_shr is initialized with values that the OCaml GC considers to be non-pointers. In particular, they cannot possibly be old-to-young pointers. This is the invariant across the whole of the LLVM bindings that makes the current code safe.
Replacing the existing calls to caml_alloc_tuple_uninit with caml_alloc_tuple will result in much of the memory allocated by the bindings being needlessly double-initialized. That is a change that is orthogonal to removing naked pointers, and I think that it should be considered separately. (It is not a change that I would myself like to see made, but it is a judgement call with pros and cons.)
| llvm/bindings/ocaml/llvm/llvm_ocaml.c | ||
|---|---|---|
| 255 | Is this CAMLparam/CAMLreturn left over from combining the two diffs, or necessary for a reason I am overlooking? | |
| 353 | I don't see why adding this CAMLparam is necessary | |
| 441–442 | Left over from combining the 2 diffs? | |
| 734–735 | Left over? | |
| 798–799 | Left over? | |
| 2069–2073 | I am not sure why this change is needed, rather than changing the (value) casts in the original to to_val. In particular, if V and BB are computed after Hd is allocated, they do not need to survive any allocations, so do not need to be roots. | |
| 2076 | List does not need to be a root since it is not live across any allocations. | |
| 2115 | Left over? | |
| 2127 | Left over? | |
| 2694–2695 | Left over? | |
| 2807 | Left over? | |
| 2829 | Left over? | |
Thank you for your code review. I didn't realize the reasoning behind the Field/caml_initialize distinction and was just following the OCaml documentation's rules to the letter. So, you taught me something new about the OCaml runtime, Perhaps this information could be included in a comment (an issue for another patch, of course).
I went and added back caml_alloc_tuple_uninit and got rid of the malloc/frees in the wrapper functions that return an array. I also removed all the CAMLparam/CAMLreturns you pointed out. (Some of the CAMLparams I had left were by accident, and others were because the wrapper function called the OCaml runtime API (e.g. by throwing an exception) and I wanted to be cautious. You're right that the roots aren't necessary.) There were some minor parts where I made the diff smaller as well.
I really appreciate your time and effort looking at my patch.
Thanks for persisting with this and tolerating all my comments! This looks like it is getting very close, nice! I'm sorry for the piecemeal reviewing, but making a full pass takes longer than I can usually spend at once and I figure it is better to send comments intermittently rather than save them until the end. So far I have spent most time looking at llvm_ocaml.{c,ml,mli} and not so much on the other files, but I think that llvm_ocaml is just about there.
For info I found https://github.com/ocaml/ocaml/blob/f1550145d4ebe8ee22853d13f97fca288aaef51f/runtime/memory.c#L203 which notes that normal assignments are ok to initialize freshly allocated blocks with value that are not pointers to young blocks.
| llvm/bindings/ocaml/llvm/llvm_ocaml.c | ||
|---|---|---|
| 46 | It might not hurt to add a comment before to_val and from_val saying that they encode pointers to LLVM objects as OCaml tagged integers, following the 3rd encoding suggested by the OCaml FFI documentation at https://ocaml.org/releases/5.0/htmlman/intfc.html#ss:c-outside-head . I don't know how stable such urls are expected to be though. | |
| 55 | ||
| 128–139 | This function could probably use a comment explaining its usage since its functionality is quite specific and has a general sounding name. I don't feel strongly, but there might be a better name possible. It is basically from_val lifted to operate on arrays, so perhaps from_val_array. | |
| 129 | Note that Wosize_val returns type mlsize_t, which is defined in a platform-specific way because it needs to be different on 32 vs 64 bit systems (it will be uint64_t on 64-bit systems, so usually wider than unsigned). It would probably be better to use mlsize_t instead of unsigned where the values come from OCaml and keep unsigned where the values come from LLVM. In cases where both are involved, I think but am not sure that it would generally be clearer to have the local variables with the unconverted types, and leave the casts between unsigned and mlsize_t implicit when passing arguments to functions. | |
| 134 | Just a note to ourselves for maybe later: If we want to make the bindings safe for concurrent clients, we may want something stronger than a non-atomic store here. (The load from Elements is volatile.) | |
| 370–374 | No big deal, but I don't know why this function was moved up a few lines | |
| 514 | I think this CAMLparam is not needed since alloc_temp allocates off the OCaml heap. And if this one is needed, then RetTy should also be a root. | |
| 523 | Same as function above | |
| 556 | here too | |
| 565 | and here | |
| 584 | here | |
| 585 | Related to the comment on alloc_temp about unsigned, I think it would be clearer if this unsigned was instead mlsize_t, leaving the case to unsigned implicit in the call to LLVMStructSetBody below. There are several other similar uses, I would look around each call to alloc_temp and at each call to Wosize_val. | |
| 1802 | Unless reordering the next several definitions is needed, it would be easier to review if they were left in the same order. They can be reordered if desired in a separate diff that only reorders things. | |
| 1957–1958 | Could you double-check this one. Temp is allocated on the caml heap unlike other functions, and as written F and Index will need to be registered as roots. | |
| 2127 | Still left over I think | |
Oh no, I did something wrong with Arcanist. I ran arc diff and thought it would push a revision to this diff, but it instead made a new patch. I feel very frustrated with Arcanist, because it isn't explicit with diff IDs, and sometimes it surprises me about what patch it pushes to. I need to figure out what's wrong with the repo state on my computer and how I can fix this problem.
| llvm/bindings/ocaml/llvm/llvm_ocaml.c | ||
|---|---|---|
| 46 | I added comments to the header file explaining how these functions work. Should I put additional comments in the source file? | |
| 1957–1958 | F is an llvalue, which is represented using the low bit tagging scheme, and Index is an int, so both are unboxed and neither is a root. Is this reasoning correct? | |
| llvm/bindings/ocaml/llvm/llvm_ocaml.c | ||
|---|---|---|
| 46 | The comment in the header is good but I think since it discusses the implementation details of the encoding, I would have put it in the .c and used a comment in the header that just said that the functions implement an encoding without specifying which one. But I have not checked if the uses in other files need to know about details of the encoding. Those are the things I think of, but go with your preference. | |
| 1957–1958 | You're right about F and Index. I would expect an implementation more like in the suggested edit, similar to llvm_struct_element_types. In particular, Temp is an ocaml block with enough space for only 1 word, and I think LLVMGetCallSiteAttributes expects a pointer to an array with space for Length pointers. | |
| llvm/bindings/ocaml/llvm/llvm_ocaml.h | ||
| 38 | ||
| 45 | maybe | |
| llvm/bindings/ocaml/debuginfo/debuginfo_ocaml.c | ||
|---|---|---|
| 364 | several unsigned ought to be mlsize_t in this file | |
| llvm/bindings/ocaml/executionengine/executionengine_ocaml.c | ||
| 104–105 | left over | |
| llvm/bindings/ocaml/llvm/llvm_ocaml.c | ||
| 798–799 | still left over | |
| llvm/bindings/ocaml/llvm/llvm_ocaml.h | ||
| 27 | intended? | |
| llvm/bindings/ocaml/target/target_ocaml.c | ||
| 57 | left over | |
| 62–63 | left over | |
| 146 | left over | |
| 154–155 | left over | |
| 172 | left over | |
| 177 | left over | |
| 241–242 | left over | |
| 308 | left over | |
There were some parts where you had commented about CAMLparam and which I had previously chosen to keep the macro because the function called the OCaml runtime, such as by raising an OCaml exception or allocating from the OCaml heap, and I wanted to be careful. However, I went and accepted your suggestions of removing them because I don't think the roots are necessary for one of two reasons:
- The local variable only exists to store the result of an OCaml heap allocation, so before the call to the OCaml runtime, it does not contain a root
- The local variable only exists to store data to pass to an LLVM API call, and by the point that the OCaml runtime is called, the local variable no longer needs to be live.
However, there is one point where I kept the CAMLparam and left a comment at, in which the function must register one of its parameters as a root as it makes an OCaml heap allocation and the argument of course needs to be live throughout the function call.
I feel a little nervous about removing the CAMLparam macros because I'm worried about mistakes.
| llvm/bindings/ocaml/llvm/llvm_ocaml.c | ||
|---|---|---|
| 46 | The reason I would rather have the comments in the header is that if the functions were to be used for some LLVM type that is not 2-byte aligned, or the implementation of some type changed so that it is no longer 2-byte aligned, the code will break. (In that case, the specific type that is not 2-byte aligned should have a dedicated alloc_ function, just like the ones that already exist for types that need custom finalizers.) Therefore, I think that the 2-byte alignment aspect is a contract that users of the functions need to know about, not an implementation detail. | |
| llvm/bindings/ocaml/target/target_ocaml.c | ||
| 146 | This root is necessary. DataLayout.t is an OCaml custom block, not a pointer with the low bit set. Int64.t is also boxed. So, this function allocates on the OCaml heap and the GC needs to know that DL is live. | |
| llvm/bindings/ocaml/llvm/llvm_ocaml.c | ||
|---|---|---|
| 46 | Yes, that makes sense. | |
| 1953–1954 | these no longer need to be added | |
| llvm/bindings/ocaml/target/target_ocaml.c | ||
| 146 | I understand that DL is a custom block, but its value is only used before the allocation in caml_copy_int64. This is the same before and after this diff, or am I overlooking something? | |
| llvm/bindings/ocaml/target/target_ocaml.c | ||
|---|---|---|
| 146 | DL is a parameter, so it needs to be live when the function ends. Otherwise, a caller could pass a DataLayout.t value, and that value could be invalidated by the GC during the function call. | |
I feel a little bit apprehensive about the lack of CAML macros in some places. After this patch is done, I think it might be worth it to review the code and make a patch that:
- Adds CAML macros if there are any places we left them out but shouldn't have
- Add comments documenting the justification for being loose with the OCaml runtime API rules (e.g. that using Field to initialize an allocation from caml_alloc_tuple_uninit is safe because you are never storing a pointer in it, or that omitting CAMLlocal is safe when the variable is only used to store the result of calling the OCaml runtime API).
Do you think this is necessary or advisable?
If the reason why this patch doesn't add CAML macros is just to keep the diff small and focus on one issue (removing naked pointers), it might be worth it to have another patch that just adds CAML macros everywhere. It's a question of whether the CAML macros add any overhead that one would want to avoid, and whether the cost of being loose with the rules and adding comments to convey the rationale to other people reading the code is worth the maintenance effort versus just following the (oversimplified) rules of the OCaml docs.
Adding comments about the reasoning behind e.g. avoiding the write barrier seems like a good idea. I don't know if it would be better to have one comment block near the top of the file, or many comments sprinkled around. There might be a lot of redundancy which would be noisy and easy to get out of sync.
There is some overhead to the CAMLparam, etc. macros. Each one involves some memory operations itself, checks the thread local state of the runtime system, and also increases the number of roots that need to be scanned each time the GC is run. I would be hesitant to add them uniformly without a good benchmark suite to measure the impact and see that it doesn't matter.
| llvm/bindings/ocaml/target/target_ocaml.c | ||
|---|---|---|
| 146 | I'm sorry but I still don't understand. Suppose the GC runs when caml_copy_int64 allocates, and happens to move the value DL points to. Then note that DL is no longer needed since it is used only to compute the argument of LLVMOffsetOfElement and in particular is dead after the allocation in caml_copy_int64. Also, if the caller of llvm_datalayout_offset_of_element needs what it passed as the DL argument, then it will be live and a root in the caller, and the GC will update the caller's pointer when moving the block pointed to by DL. So I don't see the issue, but I am not sure I understand the point you are making. Do you see what I mean? | |
| llvm/bindings/ocaml/target/target_ocaml.c | ||
|---|---|---|
| 146 | You're right, I made a mistake. I didn't consider that if the caller still needed to keep DL around, the caller stack frame would already have DL as a root, so this function doesn't need to register it as a root. Thank you for reviewing my code and pointing this out. | |
I have redone my own testing with the latest version of this diff plus changes for the inline comments in this last iteration of review. This test translates each of the bitcode files in llvm-project/llvm/test to a different IR, as well as translating and doing some static analysis on several test programs. This testing did not uncover any issues with this diff.
Additionally, the test was repeated where the translation of the functions of each llvm module was done in parallel (using Domainslib.Task.parallel_for). This is not much of a parallel stress test, but it did not encounter any issues.
So these inline comments are the last minor changes, then this can be rebased, and I can commit it if you need.
| llvm/bindings/ocaml/llvm/llvm_ocaml.c | ||
|---|---|---|
| 837 | It is pre-existing, but while you are at it, it may be clearer and more uniform to change to ++I. | |
| 842 | This CAMLparam needs to be kept since Name needs to survive the allocation of Nodes. | |
| 1145 | Removing these parens triggers warning: using the result of an assignment as a condition without parentheses [-Wparentheses] | |
What "rebasing" does this need? As far as I can tell, it is compatible with the main branch. The last commit to the OCaml bindings is my patch removing the PassManager functions, which this patch is already rebased on. If this weren't ready to merge, Phabricator would say "patch application failed."
Please do commit this for me.