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

[Filipe Cabecinhas via Phabricator via cfe-commits]

filcab added inline comments.


================
Comment at: test/CodeGenObjC/ubsan-bool.m:26
+  // OBJC:  [[ICMP:%.*]] = icmp ule i8 [[ASHR]], 1, !nosanitize
+  // OBJC: call void @__ubsan_handle_load_invalid_value
+
----------------
jroelofs wrote:
> vsk wrote:
> > jroelofs wrote:
> > > 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?
> > Good point, we don't need to emit the range check if the bitfield is an unsigned, 1-bit wide BOOL. Would you be OK with me tackling that in a follow-up patch?
> No problem, sounds good to me.
I don't like generating an error here.
-1 is a perfectly valid BOOL (signed char) value and it's a perfectly valid value to have in a (signed) 1-wide bitfield.


================
Comment at: test/CodeGenObjC/ubsan-bool.m:50
+// OBJC: [[SHL:%.*]] = shl i8 [[LOAD]], 7
+// OBJC: [[ASHR:%.*]] = ashr i8 [[SHL]], 7
+// OBJC: icmp ule i8 [[ASHR]], 1, !nosanitize
----------------
jroelofs wrote:
> hmmm. just occurred to me that this value is always going to be 0xff or 0x0, so it might be useful if there were also a frontend warning that complains about signed bitfield bools (maybe something useful for another patch).
I'm more ok with this, assuming it makes sense in ObjC.


https://reviews.llvm.org/D30423