Page MenuHomePhabricator

[llvm-ocaml] Migrate from naked pointers to prepare for OCaml 5
Needs RevisionPublic

Authored by alan on Oct 20 2022, 5:19 PM.

Diff Detail

Unit TestsFailed

TimeTest
60,080 msx64 debian > MLIR.Examples/standalone::test.toy
Script: -- : 'RUN: at line 1'; /usr/bin/cmake /var/lib/buildkite-agent/builds/llvm-project/mlir/examples/standalone -G "Ninja" -DCMAKE_CXX_COMPILER=/usr/bin/clang++ -DCMAKE_C_COMPILER=/usr/bin/clang -DLLVM_ENABLE_LIBCXX=OFF -DMLIR_DIR=/var/lib/buildkite-agent/builds/llvm-project/build/lib/cmake/mlir -DLLVM_USE_LINKER=lld -DPython3_EXECUTABLE="/usr/bin/python3.9"
60,050 msx64 debian > libFuzzer.libFuzzer::fuzzer-leak.test
Script: -- : 'RUN: at line 3'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address,fuzzer -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/lib/fuzzer -m64 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/fuzzer/LeakTest.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/fuzzer-leak.test.tmp-LeakTest
60,030 msx64 debian > libFuzzer.libFuzzer::value-profile-load.test
Script: -- : 'RUN: at line 2'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address,fuzzer -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/lib/fuzzer -m64 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/fuzzer/LoadTest.cpp -fsanitize-coverage=trace-gep -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/value-profile-load.test.tmp-LoadTest

Event Timeline

alan created this revision.Oct 20 2022, 5:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 20 2022, 5:19 PM
Herald added a subscriber: ormris. · View Herald Transcript
alan requested review of this revision.Oct 20 2022, 5:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 20 2022, 5:19 PM
alan updated this revision to Diff 469448.Oct 20 2022, 6:40 PM

Wrap data in CAMLlocal1

nikic resigned from this revision.Oct 21 2022, 3:02 AM

Nice work! Unfortunately I don't know enough about ocaml to meaningfully review this.

jberdine added a subscriber: nikic.Oct 21 2022, 3:24 AM
jberdine added inline comments.
llvm/bindings/ocaml/llvm/llvm_ocaml.c
53–69

@nikic this section of code is something that it would be good to get eyes on from someone familiar with the LLVM C API without needing any familiarity with OCaml specifics.

The to_val function is used to "encode" pointers returned from the LLVM C API to the client OCaml code, and from_val performs the corresponding "decode". What to_val does is to test if the pointer is at least 2-byte aligned, and if so encodes it by setting the low bit. If the pointer is not 2-aligned, a wrapper object is allocated and the pointer stored in it. The OCaml runtime system guarantees that the wrapper object will be word aligned, and so the decode done in from_val tests the low bit with Is_long to distinguish the two cases.

This implementation LGTM (I have not had time yet to check the usages in this diff), but a question for those more familiar with the LLVM C API is whether the case where pointers are not aligned is impossible due to guarantees about alignment? The implementation in this diff is conservative and supports unaligned pointers, but is perhaps not as efficient as possible if there are reasons to know all pointers from LLVM will be aligned.

nikic added inline comments.Oct 21 2022, 3:48 AM
llvm/bindings/ocaml/llvm/llvm_ocaml.c
53–69

I believe it's safe to assume that LLVM allocations are at least 4 byte aligned (we commonly use 2 bits for pointer tags). Only problem would be cases where something other than an allocation base pointer is returned, e.g. some arbitrary substring of a larger string. I don't know if we have any APIs that do something like this though.

jrtc27 added a subscriber: jrtc27.Oct 21 2022, 11:09 AM
jrtc27 added inline comments.
llvm/bindings/ocaml/llvm/llvm_ocaml.c
53–69

m68k's ABI caps alignment at 2 bytes (unless you __attribute__((aligned(...))) or alignas(...)), FWIW, though there are currently places where LLVM itself breaks as a result of that

alan updated this revision to Diff 469808.Oct 21 2022, 3:48 PM

Eliminate unsafe uses of caml_alloc_small

alan updated this revision to Diff 469829.Oct 21 2022, 5:14 PM

Replace more returns with CAMLreturns in debuginfo

alan added a comment.EditedOct 21 2022, 5:25 PM

The original code defined caml_alloc_tuple_uninit, which either called caml_alloc_small or caml_alloc_shr. Then, the original code would initialize the allocation by passing the pointer to the LLVM C API function.

According to the OCaml docs: https://v2.ocaml.org/manual/intfc.html#ss:c-low-level-gc-harmony

Initialization of caml_alloc_small allocations should be done with Field(v, n) = v_n, and initialization of caml_alloc_shr allocations should be done with caml_initialize(&Field(v, n), v_n). So, the usage in the original code was incorrect.

Furthermore, caml_alloc_small and caml_alloc_shr must be initialized before the next allocation. In my patch, to_val is called when initializing the allocation. Assuming that all LLVM pointers are 2-bit aligned, to_val should not allocate, but we are not sure that all pointers are 2-bit aligned. If to_val allocates, then the usage of caml_alloc_small and caml_alloc_shr are unsafe.

