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

[Vedant Kumar via Phabricator via cfe-commits]

vsk added a comment.

Thanks for the feedback!



================
Comment at: test/CodeGenCXX/ubsan-bitfields.cpp:21
+  // CHECK: call void @__ubsan_handle_load_invalid_value
+  return s->e1;
+}
----------------
arphaman wrote:
> Can we avoid the check if the bitfield is 2 bits wide?
I don't think so, because if the user memset()'s the memory backing the struct, we could still load a value outside of {1, 2, 3} from the bitfield.


================
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
----------------
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.


================
Comment at: test/CodeGenObjC/ubsan-bool.m:26
+  // OBJC:  [[ICMP:%.*]] = icmp ule i8 [[ASHR]], 1, !nosanitize
+  // OBJC: call void @__ubsan_handle_load_invalid_value
+
----------------
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')
```


https://reviews.llvm.org/D30423