Add debug checks to const gep argument bit sizes to resolve #213 #214
Conversation
| @@ -68,7 +68,17 @@ impl<'ctx> PointerValue<'ctx> { | |||
|
|
|||
| // REVIEW: Should this be on array value too? | |||
| /// 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. | |||
| /// GEP indexes must be 32 bit integer values. Constant folding may result in other sizes working. | |||
There was a problem hiding this comment.
Let's remove the "Constant folding may result in other sizes working." comment. It's a pretty neat insight but doesn't help users in any way. You can move it to a regular, non-doc comment if you'd like.
| pub unsafe fn const_gep(self, ordered_indexes: &[IntValue<'ctx>]) -> PointerValue<'ctx> { | ||
| if cfg!(debug_assertions) { |
There was a problem hiding this comment.
Let's not make this just on debug builds. Imo, preventing UB is something we should always do.
| for (index, value) in ordered_indexes.iter().enumerate() { | ||
| let bit_width = value.get_type().get_bit_width(); | ||
| if bit_width != 32 { | ||
| panic!("Index #{} in ordered_indexes argument to const_gep call was a {} bit integer instead of 32 bit integer - gep indexes must be i32 values", index, bit_width); |
There was a problem hiding this comment.
Let's return results instead. IE Result<PointerValue<'ctx>, String>. We generally don't panic on errors. The only exceptions I can think of are all very unlikely edge-cases.
|
@djozis Does my feedback make sense? Is there anything I can expand upon? |
|
@TheDan64 Sorry for the delayed response. Yes, I understand. I had trouble with this conceptually because I wanted my use of LLVM to be as efficient as possible, and considered this type of error path to be something likely encountered statically - ie. a code path encountered while first developing something invoking this, occurring always until the invocation is corrected, rather than a code path which could be triggered at runtime by providing bad inputs. For that reason, I preferred the panic approach, and to do it during debug only, since it keeps the efficiency optimal for release, but also prevents the pain I experienced. I appreciate that's just an opinion though, definitely not objectively always true, etc. It just felt strange to contribute something that would make it less appealing for my use-case. Before this slipped from mind, I was thinking that perhaps it should be caught through static typing somehow. But that's not a trivial change. |
Panicking isn't any more efficient because it still has to make a check and you could achieve the same results by unwrapping the output Result anyway. Any performance bottleneck you might notice is more likely to be LLVM than Inkwell |
|
Static typing would be ideal, but I don't think there's enough Rust support for a sufficient amount of it yet |
The efficiency I was referring to would come from keeping the check to debug mode only, as I was thinking it's a code path you'd encounter in a static way (either the program always hits that path or never does). |
I don't really think this approach makes for very idiomatic Rust. Error cases should always be enumerated through |
TheDan64
force-pushed
the
master
branch
3 times, most recently
from
b7ef5f6
to
7684346
Compare
Description
Add debug checks to const gep argument bit sizes.
Related Issue
#213
How This Has Been Tested
Tested by running tests in a dependent project, which exercised passing i64 and i32 values. Tests are similar to those in #213.
Also added tests, but was unable to build the tests locally as Windows is not current supported in the tests. A trivial attempt to add Windows support failed. I copied the added tests to a dependent project and validated that they pass. Was unable to run all tests.
Option<Breaking Changes>
This would break code which passes integers with bit widths other than 32, and were getting lucky and working due to constant folding. I'd say that's getting lucky with UB.
Checklist