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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
