[Sema] Adds the pointer-to-int-cast diagnostic

[Mordante]

Summary

Converting a pointer to an integer whose result cannot represented in the integer type is undefined behavior is C and prohibited in C++. C++ already has a diagnostic when casting. This adds a diagnostic for C.

The diagnostic is not enabled by default due to the number of diagnostics it triggered while running the tests.

Since this diagnostic uses the range of the conversion it also modifies int-to-pointer-cast diagnostic to use a range.

Fixes PR8718: No warning on casting between pointer and non-pointer-sized int

[xbolva00]

The diagnostic is not enabled by default

But GCC enables it for C even without "-Wall or -Wextra". Clang should follow it..

[merge_guards_bot]

Unit tests: pass. 61248 tests passed, 0 failed and 736 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[Mordante]

In D72231#1804929, @xbolva00 wrote:

The diagnostic is not enabled by default

But GCC enables it for C even without "-Wall or -Wextra". Clang should follow it..

I'll have a look what the impact is. When I tested it about 20 tests failed.

[Mordante]

While looking at the test failures I noticed the tests pointer_to_integral_type_conv in clang/test/Sema/MicrosoftExtensions.c were not tested. Will look into it later.

[rjmccall]

It's not unusual for new warnings to require changes to other tests. I agree with enabling this by default.

[Mordante]

• Enabled the warning by default
• Added an Microsoft extension, the unit tests already expected this behaviour

[rsmith]

Looks fine.

Would it make sense to put the MS extension warning into the -Wpointer-to-int-cast group so that we can control this warning consistently across platforms? You could get that effect by introducing a new warning group (eg, "-Wmicrosoft-pointer-to-int-cast") containing just the ExtWarn warning, and putting that group in both MicrosoftCast and PointerToIntCast.

clang/test/Sema/cast.c

1

Don't specify the warning flag here, so that we have some test coverage that this is enabled by default.

[Mordante]

Thanks for the review.

I'll have a look at your suggestion for a follow-up patch.

clang/test/Sema/cast.c

1

Good catch! This was needed before the diagnostic was enabled by default.

[Mordante]

This revision caused issue for the MSVC build bot. I created a followup patch D74694.

[nathanchance]

There appear to a be semantic difference between GCC and clang with the current version of this patch which results in a lot of additional warnings in the Linux kernel: https://godbolt.org/z/eHFJd8

[rjmccall]

In D72231#1878528, @nathanchance wrote:

There appear to a be semantic difference between GCC and clang with the current version of this patch which results in a lot of additional warnings in the Linux kernel: https://godbolt.org/z/eHFJd8

Warning about casting to an enum seems clearly correct and in scope for this warning. Warning about casting to _Bool seems clearly incorrect and should not be warned about at all.

[Mordante]

In D72231#1879347, @rjmccall wrote:

In D72231#1878528, @nathanchance wrote:

There appear to a be semantic difference between GCC and clang with the current version of this patch which results in a lot of additional warnings in the Linux kernel: https://godbolt.org/z/eHFJd8

Warning about casting to an enum seems clearly correct and in scope for this warning. Warning about casting to _Bool seems clearly incorrect and should not be warned about at all.

Agreed. I'll look at a followup patch to remove the warning for _Bool.

[nathanchance]

In D72231#1879347, @rjmccall wrote:

Warning about casting to an enum seems clearly correct and in scope for this warning. Warning about casting to _Bool seems clearly incorrect and should not be warned about at all.

Fair enough. There are 97 unique instances of the enum variant of the warning across the various configs that I test so I will start sending patches to add uintptr_t casts to silence it.

https://gist.github.com/nathanchance/2c2892e9e4b411fa78770ed3624812b4

[nickdesaulniers]

In D72231#1879347, @rjmccall wrote:

In D72231#1878528, @nathanchance wrote:

There appear to a be semantic difference between GCC and clang with the current version of this patch which results in a lot of additional warnings in the Linux kernel: https://godbolt.org/z/eHFJd8