(Also, I'm still getting the hang of Phabricator... I had left a comment on the code this morning, but realized that it wasn't visible for other people until I made this comment, after I was able to make the necessary changes.)

llvm/bindings/ocaml/llvm/llvm_ocaml.c
2392–2395

I just found out that using Store_field here is unsafe, but I'm going to work and can't fix this until I come back home.

alan updated this revision to Diff 469914.Oct 22 2022, 10:18 AM

Remove outdated comments

alan added a comment.EditedOct 22 2022, 10:42 AM

For what it's worth, in my local repository, I redefined to_val to the following:

value to_val(void *ptr) {
  if ((((value)ptr) & 1) == 0) {
    return ((value)ptr) + 1;
  } else {
    return NULL;
  }
}

and all the tests passed on OCaml 5 beta1.

These are all the LLVM types that previously used naked pointers and now use the from_val/to_val conversions.

#define ExecutionEngine_val(v) ((LLVMExecutionEngineRef)from_val(v))
#define DiagnosticInfo_val(v) ((LLVMDiagnosticInfoRef)from_val(v))
#define Context_val(v) ((LLVMContextRef)from_val(v))
#define Attribute_val(v) ((LLVMAttributeRef)from_val(v))
#define Module_val(v) ((LLVMModuleRef)from_val(v))
#define Metadata_val(v) ((LLVMMetadataRef)from_val(v))
#define Type_val(v) ((LLVMTypeRef)from_val(v))
#define Value_val(v) ((LLVMValueRef)from_val(v))
#define Use_val(v) ((LLVMUseRef)from_val(v))
#define BasicBlock_val(v) ((LLVMBasicBlockRef)from_val(v))
#define MemoryBuffer_val(v) ((LLVMMemoryBufferRef)from_val(v))
#define PassManager_val(v) ((LLVMPassManagerRef)from_val(v))
#define TargetMachine_val(v) ((LLVMTargetMachineRef)from_val(v))

Glancing at their implementations:

I think it's safe to say that all are 2-bit aligned or more.

  • Should I omit registering locals with CAMLparam if the value is known to not be an OCaml heap allocation (e.g. it is an Int or a Bool)?
  • Should I switch the implementations of to_val and from_val to assume 2-bit alignment, and not register LLVM objects with CAMLparam?
  • Should I omit CAMLparam and CAMLreturn entirely if the function does not allocate in the OCaml heap?
alan added a comment.Oct 22 2022, 12:18 PM
This comment was removed by alan.
alan updated this revision to Diff 469920.Oct 22 2022, 12:24 PM

Rewrite commit message

alan retitled this revision from [ocaml-llvm] Migrate from naked pointers in preparation for OCaml 5 to [llvm-ocaml] Migrate from naked pointers to prepare for OCaml 5.Oct 22 2022, 12:31 PM
alan added a comment.EditedOct 23 2022, 12:46 AM

The original binding code on the main branch often skipped using CAMLparamX and CAMLreturn because many of the functions did not manipulate OCaml heap objects. When converting the bindings to eliminate naked pointers, I followed all the rules at https://v2.ocaml.org/manual/intfc.html because I assumed that to_val might allocate on the OCaml heap. Now, I have a stacked diff at https://reviews.llvm.org/D136537 that assumes that all pointers that LLVM provides OCaml are at least 2-bit aligned, and therefore don't require an allocation on the OCaml heap. This diff also:

  • Skips CAMLparamX and CAMLreturn entirely if the function does not allocate on the OCaml heap
  • Only registers OCaml boxed values with CAMLparamX (i.e. it does not pass an int or a bool to CAMLparamX)

The code passes all tests on OCaml 5.0~beta1. This stacked diff is closer to the original code on the main branch. However, it relies on more assumptions that could possibly change, or I may have gotten wrong, so it is more brittle. I will leave it up to the reviewers to decide if the performance benefit is worth these changes.

aeubanks resigned from this revision.Oct 23 2022, 11:57 AM
alan added a comment.EditedOct 24 2022, 2:25 PM

I'm concerned about the diagnostic handler-related functions. An arbitrary OCaml function can be set as the handler, so any LLVM function that invokes the diagnostic handler anywhere may allocate on the OCaml heap, therefore triggering the GC. (This weakens some assumptions that I thought allowed me to omit some instances of CAMLparam in my stacked diff.) What's more, any such LLVM function may throw an OCaml exception, which will unwind the stack, meaning:

  • Calls to free in these bindings will not be reached. (The fix would be to allocate on the OCaml heap using Abstract_tag.)
  • Assumptions about resource cleanup deeper in the LLVM code will be violated (e.g. C++ destructors may not run).

Is the diagnostic handler feature safe?

alan updated this revision to Diff 470636.Oct 25 2022, 3:59 PM

Check for NULL in alloc_temp

alan added a comment.Tue, Jan 17, 8:13 PM

Starting from the 20th, I will be recovering from a major medical operation. For an unknown length of time, possibly several months, I will be incapacitated from working on this patch.

jberdine requested changes to this revision.Wed, Feb 8, 3:28 PM

Only a partial review so far.

llvm/bindings/ocaml/llvm/llvm.ml
444

Missed wrapping this function to decode args and encode result.

513

Missed wrapping this function to decode args and encode result.

This revision now requires changes to proceed.Wed, Feb 8, 3:29 PM