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

[Filipe Cabecinhas via Phabricator via cfe-commits]

filcab added a comment.

LGTM since my issue is only an issue on ObjC platforms and it seems those are the semantics it should have there.



================
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:
> 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?
It looks weird that we would disallow using a 1-bit (signed) bitfield for storing `BOOL`. But since this will only be a problem on obj-c, I don't think it will be a problem in general. If you're saying those are the semantics, then I'm ok with it.
P.S: If it documented that the only possible values for `BOOL` are `YES` and `NO` (assuming they are `1` and `0`)?
If so, I wonder if it's planned to document that you can't store a `YES` on a `BOOL` bitfield with a width of 1, since it looks like a weird case.


https://reviews.llvm.org/D30423