Warning about casting to an enum seems clearly correct and in scope for this warning. Warning about casting to _Bool seems clearly incorrect and should not be warned about at all.

Maybe we should only warn if the size of the void* is smaller than the size of the enum? (32b void*, 64b enum)? https://godbolt.org/z/oAts-u

Otherwise this warning creates a massive mess for us to clean up, and I suspect Linux kernel developers will just end up disabling the warning.

[rjmccall]

In D72231#1881760, @nickdesaulniers wrote:

In D72231#1879347, @rjmccall wrote:

In D72231#1878528, @nathanchance wrote:

There appear to a be semantic difference between GCC and clang with the current version of this patch which results in a lot of additional warnings in the Linux kernel: https://godbolt.org/z/eHFJd8

Warning about casting to an enum seems clearly correct and in scope for this warning. Warning about casting to _Bool seems clearly incorrect and should not be warned about at all.

Maybe we should only warn if the size of the void* is smaller than the size of the enum? (32b void*, 64b enum)? https://godbolt.org/z/oAts-u

Otherwise this warning creates a massive mess for us to clean up, and I suspect Linux kernel developers will just end up disabling the warning.

If deployment is easier if we split out a subgroup that we can turn off, that seems fine. But I don't see any good abstract justification for warning about a cast to int and not a cast to an int-sized enum. What would the reasoning be, just that the latter "couldn't possibly" be intended to preserve the original pointer value, so it must be an opaque value being represented as a void*? That seems pretty weak to me.

[nickdesaulniers]

In D72231#1881784, @rjmccall wrote:

In D72231#1881760, @nickdesaulniers wrote:

In D72231#1879347, @rjmccall wrote:

In D72231#1878528, @nathanchance wrote:

There appear to a be semantic difference between GCC and clang with the current version of this patch which results in a lot of additional warnings in the Linux kernel: https://godbolt.org/z/eHFJd8

Warning about casting to an enum seems clearly correct and in scope for this warning. Warning about casting to _Bool seems clearly incorrect and should not be warned about at all.

Maybe we should only warn if the size of the void* is smaller than the size of the enum? (32b void*, 64b enum)? https://godbolt.org/z/oAts-u

Otherwise this warning creates a massive mess for us to clean up, and I suspect Linux kernel developers will just end up disabling the warning.

If deployment is easier if we split out a subgroup that we can turn off, that seems fine. But I don't see any good abstract justification for warning about a cast to int and not a cast to an int-sized enum. What would the reasoning be, just that the latter "couldn't possibly" be intended to preserve the original pointer value, so it must be an opaque value being represented as a void*? That seems pretty weak to me.

Less about enums, more about casts to/from void*, since you might use that in place of a union that would be too large to describe. Specifically, this struct is used throughout the kernel for most drivers: https://elixir.bootlin.com/linux/v5.5.4/source/include/linux/mod_devicetable.h#L260 It is exceedingly common to stuff whatever data in there: https://elixir.bootlin.com/linux/v5.5.4/source/drivers/ata/ahci_brcm.c#L428 so long as the driver is careful not to reinterpret the data as the incorrect type. Describing such a union for ever possible enum packed in there would not be fun.

[rjmccall]

In D72231#1881797, @nickdesaulniers wrote:

In D72231#1881784, @rjmccall wrote:

In D72231#1881760, @nickdesaulniers wrote:

In D72231#1879347, @rjmccall wrote:

In D72231#1878528, @nathanchance wrote:

There appear to a be semantic difference between GCC and clang with the current version of this patch which results in a lot of additional warnings in the Linux kernel: https://godbolt.org/z/eHFJd8

Warning about casting to an enum seems clearly correct and in scope for this warning. Warning about casting to _Bool seems clearly incorrect and should not be warned about at all.

Maybe we should only warn if the size of the void* is smaller than the size of the enum? (32b void*, 64b enum)? https://godbolt.org/z/oAts-u

Otherwise this warning creates a massive mess for us to clean up, and I suspect Linux kernel developers will just end up disabling the warning.

