[PATCH] D72231: [Sema] Adds the pointer-to-int-cast diagnostic

[Mark de Wever via Phabricator via cfe-commits]

Mordante added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72231/new/

https://reviews.llvm.org/D72231



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72231: [Sema] Adds the pointer-to-int-cast diagnostic

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72231/new/

https://reviews.llvm.org/D72231



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72231: [Sema] Adds the pointer-to-int-cast diagnostic

[Nathan Chancellor via Phabricator via cfe-commits]

nathanchance added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72231/new/

https://reviews.llvm.org/D72231



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72231: [Sema] Adds the pointer-to-int-cast diagnostic

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72231/new/

https://reviews.llvm.org/D72231



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72231: [Sema] Adds the pointer-to-int-cast diagnostic

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72231/new/

https://reviews.llvm.org/D72231



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72231: [Sema] Adds the pointer-to-int-cast diagnostic

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72231/new/

https://reviews.llvm.org/D72231



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72231: [Sema] Adds the pointer-to-int-cast diagnostic

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72231/new/

https://reviews.llvm.org/D72231



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72231: [Sema] Adds the pointer-to-int-cast diagnostic

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72231/new/

https://reviews.llvm.org/D72231



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72231: [Sema] Adds the pointer-to-int-cast diagnostic

[Nathan Chancellor via Phabricator via cfe-commits]

nathanchance added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72231/new/

https://reviews.llvm.org/D72231



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72231: [Sema] Adds the pointer-to-int-cast diagnostic

[Mark de Wever via Phabricator via cfe-commits]

Mordante added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72231/new/

https://reviews.llvm.org/D72231



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72231: [Sema] Adds the pointer-to-int-cast diagnostic

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72231/new/

https://reviews.llvm.org/D72231



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72231: [Sema] Adds the pointer-to-int-cast diagnostic

[Nathan Chancellor via Phabricator via cfe-commits]

nathanchance added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72231/new/

https://reviews.llvm.org/D72231



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72231: [Sema] Adds the pointer-to-int-cast diagnostic

[Mark de Wever via Phabricator via cfe-commits]

Mordante added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72231/new/

https://reviews.llvm.org/D72231



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72231: [Sema] Adds the pointer-to-int-cast diagnostic

