[Diagnostics] Silence -Wsizeof-array-div for character buffers

[jrtc27]

Summary

Character buffers are sometimes used to represent a pool of memory that
contains non-character objects, due to them being synonymous with a stream of
bytes on almost all modern architectures. Often, when interacting with hardware
devices, byte buffers are therefore used as an intermediary and so we can end
Character buffers are sometimes used to represent a pool of memory that
contains non-character objects, due to them being synonymous with a stream of
bytes on almost all modern architectures. Often, when interacting with hardware
devices, byte buffers are therefore used as an intermediary and so we can end
up generating lots of false-positives.

Moreover, due to the ability of character pointers to alias non-character
pointers, the strict aliasing violations that would generally be implied by the
calculations caught by the warning (if the calculation itself is in fact
correct) do not apply here, and so although the length calculation may be
wrong, that is the only possible issue.

[xbolva00]

I think this is reasonable change.

[jrtc27]

Having said that, the one warning this raised in FreeBSD (at least for our configuration) was a completely unnecessary use of a character buffer and could be cleaned up, so maybe this case is a useful one to catch. Does anyone else have an opinion on this?

[lebedev.ri]

There's not a single word in the patch's description.

[jrtc27]

In D68526#1696587, @lebedev.ri wrote:

There's not a single word in the patch's description.

If I were to add a description, it would be something like:

Character buffers are sometimes used to represent a pool of memory that
contains non-character objects, due to them being synonymous with a stream of
bytes on almost all modern architectures. Often, when interacting with hardware
devices, byte buffers are therefore used as an intermediary and so we can end
Character buffers are sometimes used to represent a pool of memory that
contains non-character objects, due to them being synonymous with a stream of
bytes on almost all modern architectures. Often, when interacting with hardware
devices, byte buffers are therefore used as an intermediary and so we can end
up generating lots of false-positives.

Moreover, due to the ability of character pointers to alias non-character
pointers, the strict aliasing violations that would generally be implied by the
calculations caught by the warning (if the calculation itself is in fact
correct) do not apply here, and so although the length calculation may be
wrong, that is the only possible issue.

But it seems that this first premise is actually wrong; at least in FreeBSD, and using a quick grep through Linux for obvious names (sizeof.*buf) /, sizeof.*buffer) / and sizeof.*data) /) I couldn't find one instance of an array where the types didn't match, but that was for u64 vs u32 so isn't even covered by this. Maybe it was true in the past, but these days people seem to be diligent about using the right types, with proper unions when needed.

The aliasing point still applies (in that this case is slightly less dangerous), but given it draws attention to code that may be able to be written more nicely with better types, I'd be inclined to abandon this. Unless people come screaming that we're spewing too many warnings for their code, that is, of course.

[xbolva00]

I think I saw some false positives with char buffers, so I am not against this patch personally.

[xbolva00]

@tkanis wrote in post commit review:

“What do you think about also not emitting the warning if the lhs sizeof is an array of signed or unsigned char? The warning wants the rhs sizeof to be sizeof(char) which is 1, and dividing by that doesn't really make sense. ”

I agree with him. I think you can land this patch.

[thakis]

Thanks, I think this is a good change. It fixes a false positive in wayland and one somewhere in Chromium's windows sandbox. See the commit thread of r371605 for examples.

[jrtc27]

Rebased

clang/lib/Sema/SemaExpr.cpp

if (const auto *DRE = dyn_cast<DeclRefExpr>(LHSArg)) {
       if (const ValueDecl *LHSArgDecl = DRE->getDecl())
         S.Diag(LHSArgDecl->getLocation(), diag::note_pointer_declared_here)
             << LHSArgDecl;
     }
   } else if (const auto *ArrayTy = S.Context.getAsArrayType(LHSTy)) {
     QualType ArrayElemTy = ArrayTy->getElementType();
     if (ArrayElemTy != S.Context.getBaseElementType(ArrayTy) ||
         ArrayElemTy->isDependentType() || RHSTy->isDependentType() ||
+        ArrayElemTy->isCharType() ||
         S.Context.getTypeSize(ArrayElemTy) == S.Context.getTypeSize(RHSTy))
       return;
     S.Diag(Loc, diag::warn_division_sizeof_array)
         << LHSArg->getSourceRange() << ArrayElemTy << RHSTy;
     if (const auto *DRE = dyn_cast<DeclRefExpr>(LHSArg)) {
       if (const ValueDecl *LHSArgDecl = DRE->getDecl())
         S.Diag(LHSArgDecl->getLocation(), diag::note_array_declared_here)
             << LHSArgDecl;

clang/test/Sema/div-sizeof-array.cpp

void test(void) {
   // expected-note@-1 {{place parentheses around the 'sizeof(short)' expression to silence this warning}}
   int a6 = sizeof(arr) / sizeof(int32);
   int a7 = sizeof(arr) / sizeof(int);
   int a9 = sizeof(arr) / sizeof(unsigned int);
   const char arr3[2] = "A";
   int a10 = sizeof(arr3) / sizeof(char);
   int a11 = sizeof(arr2) / (sizeof(unsigned));
   int a12 = sizeof(arr) / (sizeof(short));
+  int a13 = sizeof(arr3) / sizeof(p);
+  int a14 = sizeof(arr3) / sizeof(int);
 
   int arr4[10][12];
   int b1 = sizeof(arr4) / sizeof(arr2[12]);
   int b2 = sizeof(arr4) / sizeof(int *);
   int b3 = sizeof(arr4) / sizeof(short *);
   int arr5[][5] = {
       {1, 2, 3, 4, 5},
       {6, 7, 8, 9, 0},
   };
   int c1 = sizeof(arr5) / sizeof(*arr5);
   int c2 = sizeof(arr5) / sizeof(**arr5);
   int c3 = sizeof(arr5) / sizeof(int);
 
   float m[4][4];
   int d1 = sizeof(m) / sizeof(**m);
 }