If deployment is easier if we split out a subgroup that we can turn off, that seems fine. But I don't see any good abstract justification for warning about a cast to int and not a cast to an int-sized enum. What would the reasoning be, just that the latter "couldn't possibly" be intended to preserve the original pointer value, so it must be an opaque value being represented as a void*? That seems pretty weak to me.

Less about enums, more about casts to/from void*, since you might use that in place of a union that would be too large to describe. Specifically, this struct is used throughout the kernel for most drivers: https://elixir.bootlin.com/linux/v5.5.4/source/include/linux/mod_devicetable.h#L260 It is exceedingly common to stuff whatever data in there: https://elixir.bootlin.com/linux/v5.5.4/source/drivers/ata/ahci_brcm.c#L428 so long as the driver is careful not to reinterpret the data as the incorrect type. Describing such a union for ever possible enum packed in there would not be fun.

No, I understand the pattern, but they must have already done some sort of pass over the code to make it warning-clean when they're working with a smaller integer type. Or do they just in practice never store smaller integers in there, whereas it's hard to control size with an enum?

[nickdesaulniers]

In D72231#1881855, @rjmccall wrote:

No, I understand the pattern, but they must have already done some sort of pass over the code to make it warning-clean when they're working with a smaller integer type. Or do they just in practice never store smaller integers in there, whereas it's hard to control size with an enum?

Unsure.

[nathanchance]

In D72231#1881855, @rjmccall wrote:

In D72231#1881797, @nickdesaulniers wrote:

In D72231#1881784, @rjmccall wrote:

In D72231#1881760, @nickdesaulniers wrote:

In D72231#1879347, @rjmccall wrote:

In D72231#1878528, @nathanchance wrote:

There appear to a be semantic difference between GCC and clang with the current version of this patch which results in a lot of additional warnings in the Linux kernel: https://godbolt.org/z/eHFJd8

Warning about casting to an enum seems clearly correct and in scope for this warning. Warning about casting to _Bool seems clearly incorrect and should not be warned about at all.

Maybe we should only warn if the size of the void* is smaller than the size of the enum? (32b void*, 64b enum)? https://godbolt.org/z/oAts-u

Otherwise this warning creates a massive mess for us to clean up, and I suspect Linux kernel developers will just end up disabling the warning.

If deployment is easier if we split out a subgroup that we can turn off, that seems fine. But I don't see any good abstract justification for warning about a cast to int and not a cast to an int-sized enum. What would the reasoning be, just that the latter "couldn't possibly" be intended to preserve the original pointer value, so it must be an opaque value being represented as a void*? That seems pretty weak to me.

Less about enums, more about casts to/from void*, since you might use that in place of a union that would be too large to describe. Specifically, this struct is used throughout the kernel for most drivers: https://elixir.bootlin.com/linux/v5.5.4/source/include/linux/mod_devicetable.h#L260 It is exceedingly common to stuff whatever data in there: https://elixir.bootlin.com/linux/v5.5.4/source/drivers/ata/ahci_brcm.c#L428 so long as the driver is careful not to reinterpret the data as the incorrect type. Describing such a union for ever possible enum packed in there would not be fun.

No, I understand the pattern, but they must have already done some sort of pass over the code to make it warning-clean when they're working with a smaller integer type. Or do they just in practice never store smaller integers in there, whereas it's hard to control size with an enum?

