Page MenuHomePhabricator

[llvm-ocaml] Assume pointers are at least 2-bit aligned
Needs RevisionPublic

Authored by alan on Oct 22 2022, 12:55 PM.

Details

Summary

This diff assumes that all pointers that LLVM gives to OCaml are at least 2-bit aligned, removing the branch that allocates/unwraps the OCaml Abstract_tag block.

The current binding code on the main branch skips a lot of CAMLparamX and CAMLreturn boilerplate because the wrapped code did not allocate. My parent patch added the CAMLparamX and CAMLreturn boilerplate because I assumed the code might allocate. This patch undoes that, since it again assumes that there is no allocation.

Furthermore, if the code does allocate, I only register boxed values (such as strings) with CAMLlocalX, skipping unboxed values (such as ints).

Diff Detail

Unit TestsFailed

TimeTest
60,040 msx64 debian > ThreadSanitizer-x86_64.ThreadSanitizer-x86_64::restore_stack.cpp
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=thread -Wall -m64 -msse4.2 -gline-tables-only -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/../ -std=c++11 -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/../ -O1 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/restore_stack.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/tsan/X86_64Config/Output/restore_stack.cpp.tmp && not /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/tsan/X86_64Config/Output/restore_stack.cpp.tmp 2>&1 | FileCheck /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/restore_stack.cpp
60,040 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 22 2022, 12:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 22 2022, 12:55 PM
alan requested review of this revision.Oct 22 2022, 12:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 22 2022, 12:55 PM
alan removed a subscriber: llvm-commits.Oct 22 2022, 1:09 PM
alan updated this revision to Diff 469937.Oct 22 2022, 3:15 PM

Testing...

alan updated this revision to Diff 469941.Oct 22 2022, 3:28 PM

Add missing return

alan edited the summary of this revision. (Show Details)Oct 22 2022, 4:16 PM
alan added a comment.Oct 22 2022, 6:51 PM

I'm extremely puzzled by https://github.com/llvm/llvm-project/blob/main/llvm/test/Bindings/OCaml/ext_exc.ml. When I test on OCaml 4.14, all tests pass, but when I test on OCaml 5.0~beta1, this test hangs forever. All tests on the parent patch pass on OCaml 5.0~beta1.

When I look at the backtrace with gdb, the test hangs in the function LLVMCreateMemoryBufferWithSTDIN. I think that the code is reading from stdin? Is the code supposed to signal an error instead?

alan updated this revision to Diff 469950.Oct 22 2022, 7:13 PM

Don't use CAML macros in iterator functions

alan updated this revision to Diff 469956.Oct 22 2022, 9:13 PM

Add missing CAMLreturn

alan updated this revision to Diff 469964.Oct 22 2022, 11:05 PM

Fix missing CAMLreturn causing hanging tests

alan updated this revision to Diff 469966.Oct 23 2022, 12:37 AM

Convert the rest of the files

alan added a comment.Oct 24 2022, 5:18 AM

https://discuss.ocaml.org/t/relaxed-rules-for-binding-a-c-library/10693/ seems to imply that the shortcuts that this diff makes are still safe.

alan updated this revision to Diff 470318.Oct 24 2022, 4:23 PM

Assume all functions may allocate on the OCaml heap

alan updated this revision to Diff 470637.Oct 25 2022, 4:06 PM

Rebase onto parent patch

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

Just a few initial comments so far, I have read and tested this some, but need to do more. Also, I think it would be easier to review if this diff and D136400 were combined into a single diff.

llvm/bindings/ocaml/llvm/llvm_ocaml.c
38–40

It may not matter, but I would be more comfortable to use bitwise operations to set and clear the low bit of pointers. And I think it would be better to add an assertion to test that the low bit is clear, as a way to more explicitly document the assumption, and to give us a chance to catch it in a debug build if it is ever violated.

250–252

CAMLparam and CAMLreturn do not need to be added (by the composition of this diff and its parent) here, right? There are other functions that seem to be in a similar situation, but asking about one example to check my understanding.

602

alloc_temp expects an ocaml block, not a size

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