[PATCH] D30423: [ubsan] Detect UB loads from bitfields

[Jonathan Roelofs via Phabricator via cfe-commits]

jroelofs added a comment.

I think this might miss loads from bitfield ivars. Also, what about the conversion that happens for properties whose backing ivar is a bitfield? (or does that happen in the runtime? can't remember)



================
Comment at: test/CodeGenObjC/ubsan-bool.m:25
+  // OBJC:  [[ASHR:%.*]] = ashr i8 [[SHL]], 7
+  // OBJC:  [[ICMP:%.*]] = icmp ule i8 [[ASHR]], 1, !nosanitize
+  // OBJC: call void @__ubsan_handle_load_invalid_value
----------------
vsk wrote:
> arphaman wrote:
> > One unrelated thing that I noticed in the IR, the `zext`s to `i64` are emitted before the branch, even though they are used only in the `invalid_value` blocks. I know that the optimizer can move those anyway, but would there any be any benefit in moving them into the blocks at the frontend IRGen level?
> I'm not sure. Maybe it would make the live range of the zext smaller at -O0? I'll look into this and see if there's a real perf impact.
@arphaman if it can be done easily/eleganly during IRGen, then this is a compile-duration win.


================
Comment at: test/CodeGenObjC/ubsan-bool.m:26
+  // OBJC:  [[ICMP:%.*]] = icmp ule i8 [[ASHR]], 1, !nosanitize
+  // OBJC: call void @__ubsan_handle_load_invalid_value
+
----------------
vsk wrote:
> arphaman wrote:
> > Is it possible to avoid the check here since the bitfield is just one bit wide?
> No, a user could do:
> 
> ```
> struct S1 s;
> s.b1 = 1;
> if (s.b1 == 1) // evaluates to false, s.b1 is negative
> ```
> 
> With this patch we get:
> ```
> runtime error: load of value -1, which is not a valid value for type 'BOOL' (aka 'signed char')
> ```
What if BOOL is an `unsigned char`, or a `char` that's not signed?


https://reviews.llvm.org/D30423