Yes, if the data is a regular int, rather than an enum, all of the callsites either cast to long or uintptr_t (which is typedef'd in the kernel to unsigned long). There are a lot fewer of those spots in the kernel (at least from my super quick rg search), most of the spots use an enum, maybe to purposefully avoid this warning? Most, if not all the sites, only store a number that is less than 5 because they use that number to determine exactly which device is present from the match data so the driver can handle different quirks with things like case statements.

[rjmccall]

Okay. Can we raise this with the kernel folks instead of just assuming they'll be opposed? An obvious patch to fix a few dozen places where they're hit by a warning they intentionally enabled really doesn't seem like a burden. If they push back, fine, we can enable the warning without covering enums.

[Mordante]

In D72231#1882658, @rjmccall wrote:

Okay. Can we raise this with the kernel folks instead of just assuming they'll be opposed? An obvious patch to fix a few dozen places where they're hit by a warning they intentionally enabled really doesn't seem like a burden. If they push back, fine, we can enable the warning without covering enums.

In the original diff the warning was opt in, in the final version it's enabled by default. So they didn't enable the warning intentionally. I agree; if the kernel folks rather not change their code I can create an option to disable the warning for enums, just like it can already be disabled for void*s.

clang/docs/ReleaseNotes.rst

 - -Wtautological-bitwise-compare is a new warning group.  This group has the
   current warning which diagnoses the tautological comparison of a bitwise
   operation and a constant. The group also has the new warning which diagnoses
   when a bitwise-or with a non-negative value is converted to a bool, since
   that bool will always be true.
 - -Wbitwise-conditional-parentheses will warn on operator precedence issues
   when mixing bitwise-and (&) and bitwise-or (|) operator with the
   conditional operator (?:).
+- -Wpointer-to-int-cast is a new warning group. This group warns about C-style
+  casts of pointers to a integer type too small to hold all possible values.
 
 Non-comprehensive list of changes in this release
 -------------------------------------------------
 
 * In both C and C++ (C17 ``6.5.6p8``, C++ ``[expr.add]``), pointer arithmetic is
   only permitted within arrays. In particular, the behavior of a program is not
   defined if it adds a non-zero offset (or in C, any offset) to a null pointer,
   or if it forms a null pointer by subtracting an integer from a non-null

clang/include/clang/Basic/DiagnosticGroups.td

 
 def TypeSafety : DiagGroup<"type-safety">;
 
 def IncompatibleExceptionSpec : DiagGroup<"incompatible-exception-spec">;
 
 def IntToVoidPointerCast : DiagGroup<"int-to-void-pointer-cast">;
 def IntToPointerCast : DiagGroup<"int-to-pointer-cast",
                                  [IntToVoidPointerCast]>;
+def VoidPointerToIntCast : DiagGroup<"void-pointer-to-int-cast">;
+def PointerToIntCast : DiagGroup<"pointer-to-int-cast",
+                                 [VoidPointerToIntCast]>;
 
 def Move : DiagGroup<"move", [
     PessimizingMove,
     RedundantMove,
     ReturnStdMove,
     SelfMove
   ]>;
 

clang/include/clang/Basic/DiagnosticSemaKinds.td

 // Having a separate warning flag allows users to control the warning
 // for their workflow.
 def warn_int_to_pointer_cast : Warning<
   "cast to %1 from smaller integer type %0">,
   InGroup<IntToPointerCast>;
 def warn_int_to_void_pointer_cast : Warning<
   "cast to %1 from smaller integer type %0">,
   InGroup<IntToVoidPointerCast>;
+def warn_pointer_to_int_cast : Warning<
+  "cast to smaller integer type %1 from %0">,
+  InGroup<PointerToIntCast>, DefaultIgnore;
+def warn_void_pointer_to_int_cast : Warning<
+  "cast to smaller integer type %1 from %0">,
+  InGroup<VoidPointerToIntCast>, DefaultIgnore;
 
 def warn_attribute_ignored_for_field_of_type : Warning<
   "%0 attribute ignored for field of type %1">,
   InGroup<IgnoredAttributes>;
 def warn_no_underlying_type_specified_for_enum_bitfield : Warning<
   "enums in the Microsoft ABI are signed integers by default; consider giving "
   "the enum %0 an unsigned underlying type to make this code portable">,
   InGroup<SignedEnumBitfield>, DefaultIgnore;

clang/lib/Sema/SemaCast.cpp

static void DiagnoseCallingConvCast(Sema &Self, const ExprResult &SrcExpr,
   StringRef AttrSpelling = PP.getLastMacroWithSpelling(NameLoc, AttrTokens);
   if (!AttrSpelling.empty())
     CCAttrText = AttrSpelling;
   OS << ' ';
   Self.Diag(NameLoc, diag::note_change_calling_conv_fixit)
       << FD << DstCCName << FixItHint::CreateInsertion(NameLoc, CCAttrText);
 }
 
-static void checkIntToPointerCast(bool CStyle, SourceLocation Loc,
+static void checkIntToPointerCast(bool CStyle, const SourceRange &OpRange,
                                   const Expr *SrcExpr, QualType DestType,
                                   Sema &Self) {
   QualType SrcType = SrcExpr->getType();
 
   // Not warning on reinterpret_cast, boolean, constant expressions, etc
   // are not explicit design choices, but consistent with GCC's behavior.
   // Feel free to modify them if you've reason/evidence for an alternative.
   if (CStyle && SrcType->isIntegralType(Self.Context)
       && !SrcType->isBooleanType()
       && !SrcType->isEnumeralType()
       && !SrcExpr->isIntegerConstantExpr(Self.Context)
       && Self.Context.getTypeSize(DestType) >
          Self.Context.getTypeSize(SrcType)) {
     // Separate between casts to void* and non-void* pointers.
     // Some APIs use (abuse) void* for something like a user context,
     // and often that value is an integer even if it isn't a pointer itself.
     // Having a separate warning flag allows users to control the warning
     // for their workflow.
     unsigned Diag = DestType->isVoidPointerType() ?
                       diag::warn_int_to_void_pointer_cast
                     : diag::warn_int_to_pointer_cast;
-    Self.Diag(Loc, Diag) << SrcType << DestType;
+    Self.Diag(OpRange.getBegin(), Diag) << SrcType << DestType << OpRange;
   }
 }
 
 static bool fixOverloadedReinterpretCastExpr(Sema &Self, QualType DestType,
                                              ExprResult &Result) {
   // We can only fix an overloaded reinterpret_cast if
   // - it is a template with explicit arguments that resolves to an lvalue
   //   unambiguously, or
if ((Self.Context.getTypeSize(SrcType) >
       return TC_Failed;
     }
     Kind = CK_PointerToIntegral;
     return TC_Success;
   }
 
   if (SrcType->isIntegralOrEnumerationType()) {
     assert(destIsPtr && "One type must be a pointer");
-    checkIntToPointerCast(CStyle, OpRange.getBegin(), SrcExpr.get(), DestType,
-                          Self);
+    checkIntToPointerCast(CStyle, OpRange, SrcExpr.get(), DestType, Self);
     // C++ 5.2.10p5: A value of integral or enumeration type can be explicitly
     //   converted to a pointer.
     // C++ 5.2.10p9: [Note: ...a null pointer constant of integral type is not
     //   necessarily converted to a null pointer value.]
     Kind = CK_IntegralToPointer;
     return TC_Success;
   }
 
void CastOperation::CheckCStyleCast() {
   if (!DestType->isArithmeticType()) {
     if (!SrcType->isIntegralType(Self.Context) && SrcType->isArithmeticType()) {
       Self.Diag(SrcExpr.get()->getExprLoc(),
                 diag::err_cast_pointer_from_non_pointer_int)
         << SrcType << SrcExpr.get()->getSourceRange();
       SrcExpr = ExprError();
       return;
     }
-    checkIntToPointerCast(/* CStyle */ true, OpRange.getBegin(), SrcExpr.get(),
-                          DestType, Self);
+    checkIntToPointerCast(/* CStyle */ true, OpRange, SrcExpr.get(), DestType,
+                          Self);
   } else if (!SrcType->isArithmeticType()) {
     if (!DestType->isIntegralType(Self.Context) &&
         DestType->isArithmeticType()) {
       Self.Diag(SrcExpr.get()->getBeginLoc(),
                 diag::err_cast_pointer_to_non_pointer_int)
           << DestType << SrcExpr.get()->getSourceRange();
       SrcExpr = ExprError();
       return;
     }
+
+    if ((Self.Context.getTypeSize(SrcType) >
+         Self.Context.getTypeSize(DestType))) {
+      // C 6.3.2.3p6: Any pointer type may be converted to an integer type.
+      // Except as previously specified, the result is implementation-defined.
+      // If the result cannot be represented in the integer type, the behavior
+      // is undefined. The result need not be in the range of values of any
+      // integer type.
+      unsigned Diag = SrcType->isVoidPointerType()
+                          ? diag::warn_void_pointer_to_int_cast
+                          : diag::warn_pointer_to_int_cast;
+      Self.Diag(OpRange.getBegin(), Diag) << SrcType << DestType << OpRange;
+    }
   }
 
   if (Self.getLangOpts().OpenCL &&
       !Self.getOpenCLOptions().isEnabled("cl_khr_fp16")) {
     if (DestType->isHalfType()) {
       Self.Diag(SrcExpr.get()->getBeginLoc(), diag::err_opencl_cast_to_half)
           << DestType << SrcExpr.get()->getSourceRange();
       SrcExpr = ExprError();

clang/test/Sema/cast.c

-// RUN: %clang_cc1 -fsyntax-only -triple x86_64-unknown-unknown %s -verify
+// RUN: %clang_cc1 -fsyntax-only -triple x86_64-unknown-unknown -Wpointer-to-int-cast %s -verify
 
 typedef struct { unsigned long bits[(((1) + (64) - 1) / (64))]; } cpumask_t;
 cpumask_t x;
 void foo() {
   (void)x;
 }
 void bar() {
   char* a;
   double b;
   b = (double)a; // expected-error {{pointer cannot be cast to type}}
   a = (char*)b; // expected-error {{cannot be cast to a pointer type}}
 }
 
 long bar1(long *next) {
         return (long)(*next)++;  
 }
 
 typedef _Bool Bool;
 typedef int Int;
void testCDouble(CDouble v) {
   (void) (Double) v;
   (void) (CInt) v;
   (void) (CLong) v;
   (void) (CFloat) v;
   (void) (CDouble) v;
 }
 
 void testVoidPtr(VoidPtr v) {
-  (void) (Bool) v;
-  (void) (Int) v;
+  (void) (Bool) v; // expected-warning{{cast to smaller integer type 'Bool' (aka '_Bool') from 'VoidPtr' (aka 'void *')}}
+  (void) (Int) v; // expected-warning{{cast to smaller integer type 'Int' (aka 'int') from 'VoidPtr' (aka 'void *')}}
   (void) (Long) v;
   (void) (VoidPtr) v;
   (void) (CharPtr) v;
+  // Test that casts to void* can be controlled separately
+  // from other -Wpointer-to-int-cast warnings.
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wvoid-pointer-to-int-cast"
+  (void) (Bool) v; // no-warning
+#pragma clang diagnostic pop
 }
 
 void testCharPtr(CharPtr v) {
-  (void) (Bool) v;
-  (void) (Int) v;
+  (void) (Bool) v; // expected-warning{{cast to smaller integer type 'Bool' (aka '_Bool') from 'CharPtr' (aka 'char *')}}
+  (void) (Int) v; // expected-warning{{cast to smaller integer type 'Int' (aka 'int') from 'CharPtr' (aka 'char *')}}
   (void) (Long) v;
   (void) (VoidPtr) v;
   (void) (CharPtr) v;
+  // Test that casts to void* can be controlled separately
+  // from other -Wpointer-to-int-cast warnings.
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wvoid-pointer-to-int-cast"
+  (void) (Bool) v; // expected-warning{{cast to smaller integer type 'Bool' (aka '_Bool') from 'CharPtr' (aka 'char *')}}
+#pragma clang diagnostic pop
 }
 
 typedef enum { x_a, x_b } X;
 void *intToPointerCast2(X x) {
   return (void*)x;
 }
 
 void *intToPointerCast3() {
   return (void*)(1 + 3);
 }