[clang][Sema] Use size of char for void types

[void]

Summary

The extension that allows for pointer arithmetic on 'void' types treats
the 'void' as a 'char'. We should use the 'char' size instead of one in
this case to allow warning when pointer arithmetic would go out of
bounds.

[serge-sans-paille]

clang/test/Sema/array-bounds-ptr-arith.c

14

I'm fine with that change, but I don't understand how it relates to that commit ;-)

[void]

clang/test/Sema/array-bounds-ptr-arith.c

14

The original size 80 was large enough to emit a warning. However, 9 *didn't* emit a warning, but should. (Note that 8 won't emit a warning because of pointer arithmetic voodoo.)

[nickdesaulniers]

clang/test/Sema/array-bounds-ptr-arith.c

14

It's slightly confusing, but the previous increment of 80 masked the issue with a smaller increment that was still out of bounds, IIUC. I think the previous code had a bytes vs bits bug?

[void]

clang/test/Sema/array-bounds-ptr-arith.c

14

Yes, that's pretty much what way happening. The comparison would be 80 (index) vs 64 (array length). But when the ptrarith_typesize is one, the comparison is 9 vs 64.

[void]

Fix up testcase.

[nickdesaulniers]

LGTM; please consider appending to the commit message something along the lines of "operations on the result of getTypeSize() are in bits, not bytes. Using 1 as the value for void* is the correct number of bytes, but we're doing arithmetic in bits." or something to denote this is a unit-of-measure related bug.

[void]

In D135989#3859763, @nickdesaulniers wrote:

LGTM; please consider appending to the commit message something along the lines of "operations on the result of getTypeSize() are in bits, not bytes. Using 1 as the value for void* is the correct number of bytes, but we're doing arithmetic in bits." or something to denote this is a unit-of-measure related bug.

Done. Thanks!

[serge-sans-paille]

Thanks for clarifying!

clang/lib/Sema/SemaChecking.cpp

if (index.isUnsigned() || !index.isNegative()) {
     // access exceeds the array bounds. However we can still diagnose an array
     // access which precedes the array bounds.
     if (BaseType->isIncompleteType())
       return;
 
     llvm::APInt size = ArrayTy->getSize();
 
     if (BaseType != EffectiveType) {
-      // Make sure we're comparing apples to apples when comparing index to size
+      // Make sure we're comparing apples to apples when comparing index to
+      // size.
       uint64_t ptrarith_typesize = Context.getTypeSize(EffectiveType);
       uint64_t array_typesize = Context.getTypeSize(BaseType);
-      // Handle ptrarith_typesize being zero, such as when casting to void*
-      if (!ptrarith_typesize) ptrarith_typesize = 1;
+
+      // Handle ptrarith_typesize being zero, such as when casting to void*.
+      // Use the size in bits (what "getTypeSize()" returns) rather than bytes.
+      if (!ptrarith_typesize)
+        ptrarith_typesize = Context.getCharWidth();
+
       if (ptrarith_typesize != array_typesize) {
-        // There's a cast to a different size type involved
+        // There's a cast to a different size type involved.
         uint64_t ratio = array_typesize / ptrarith_typesize;
+
         // TODO: Be smarter about handling cases where array_typesize is not a
-        // multiple of ptrarith_typesize
+        // multiple of ptrarith_typesize.
         if (ptrarith_typesize * ratio == array_typesize)
           size *= llvm::APInt(size.getBitWidth(), ratio);
       }
     }
 
     if (size.getBitWidth() > index.getBitWidth())
       index = index.zext(size.getBitWidth());
     else if (size.getBitWidth() < index.getBitWidth())

clang/test/Sema/array-bounds-ptr-arith.c

 // RUN: %clang_cc1 -verify=expected -Warray-bounds-pointer-arithmetic %s
 // RUN: %clang_cc1 -verify=expected -Warray-bounds-pointer-arithmetic %s -fstrict-flex-arrays=0
 // RUN: %clang_cc1 -verify=expected,strict -Warray-bounds-pointer-arithmetic %s -fstrict-flex-arrays=2
 
 // Test case from PR10615
 struct ext2_super_block{
   unsigned char s_uuid[8]; // expected-note {{declared here}}
 };
-void* ext2_statfs (struct ext2_super_block *es,int a)
-{
-   return (void *)es->s_uuid + sizeof(int); // no-warning
+
+void* ext2_statfs (struct ext2_super_block *es,int a) {
+  return (void *)es->s_uuid + sizeof(int); // no-warning
 }
-void* broken (struct ext2_super_block *es,int a)
-{
-   return (void *)es->s_uuid + 80; // expected-warning {{refers past the end of the array}}
+void* broken (struct ext2_super_block *es,int a) {
+  return (void *)es->s_uuid + 9; // expected-warning {{the pointer incremented by 9 refers past the end of the array}}
 }
 
 // Test case reduced from PR11594
 struct S {
   int n;
 };
 void pr11594(struct S *s) {
   int a[10];