TheDan64 / inkwell

It's a New Kind of Wrapper for Exposing LLVM (Safely)
https://thedan64.github.io/inkwell/
Apache License 2.0
2.38k stars 229 forks source link

Building GEP instruction shouldn't be unsafe #514

Open vaivaswatha opened 4 months ago

vaivaswatha commented 4 months ago

The doc comment on the method has the following statement.

GEP is very likely to segfault if indexes are used incorrectly, and is therefore an unsafe function. Maybe we can change this in the future.

Wrong GEP indices leads to asserts (on debug builds of LLVM), and is perhaps a good enough reason to not have unsafe for this method.

Alternatively, a new inkwell datatype can be introduced for GEP indices, to make them safer. Something similar was done in Haskell. See https://luctielen.com/posts/making_llvm_gep_safer_in_haskell/.

TheDan64 commented 4 months ago

asserts failing in C translate to UB or segfault on the rust side. So that's the reason for unsafe.

Pretty cool. I've thought about something similar to that haskell post, but never was able to get it working fully

vaivaswatha commented 4 months ago

Another simpler solution (but may be not so elegant) would be to just validate that the indices are valid for the provided pointer type.

This will require re-implementing something like GetElementPtrIns::getIndexedType. Note that the llvm-c API doesn't expose that method. (I tried to ask if they are willing to have it exposed, but they seemed reluctant).

With such a function, we could evaluate whether the indices are valid, and only then call LLVM's gep builder, being sure that the asserts won't hit.