Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix segfaults with LLVM >= 13.0.0 #10

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+8 −1
Open

Fix segfaults with LLVM >= 13.0.0 #10

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion src/backend/backend.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -555,7 +555,14 @@ let make ~modul ~imports options x =
let debug = options#debug
end)
in
Module.make ~imports x
Module.make ~imports x |>
Llvm_bitwriter.write_bitcode_to_memory_buffer |>
Llvm_bitreader.parse_bitcode c
(* TODO: WTF?! Someone please tell me why without this LLVM >= 13 segfaults *)
(* I was able to get it not segfault for some of the tests by replacing all
uses of Llvm.global_context into Llvm.create_context, but it would
duplicate the intrinsics (e.g. @llvm.stacksave & @llvm.stacksave.renamed)
and fail at link time if they were used. *)
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jberdine Do you have any idea what's going by any chance?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very strange indeed! All the things I work with that get any testing are still on llvm 12, so it could be just something waiting to go wrong. @vaivaswatha does this ring any bells for you?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very strange indeed! All the things I work with that get any testing are still on llvm 12, so it could be just something waiting to go wrong. @vaivaswatha does this ring any bells for you?

Nope, no idea. Weird though.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ll try to port the llvm binding to OCaml 5.0. This requires removing all the raw pointers returned to the ocaml world, I’m hoping the problem comes from some place when the binding uses raw pointers where it isn’t safe to do so in OCaml 4

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have code using llvm where I have to make sure that there are no dead ocaml values with pointers into llvm-allocated memory before calling llvm functions that deallocate memory. What can happen is that the ocaml heap can grow into the memory freed by llvm, and the dead ocaml values with naked pointers end up pointing into the heap, causing the GC to follow them and crash.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mmh, I’m not sure if it’s related. Unless I’m missing something, both should be equivalent as far as the OCaml runtime is concerned.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but experimenting I noticed that with llvm 12 one can get away with calling dispose_context on global_context while with llvm >= 13 this crashes. Since you could avoid the crashes by using create_context instead, this makes me wonder if somewhere some code is manipulating the context in a way that is not happy with the new way the global context is allocated. But as I said, it's definitely a stretch.

Copy link
Owner Author

@kit-ty-kate kit-ty-kate Feb 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was about to test your theory but i realized this change was not made in LLVM 13 but in LLVM 15, so this is for sure unrelated

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, ok, sorry for the noise. I wonder what I actually did when testing.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries at all. Thanks for looking into it <3


let main options main_module =
let module Module =
Expand Down