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

[Vedant Kumar via Phabricator via cfe-commits]

vsk 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
+
----------------
filcab wrote:
> 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.
True/false should be the only values loaded from a boolean. Since we made ubsan stricter about the range of BOOL (r289290), the check has caught a lot of bugs in our software. Specifically, it's helped root out buggy BOOL initialization and portability problems (the signedness of BOOL is not consistent on Apple platforms). There have been no false positives so far.

Is this an issue you think needs to be addressed in this patch?


================
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
----------------
filcab wrote:
> 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.
Yes, we have an internal feature request for such a warning.


https://reviews.llvm.org/D30423