[Mark de Wever via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Mordante marked an inline comment as done.
Closed by commit rG9658d895c81a: [Sema] Adds the pointer-to-int-cast diagnostic 
(authored by Mordante).

Changed prior to commit:
  https://reviews.llvm.org/D72231?vs=242393=244872#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72231/new/

https://reviews.llvm.org/D72231

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaCast.cpp
  clang/test/Analysis/bstring.c
  clang/test/Analysis/casts.c
  clang/test/Analysis/misc-ps.c
  clang/test/Analysis/misc-ps.m
  clang/test/Analysis/ptr-arith.c
  clang/test/Analysis/svalbuilder-logic.c
  clang/test/Analysis/symbol-reaper.c
  clang/test/CodeGen/const-init.c
  clang/test/Sema/MicrosoftExtensions.c
  clang/test/Sema/block-return.c
  clang/test/Sema/cast.c
  clang/test/Sema/const-eval.c
  clang/test/Sema/init.c
  clang/test/Sema/offsetof.c
  clang/test/Sema/static-init.c
  clang/test/Sema/struct-decl.c
  clang/test/SemaObjC/arc.m
  clang/test/SemaObjC/gcc-cast-ext.m
  clang/test/SemaObjC/protocol-archane.m

Index: clang/test/SemaObjC/protocol-archane.m
===
--- clang/test/SemaObjC/protocol-archane.m
+++ clang/test/SemaObjC/protocol-archane.m
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -Wno-objc-root-class %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wno-pointer-to-int-cast -Wno-objc-root-class %s
 // rdar://5986251
 
 @protocol SomeProtocol
Index: clang/test/SemaObjC/gcc-cast-ext.m
===
--- clang/test/SemaObjC/gcc-cast-ext.m
+++ clang/test/SemaObjC/gcc-cast-ext.m
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -verify -fms-extensions -Wno-objc-root-class %s
+// RUN: %clang_cc1 -verify -Wno-pointer-to-int-cast -Wno-objc-root-class %s
 @class NSInvocation, NSMethodSignature, NSCoder, NSString, NSEnumerator;
 typedef struct _NSRange { } NSRange;
 
Index: clang/test/SemaObjC/arc.m
===
--- clang/test/SemaObjC/arc.m
+++ clang/test/SemaObjC/arc.m
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -triple x86_64-apple-darwin11 -fobjc-runtime-has-weak -fsyntax-only -fobjc-arc -fblocks -verify -Wno-objc-root-class %s
-// RUN: not %clang_cc1 -triple x86_64-apple-darwin11 -fobjc-runtime-has-weak -fsyntax-only -fobjc-arc -fblocks -Wno-objc-root-class -fdiagnostics-parseable-fixits %s 2>&1
+// RUN: %clang_cc1 -triple x86_64-apple-darwin11 -fobjc-runtime-has-weak -fsyntax-only -fobjc-arc -fblocks -verify -Wno-pointer-to-int-cast -Wno-objc-root-class %s
+// RUN: not %clang_cc1 -triple x86_64-apple-darwin11 -fobjc-runtime-has-weak -fsyntax-only -fobjc-arc -fblocks -Wno-pointer-to-int-cast -Wno-objc-root-class -fdiagnostics-parseable-fixits %s 2>&1
 
 typedef unsigned long NSUInteger;
 typedef const void * CFTypeRef;
Index: clang/test/Sema/struct-decl.c
===
--- clang/test/Sema/struct-decl.c
+++ clang/test/Sema/struct-decl.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -Wno-pointer-to-int-cast -fsyntax-only -verify %s
 // PR3459
 struct bar {
   char n[1];
Index: clang/test/Sema/static-init.c
===
--- clang/test/Sema/static-init.c
+++ clang/test/Sema/static-init.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -Wno-bool-conversion %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wno-pointer-to-int-cast -Wno-bool-conversion %s
 
 typedef __typeof((int*) 0 - (int*) 0) intptr_t;
 
Index: clang/test/Sema/offsetof.c
===
--- clang/test/Sema/offsetof.c
+++ clang/test/Sema/offsetof.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -Wno-pointer-to-int-cast -fsyntax-only -verify %s
 
 #define offsetof(TYPE, MEMBER) __builtin_offsetof (TYPE, MEMBER)
 
Index: clang/test/Sema/init.c
===
--- clang/test/Sema/init.c
+++ clang/test/Sema/init.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -verify -fsyntax-only -ffreestanding
+// RUN: %clang_cc1 %s -Wno-pointer-to-int-cast -verify -fsyntax-only -ffreestanding
 
 #include 
 #include 
Index: clang/test/Sema/const-eval.c
===
--- clang/test/Sema/const-eval.c
+++ clang/test/Sema/const-eval.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -triple x86_64-linux %s -Wno-tautological-pointer-compare
+// RUN: %clang_cc1 -fsyntax-only -verify -triple x86_64-linux %s -Wno-tautological-pointer-compare -Wno-pointer-to-int-cast
 
 #define EVAL_EXPR(testno, expr) int test##testno = sizeof(struct{char qq[expr];});
 int x;
Index: 

[PATCH] D72231: [Sema] Adds the pointer-to-int-cast diagnostic

[Mark de Wever via Phabricator via cfe-commits]

Mordante marked 2 inline comments as done.
Mordante added a comment.
Herald added a subscriber: martong.

Thanks for the review.

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




Comment at: clang/test/Sema/cast.c:1
-// 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
 

rsmith wrote:
> Don't specify the warning flag here, so that we have some test coverage that 
> this is enabled by default.
Good catch! This was needed before the diagnostic was enabled by default.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72231/new/

https://reviews.llvm.org/D72231



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72231: [Sema] Adds the pointer-to-int-cast diagnostic

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

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




Comment at: clang/test/Sema/cast.c:1
-// 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
 

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72231/new/

https://reviews.llvm.org/D72231



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72231: [Sema] Adds the pointer-to-int-cast diagnostic

[Mark de Wever via Phabricator via cfe-commits]

Mordante updated this revision to Diff 242393.
Mordante added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72231/new/

https://reviews.llvm.org/D72231

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaCast.cpp
  clang/test/Analysis/bstring.c
  clang/test/Analysis/casts.c
  clang/test/Analysis/misc-ps.c
  clang/test/Analysis/misc-ps.m
  clang/test/Analysis/ptr-arith.c
  clang/test/Analysis/svalbuilder-logic.c
  clang/test/Analysis/symbol-reaper.c
  clang/test/CodeGen/const-init.c
  clang/test/Sema/MicrosoftExtensions.c
  clang/test/Sema/block-return.c
  clang/test/Sema/cast.c
  clang/test/Sema/const-eval.c
  clang/test/Sema/init.c
  clang/test/Sema/offsetof.c
  clang/test/Sema/static-init.c
  clang/test/Sema/struct-decl.c
  clang/test/SemaObjC/arc.m
  clang/test/SemaObjC/gcc-cast-ext.m
  clang/test/SemaObjC/protocol-archane.m

Index: clang/test/SemaObjC/protocol-archane.m
===
--- clang/test/SemaObjC/protocol-archane.m
+++ clang/test/SemaObjC/protocol-archane.m
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -Wno-objc-root-class %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wno-pointer-to-int-cast -Wno-objc-root-class %s
 // rdar://5986251
 
 @protocol SomeProtocol
Index: clang/test/SemaObjC/gcc-cast-ext.m
===
--- clang/test/SemaObjC/gcc-cast-ext.m
+++ clang/test/SemaObjC/gcc-cast-ext.m
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -verify -fms-extensions -Wno-objc-root-class %s
+// RUN: %clang_cc1 -verify -Wno-pointer-to-int-cast -Wno-objc-root-class %s
 @class NSInvocation, NSMethodSignature, NSCoder, NSString, NSEnumerator;
 typedef struct _NSRange { } NSRange;
 
Index: clang/test/SemaObjC/arc.m
===
--- clang/test/SemaObjC/arc.m
+++ clang/test/SemaObjC/arc.m
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -triple x86_64-apple-darwin11 -fobjc-runtime-has-weak -fsyntax-only -fobjc-arc -fblocks -verify -Wno-objc-root-class %s
-// RUN: not %clang_cc1 -triple x86_64-apple-darwin11 -fobjc-runtime-has-weak -fsyntax-only -fobjc-arc -fblocks -Wno-objc-root-class -fdiagnostics-parseable-fixits %s 2>&1
+// RUN: %clang_cc1 -triple x86_64-apple-darwin11 -fobjc-runtime-has-weak -fsyntax-only -fobjc-arc -fblocks -verify -Wno-pointer-to-int-cast -Wno-objc-root-class %s
+// RUN: not %clang_cc1 -triple x86_64-apple-darwin11 -fobjc-runtime-has-weak -fsyntax-only -fobjc-arc -fblocks -Wno-pointer-to-int-cast -Wno-objc-root-class -fdiagnostics-parseable-fixits %s 2>&1
 
 typedef unsigned long NSUInteger;
 typedef const void * CFTypeRef;
Index: clang/test/Sema/struct-decl.c
===
--- clang/test/Sema/struct-decl.c
+++ clang/test/Sema/struct-decl.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -Wno-pointer-to-int-cast -fsyntax-only -verify %s
 // PR3459
 struct bar {
   char n[1];
Index: clang/test/Sema/static-init.c
===
--- clang/test/Sema/static-init.c
+++ clang/test/Sema/static-init.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -Wno-bool-conversion %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wno-pointer-to-int-cast -Wno-bool-conversion %s
 
 typedef __typeof((int*) 0 - (int*) 0) intptr_t;
 
Index: clang/test/Sema/offsetof.c
===
--- clang/test/Sema/offsetof.c
+++ clang/test/Sema/offsetof.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -Wno-pointer-to-int-cast -fsyntax-only -verify %s
 
 #define offsetof(TYPE, MEMBER) __builtin_offsetof (TYPE, MEMBER)
 
Index: clang/test/Sema/init.c
===
--- clang/test/Sema/init.c
+++ clang/test/Sema/init.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -verify -fsyntax-only -ffreestanding
+// RUN: %clang_cc1 %s -Wno-pointer-to-int-cast -verify -fsyntax-only -ffreestanding
 
 #include 
 #include 
Index: clang/test/Sema/const-eval.c
===
--- clang/test/Sema/const-eval.c
+++ clang/test/Sema/const-eval.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -triple x86_64-linux %s -Wno-tautological-pointer-compare
+// RUN: %clang_cc1 -fsyntax-only -verify -triple x86_64-linux %s -Wno-tautological-pointer-compare -Wno-pointer-to-int-cast
 
 #define EVAL_EXPR(testno, expr) int test##testno = sizeof(struct{char qq[expr];});
 int x;
Index: clang/test/Sema/cast.c
===
--- clang/test/Sema/cast.c
+++ clang/test/Sema/cast.c
@@ -1,4 +1,4 

[PATCH] D72231: [Sema] Adds the pointer-to-int-cast diagnostic

[John McCall via Phabricator via cfe-commits]

rjmccall added a reviewer: rjmccall.
rjmccall added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72231/new/

https://reviews.llvm.org/D72231



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72231: [Sema] Adds the pointer-to-int-cast diagnostic

[Mark de Wever via Phabricator via cfe-commits]

Mordante planned changes to this revision.
Mordante added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72231/new/

https://reviews.llvm.org/D72231



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72231: [Sema] Adds the pointer-to-int-cast diagnostic

[Mark de Wever via Phabricator via cfe-commits]

Mordante added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72231/new/

https://reviews.llvm.org/D72231



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72231: [Sema] Adds the pointer-to-int-cast diagnostic

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon check-circle color=green} Unit tests: pass. 61248 tests passed, 0 failed 
and 736 were skipped.

{icon times-circle color=red} clang-tidy: fail. Please fix clang-tidy findings 
.

{icon times-circle color=red} 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 



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72231/new/

https://reviews.llvm.org/D72231



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72231: [Sema] Adds the pointer-to-int-cast diagnostic

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

>> The diagnostic is not enabled by default

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72231/new/

https://reviews.llvm.org/D72231



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72231: [Sema] Adds the pointer-to-int-cast diagnostic

[Mark de Wever via Phabricator via cfe-commits]

Mordante created this revision.
Mordante added reviewers: dblaikie, rsmith.
Mordante added a project: clang.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D72231

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaCast.cpp
  clang/test/Sema/cast.c

Index: clang/test/Sema/cast.c
===
--- clang/test/Sema/cast.c
+++ clang/test/Sema/cast.c
@@ -1,4 +1,4 @@
-// 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;
@@ -151,19 +151,31 @@
 }
 
 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;
Index: clang/lib/Sema/SemaCast.cpp
===
--- clang/lib/Sema/SemaCast.cpp
+++ clang/lib/Sema/SemaCast.cpp
@@ -1961,7 +1961,7 @@
   << FD << DstCCName << FixItHint::CreateInsertion(NameLoc, CCAttrText);
 }
 
-static void checkIntToPointerCast(bool CStyle, SourceLocation Loc,
+static void checkIntToPointerCast(bool CStyle, const SourceRange ,
   const Expr *SrcExpr, QualType DestType,
   Sema ) {
   QualType SrcType = SrcExpr->getType();
@@ -1983,7 +1983,7 @@
 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;
   }
 }
 
@@ -2218,8 +2218,7 @@
 
   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
@@ -2734,8 +2733,8 @@
   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()) {
@@ -2745,6 +2744,19 @@
   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