[PATCH] D141310: [clang] add -Wcompare-function-pointers

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D141310#4285703 , @dblaikie wrote:

> FWIW I think it's still worth some data from applying this to a broad 
> codebase like Chromium/wherever it's planned to be used - whether it's 
> practical to make a codebase clean of this warning, what sort of challenges 
> arise, whether we should consider/need some way to suppress the warning in 
> particular cases, etc.

+1 to trying to find this information before we land the changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141310

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

[PATCH] D141310: [clang] add -Wcompare-function-pointers

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

FWIW I think it's still worth some data from applying this to a broad codebase 
like Chromium/wherever it's planned to be used - whether it's practical to make 
a codebase clean of this warning, what sort of challenges arise, whether we 
should consider/need some way to suppress the warning in particular cases, etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141310

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

[PATCH] D141310: [clang] add -Wcompare-function-pointers

[Christopher Di Bella via Phabricator via cfe-commits]

cjdb added a comment.

@aaron.ballman are you okay with this being merged now, provided that it's off 
by default? Apologies for letting this one fall through the cracks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141310

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

[PATCH] D141310: [clang] add -Wcompare-function-pointers

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

The code changes look good to me, the only real question I have left is whether 
this will be enabled by enough folks to be worth adding a default-off 
diagnostic. My intuition is that this is useful functionality for the folks who 
use `-icf=all` and when I look around to see if that option is being used, I 
see a terrifying number of uses of it given how broken I consider its semantics 
to be: 
https://sourcegraph.com/search?q=context:global+-icf%3Dall+-file:.*test.*=standard=1=repo

My thinking is that it's worthwhile to add this even though it's 
off-by-default, but hopefully we can find ways to encourage its use better. For 
example. it might be nice to suggest users enable this warning from the linker 
documentation about the `icf` option, or a blog post on the warning, etc. But I 
don't think that's a requirement to accept this. That said, I'm also curious 
about the details of how well users are able to react to this diagnostic 
(what's the false positive rate, can users make the corrections they need from 
the diagnostic, etc), so I'd prefer to hold off on landing this for a little 
bit until we have some idea of that from the chromium folks. (Given the branch 
is happening next week, I think this should wait to land until after the branch 
anyway so it has more time to bake.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141310

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

[PATCH] D141310: [clang] add -Wcompare-function-pointers

[Adrian Dole via Phabricator via cfe-commits]

adriandole updated this revision to Diff 490671.
adriandole added a comment.

Only trigger this warning when comparing function pointers of the same type, 
since comparing distinct types is already an error.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141310

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaExpr.cpp
  clang/test/SemaCXX/compare-function-pointer.cpp


Index: clang/test/SemaCXX/compare-function-pointer.cpp
===
--- clang/test/SemaCXX/compare-function-pointer.cpp
+++ clang/test/SemaCXX/compare-function-pointer.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -std=c++20 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++20 -verify 
-Wcompare-function-pointers %s
 
 using fp0_t = void (*)();
 using fp1_t = int (*)();
@@ -6,8 +6,8 @@
 extern fp0_t a, b;
 extern fp1_t c;
 
-bool eq0 = a == b;
-bool ne0 = a != b;
+bool eq0 = a == b;  // expected-warning {{distinct function pointers ('fp0_t' 
(aka 'void (*)()') and 'fp0_t')}}
+bool ne0 = a != b;  // expected-warning {{distinct function pointers}}
 bool lt0 = a < b;   // expected-warning {{ordered comparison of function 
pointers ('fp0_t' (aka 'void (*)()') and 'fp0_t')}}
 bool le0 = a <= b;  // expected-warning {{ordered comparison of function 
pointers}}
 bool gt0 = a > b;   // expected-warning {{ordered comparison of function 
pointers}}
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -12714,6 +12714,13 @@
LHS.get()->getSourceRange());
   }
 
+  if (!IsOrdered && LHSType->isFunctionPointerType() &&
+  RHSType->isFunctionPointerType() && !LHSIsNull && !RHSIsNull &&
+  RHSType == LHSType)
+Diag(Loc, diag::warn_typecheck_comparison_of_function_pointers)
+  << LHSType << RHSType
+  << LHS.get()->getSourceRange() << RHS.get()->getSourceRange();
+
   if (IsOrdered && LHSType->isFunctionPointerType() &&
   RHSType->isFunctionPointerType()) {
 // Valid unless a relational comparison of function pointers
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -7010,6 +7010,10 @@
   "%0 is %select{|in}2complete and "
   "%1 is %select{|in}3complete">,
   InGroup;
+def warn_typecheck_comparison_of_function_pointers : Warning<
+  "distinct function pointers (%0 and %1) may compare equal "
+  "when using identical code folding">,
+  InGroup, DefaultIgnore;
 def warn_typecheck_ordered_comparison_of_function_pointers : Warning<
   "ordered comparison of function pointers (%0 and %1)">,
   InGroup;
Index: clang/include/clang/Basic/DiagnosticGroups.td
===
--- clang/include/clang/Basic/DiagnosticGroups.td
+++ clang/include/clang/Basic/DiagnosticGroups.td
@@ -565,6 +565,7 @@
 def DeprecatedObjCIsaUsage : DiagGroup<"deprecated-objc-isa-usage">;
 def ExplicitInitializeCall : DiagGroup<"explicit-initialize-call">;
 def OrderedCompareFunctionPointers : 
DiagGroup<"ordered-compare-function-pointers">;
+def CompareFunctionPointers : DiagGroup<"compare-function-pointers", 
[OrderedCompareFunctionPointers]>;
 def PackedNonPod : DiagGroup<"packed-non-pod">;
 def Packed : DiagGroup<"packed", [PackedNonPod]>;
 def Padded : DiagGroup<"padded">;
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -465,6 +465,8 @@
 - Clang now automatically adds ``[[clang::lifetimebound]]`` to the parameters 
of
   ``std::move, std::forward`` et al, this enables Clang to diagnose more cases
   where the returned reference outlives the object.
+- Add ``-Wcompare-function-pointers`` to warn about comparisons that may have 
their behavior
+  change when enabling the identical code folding optimization feature of some 
linkers.
 
 Non-comprehensive list of changes in this release
 -


Index: clang/test/SemaCXX/compare-function-pointer.cpp
===
--- clang/test/SemaCXX/compare-function-pointer.cpp
+++ clang/test/SemaCXX/compare-function-pointer.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -std=c++20 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++20 -verify -Wcompare-function-pointers %s
 
 using fp0_t = void (*)();
 using fp1_t = int (*)();
@@ -6,8 +6,8 @@
 extern fp0_t a, b;
 extern fp1_t c;
 
-bool eq0 = a == b;
-bool ne0 = a != b;
+bool eq0 = a == b;  // 

[PATCH] D141310: [clang] add -Wcompare-function-pointers

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

>>> It's not that noisy compiling clang (eight hits).
>>
>> Good to know - I'm surprised it's that low.
>>
>> Is there some idiom we can use/document/recommend for people to use when the 
>> warning is a false positive? (when the user is confident the functions won't 
>> be folded together)
>
> How would the user know the warning is a false positive in the first place?

It's certainly no guarantee (a pathalogical compiler could busybox every 
function into one function, regardless of how different tnhose functions are) - 
but likely if the functions have different observable behavior they won't be 
folded (eg: if they write to different global variables, do different 
arithmetic, etc). It wouldn't surprise me if people were willing to thread that 
needle.

If @adriandole's intent is to not thread that needle, and actually remove all 
function pointer comparisons from a codebase using icf=all, I'd be curious to 
hear experience of that migration on a large codebase, yeah.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141310

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

[PATCH] D141310: [clang] add -Wcompare-function-pointers

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D141310#4063203 , @dblaikie wrote:

> In D141310#4062776 , @adriandole 
> wrote:
>
>> We use `icf=all` and have encountered bugs caused by function pointer 
>> comparisons.
>
> & the savings are worth it compared to icf=safe? (given the 
> limitations/bugs/investment in warnings like this, etc) I guess

That's the part I keep struggling with. It sounds like `icf=all` is a 
fundamentally broken feature; there's no way to catch all of the issues it 
introduces in code (you really need instrumentation for that, I'd imagine). If 
your code isn't impacted by the fundamental issues with the feature, then it's 
"safe" to use, but it sure doesn't seem like `icf=all` is something we want to 
*encourage* people to enable by making it seem safer than it really is. On the 
other hand, it exists, it's not likely to go away, and giving users SOME help 
is better than giving users NO help.

>> It's not that noisy compiling clang (eight hits).
>
> Good to know - I'm surprised it's that low.
>
> Is there some idiom we can use/document/recommend for people to use when the 
> warning is a false positive? (when the user is confident the functions won't 
> be folded together)

How would the user know the warning is a false positive in the first place?




Comment at: clang/test/SemaCXX/compare-function-pointer.cpp:17
 
-bool eq1 = a == c;  // expected-error {{comparison of distinct pointer types}}
-bool ne1 = a != c;  // expected-error {{comparison of distinct pointer types}}
+bool eq1 = a == c;  // expected-error {{comparison of distinct pointer types}} 
expected-warning {{comparison of function pointers}}
+bool ne1 = a != c;  // expected-error {{comparison of distinct pointer types}} 
expected-warning {{comparison of function pointers}}

cjdb wrote:
> It doesn't make sense to me that we would emit a warning when we're already 
> emitting an error.
+1, this feels like we're adding more heat without any light.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141310

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

[PATCH] D141310: [clang] add -Wcompare-function-pointers

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D141310#4062776 , @adriandole 
wrote:

> @dblaikie, we would use this warning in Chrome OS.

Ah, good to know!

> We use `icf=all` and have encountered bugs caused by function pointer 
> comparisons.

& the savings are worth it compared to icf=safe? (given the 
limitations/bugs/investment in warnings like this, etc) I guess

> It's not that noisy compiling clang (eight hits).

Good to know - I'm surprised it's that low.

Is there some idiom we can use/document/recommend for people to use when the 
warning is a false positive? (when the user is confident the functions won't be 
folded together)

> Working on testing it for Chrome OS.

ah, cool - be good to know what that looks like/what kind of changes you end up 
needing to make to the codebase to get it building cleanly/how much of the work 
involves fixing real bugs compared to suppressing/satisfying the compiler.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141310

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

[PATCH] D141310: [clang] add -Wcompare-function-pointers

[Adrian Dole via Phabricator via cfe-commits]

adriandole added a comment.

@dblaikie, we would use this warning in Chrome OS. We use `icf=all` and have 
encountered bugs caused by function pointer comparisons.

It's not that noisy compiling clang (eight hits). Working on testing it for 
Chrome OS.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141310

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

[PATCH] D141310: [clang] add -Wcompare-function-pointers

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

But I don't mean to suggest I should be a decider/veto here - it's cheap to 
maintain/no big deal, so maybe that's fine - but for myself, having at least 
some large scale customer with existing experience testing the warning and a 
strong commitment/motivation to keep using it would be a reasonable ask for 
this kind of warning in general, even if we have/are relaxing the "exceptional 
circumstances are required to add off-by-default warnings" policy of old.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141310

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

[PATCH] D141310: [clang] add -Wcompare-function-pointers

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D141310#4060069 , @aaron.ballman 
wrote:

> In D141310#4054351 , @dblaikie 
> wrote:
>
>> @adriandole do you plan to deploy this in a codebase? Have you tried it on a 
>> codebase already?
>>
>> I'd worry this would just be too noisy, and there's probably enough benign 
>> pointer comparisons that'll never hit the ICF false-equality situation (eg: 
>> putting some callbacks in a map/set/something - where the callbacks all do 
>> genuinely different things, so they'd never end up with accidental identical 
>> functions/folding) that it wouldn't be feasible to use this in a real 
>> codebase?
>
> Are your worries lessened by the fact that this is (by necessity of the way 
> the toolchain is composed) be an off-by-default warning that users must opt 
> into? My thinking is that this shouldn't be *too* chatty because it's 
> specific to equality comparisons between (non-nullptr) function pointers, but 
> I agree that having some confirmation about this finding true positives that 
> aren't swamped by false positives would be beneficial.

In some ways an off-by-default warning makes me worry more about it having some 
good test coverage/usage - since it won't get it incidentally from being on by 
default (of course we want on by default warnings well vetted so we don't annoy 
users - but we certainly won't be lacking feedback on them if their false 
positive rate is too high).

I'd think that there'd be enough people putting function pointers in maps or 
other data structures that would compare them that this warning would be fairly 
esoteric/only applicable to fairly small codebases (or ones that have 
incidentally avoided any function pointer usage) and may be more amenable to a 
clang-tidy check than a compiler warning.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141310

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

[PATCH] D141310: [clang] add -Wcompare-function-pointers

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D141310#4054351 , @dblaikie wrote:

> @adriandole do you plan to deploy this in a codebase? Have you tried it on a 
> codebase already?
>
> I'd worry this would just be too noisy, and there's probably enough benign 
> pointer comparisons that'll never hit the ICF false-equality situation (eg: 
> putting some callbacks in a map/set/something - where the callbacks all do 
> genuinely different things, so they'd never end up with accidental identical 
> functions/folding) that it wouldn't be feasible to use this in a real 
> codebase?

Are your worries lessened by the fact that this is (by necessity of the way the 
toolchain is composed) be an off-by-default warning that users must opt into? 
My thinking is that this shouldn't be *too* chatty because it's specific to 
equality comparisons between (non-nullptr) function pointers, but I agree that 
having some confirmation about this finding true positives that aren't swamped 
by false positives would be beneficial.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141310

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

[PATCH] D141310: [clang] add -Wcompare-function-pointers

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

@adriandole do you plan to deploy this in a codebase? Have you tried it on a 
codebase already?

I'd worry this would just be too noisy, and there's probably enough benign 
pointer comparisons that'll never hit the ICF false-equality situation (eg: 
putting some callbacks in a map/set/something - where the callbacks all do 
genuinely different things, so they'd never end up with accidental identical 
functions/folding) that it wouldn't be feasible to use this in a real codebase?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141310

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

[PATCH] D141310: [clang] add -Wcompare-function-pointers

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7007
+  "comparison of function pointers (%0 and %1)">,
+  InGroup, DefaultIgnore;
 def warn_typecheck_ordered_comparison_of_function_pointers : Warning<

aaron.ballman wrote:
> aaron.ballman wrote:
> > cjdb wrote:
> > > aaron.ballman wrote:
> > > > cjdb wrote:
> > > > > cjdb wrote:
> > > > > > It's very rare that we set a warning to `DefaultIgnore`. What's the 
> > > > > > motivation for this?
> > > > > > This warning is disabled by default, since it's only relevant if 
> > > > > > ICF is explicitly enabled.
> > > > > 
> > > > > I see why now. Perhaps this warning should be enabled by default when 
> > > > > ICF is also enabled, and an error otherwise.
> > > > The problem is: ICF is an lld thing, it's not a Clang thing; so Clang 
> > > > has no idea if ICF will or won't be enabled.
> > > Okay, that sounds like we can't make it an error to turn this warning on 
> > > when ICF isn't enabled, but what about turning it on when the driver sees 
> > > `-icf=all`? Or does that bypass `clang_cc1` altogether?
> > The diagnostic serves no purpose unless the user is linking with 
> > `-icf=all`, so agreed we can't enable this by default. We might be able to 
> > do something by looking at linker flags passed through clang on to the 
> > driver, but it's not going to be perfect (users can link manually without 
> > invoking through the compiler, and I'm not certain what IDEs do when 
> > driving builds with Clang).
> @MaskRay  -- do you think we should try to enable this diagnostic by default 
> by looking at driver flags that will be passed to the linker, or does it seem 
> more reasonable to you to have this ignored by default?
I think it should remain ignored by default.

Link actions happen separate at a late stage and we generally don't want linker 
options to affect compiles. (e.g. We haven't allowed -fuse-ld= to affect 
compiles).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141310

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

[PATCH] D141310: [clang] add -Wcompare-function-pointers

[Adrian Dole via Phabricator via cfe-commits]

adriandole updated this revision to Diff 489109.
adriandole added a comment.

- More helpful diagnostic message.
- Check variable and type names are printed in tests.
- Enable `-Wordered-function-pointer-comparison` when this warning is enabled.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141310

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaExpr.cpp
  clang/test/SemaCXX/compare-function-pointer.cpp


Index: clang/test/SemaCXX/compare-function-pointer.cpp
===
--- clang/test/SemaCXX/compare-function-pointer.cpp
+++ clang/test/SemaCXX/compare-function-pointer.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -std=c++20 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++20 -verify 
-Wcompare-function-pointers %s
 
 using fp0_t = void (*)();
 using fp1_t = int (*)();
@@ -6,8 +6,8 @@
 extern fp0_t a, b;
 extern fp1_t c;
 
-bool eq0 = a == b;
-bool ne0 = a != b;
+bool eq0 = a == b;  // expected-warning {{distinct function pointers ('fp0_t' 
(aka 'void (*)()') and 'fp0_t')}}
+bool ne0 = a != b;  // expected-warning {{distinct function pointers}}
 bool lt0 = a < b;   // expected-warning {{ordered comparison of function 
pointers ('fp0_t' (aka 'void (*)()') and 'fp0_t')}}
 bool le0 = a <= b;  // expected-warning {{ordered comparison of function 
pointers}}
 bool gt0 = a > b;   // expected-warning {{ordered comparison of function 
pointers}}
@@ -15,7 +15,9 @@
 auto tw0 = a <=> b; // expected-error {{ordered comparison of function 
pointers}}
 
 bool eq1 = a == c;  // expected-error {{comparison of distinct pointer types}}
+// expected-warning@-1 {{distinct function pointers 
('fp0_t' (aka 'void (*)()') and 'fp1_t' (aka 'int (*)()'))}}
 bool ne1 = a != c;  // expected-error {{comparison of distinct pointer types}}
+// expected-warning@-1 {{distinct function pointers}}
 bool lt1 = a < c;   // expected-warning {{ordered comparison of function 
pointers ('fp0_t' (aka 'void (*)()') and 'fp1_t' (aka 'int (*)()'))}}
 // expected-error@-1 {{comparison of distinct pointer 
types}}
 bool le1 = a <= c;  // expected-warning {{ordered comparison of function 
pointers}}
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -12711,6 +12711,12 @@
LHS.get()->getSourceRange());
   }
 
+  if (!IsOrdered && LHSType->isFunctionPointerType() &&
+  RHSType->isFunctionPointerType() && !LHSIsNull && !RHSIsNull)
+Diag(Loc, diag::warn_typecheck_comparison_of_function_pointers)
+  << LHSType << RHSType
+  << LHS.get()->getSourceRange() << RHS.get()->getSourceRange();
+
   if (IsOrdered && LHSType->isFunctionPointerType() &&
   RHSType->isFunctionPointerType()) {
 // Valid unless a relational comparison of function pointers
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -7009,6 +7009,10 @@
   "%0 is %select{|in}2complete and "
   "%1 is %select{|in}3complete">,
   InGroup;
+def warn_typecheck_comparison_of_function_pointers : Warning<
+  "distinct function pointers (%0 and %1) may compare equal "
+  "when using identical code folding">,
+  InGroup, DefaultIgnore;
 def warn_typecheck_ordered_comparison_of_function_pointers : Warning<
   "ordered comparison of function pointers (%0 and %1)">,
   InGroup;
Index: clang/include/clang/Basic/DiagnosticGroups.td
===
--- clang/include/clang/Basic/DiagnosticGroups.td
+++ clang/include/clang/Basic/DiagnosticGroups.td
@@ -564,6 +564,7 @@
 def DeprecatedObjCIsaUsage : DiagGroup<"deprecated-objc-isa-usage">;
 def ExplicitInitializeCall : DiagGroup<"explicit-initialize-call">;
 def OrderedCompareFunctionPointers : 
DiagGroup<"ordered-compare-function-pointers">;
+def CompareFunctionPointers : DiagGroup<"compare-function-pointers", 
[OrderedCompareFunctionPointers]>;
 def PackedNonPod : DiagGroup<"packed-non-pod">;
 def Packed : DiagGroup<"packed", [PackedNonPod]>;
 def Padded : DiagGroup<"padded">;
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -455,6 +455,8 @@
 - Add ``-Wreturn-local-addr``, a GCC alias for ``-Wreturn-stack-address``.
 - Clang now suppresses ``-Wlogical-op-parentheses`` on ``(x && a || b)`` and 
``(a || b && x)``
   only when ``x`` is a string literal.
+- Add ``-Wcompare-function-pointers`` to warn about comparisons 

[PATCH] D141310: [clang] add -Wcompare-function-pointers

[Adrian Dole via Phabricator via cfe-commits]

adriandole added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticGroups.td:565
+def CompareFunctionPointers : DiagGroup<"compare-function-pointers">;
 def OrderedCompareFunctionPointers : 
DiagGroup<"ordered-compare-function-pointers">;
 def PackedNonPod : DiagGroup<"packed-non-pod">;

aaron.ballman wrote:
> Should `-Wcompare-function-pointers` enable 
> `-Wordered-compare-function-pointers`? I think I would normally assume such a 
> relationship as the ordered variant sounds like a subset of the unordered 
> variant.
Makes sense, and it also fixes the issue of raising both this warning and 
`-Wordered-compare-function-pointers` on the same line. Will update.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141310

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

[PATCH] D141310: [clang] add -Wcompare-function-pointers

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:451
+- Add ``-Wcompare-function-pointers`` to warn about comparisons that may have
+  their behavior change when using ``icf=all``.
 

aaron.ballman wrote:
> This makes it more clear this is an lld-specific change.
Changing this suggestion to: "that may have their behavior change when enabling 
the identical code folding optimization feature of some linkers."



Comment at: clang/include/clang/Basic/DiagnosticGroups.td:565
+def CompareFunctionPointers : DiagGroup<"compare-function-pointers">;
 def OrderedCompareFunctionPointers : 
DiagGroup<"ordered-compare-function-pointers">;
 def PackedNonPod : DiagGroup<"packed-non-pod">;

Should `-Wcompare-function-pointers` enable 
`-Wordered-compare-function-pointers`? I think I would normally assume such a 
relationship as the ordered variant sounds like a subset of the unordered 
variant.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7006
+def warn_typecheck_comparison_of_function_pointers : Warning<
+  "comparison of function pointers (%0 and %1)">,
+  InGroup, DefaultIgnore;

adriandole wrote:
> aaron.ballman wrote:
> > adriandole wrote:
> > > aaron.ballman wrote:
> > > > cjdb wrote:
> > > > > This diagnostic feels very bare bones, and doesn't explain why the 
> > > > > user should care. It would be good to rephrase the message so that it 
> > > > > can incorporate some kind of reason too.
> > > > Agreed -- diagnostic wording is usually trying to tell the user what 
> > > > they did wrong to guide them how to fix it, but this reads more like 
> > > > the diagnostic explains what the code does. This one will be especially 
> > > > interesting because people would naturally expect that comparison to 
> > > > work per the standard. And if enabling ICF breaks that then ICF is 
> > > > non-conforming because it makes valid code silently invalid. I think 
> > > > this is an ICF bug (it may still be worth warning users about though 
> > > > because you can use newer Clang with an older lld and hit the issue 
> > > > even if lld addresses this issue).
> > > > 
> > > > CC @lhames @sbc100 @ruiu in to try to scare up an lld code owner to see 
> > > > if this is known and if there's something that can be done on the lld 
> > > > side to not break valid code.
> > > There's already an ICF option that doesn't break valid code: `-icf=safe`. 
> > > Only if you explicitly decide that you don't care about the results of 
> > > function pointer comparisons would you use `-icf=all`, which enables more 
> > > code folding to be done than the safe option.
> > Ohhh interesting, so it's probably known that `-icf=all` will break code 
> > because it's not conforming and thus isn't an lld bug at all. Do (most?) 
> > other linkers also have the same functionality as `-icf=all`? I'm trying to 
> > understand whether this should be added to clang at all as it seems like 
> > it's a warning for a very particular usage pattern (a specific linker with 
> > a specific option ), which make it more reasonable for clang-tidy instead 
> > of the compiler.
> ld.gold and mold have it (same name, `-icf=all`) as does MSVC (`/OPT:ICF`).
Thanks for the details, that makes this seem more reasonable in Clang itself 
IMO.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7007
+  "comparison of function pointers (%0 and %1)">,
+  InGroup, DefaultIgnore;
 def warn_typecheck_ordered_comparison_of_function_pointers : Warning<

aaron.ballman wrote:
> cjdb wrote:
> > aaron.ballman wrote:
> > > cjdb wrote:
> > > > cjdb wrote:
> > > > > It's very rare that we set a warning to `DefaultIgnore`. What's the 
> > > > > motivation for this?
> > > > > This warning is disabled by default, since it's only relevant if ICF 
> > > > > is explicitly enabled.
> > > > 
> > > > I see why now. Perhaps this warning should be enabled by default when 
> > > > ICF is also enabled, and an error otherwise.
> > > The problem is: ICF is an lld thing, it's not a Clang thing; so Clang has 
> > > no idea if ICF will or won't be enabled.
> > Okay, that sounds like we can't make it an error to turn this warning on 
> > when ICF isn't enabled, but what about turning it on when the driver sees 
> > `-icf=all`? Or does that bypass `clang_cc1` altogether?
> The diagnostic serves no purpose unless the user is linking with `-icf=all`, 
> so agreed we can't enable this by default. We might be able to do something 
> by looking at linker flags passed through clang on to the driver, but it's 
> not going to be perfect (users can link manually without invoking through the 
> compiler, and I'm not certain what IDEs do when driving builds with Clang).
@MaskRay  -- do you think we should try to enable this diagnostic by default by 
looking at driver flags that will be passed to the linker, or does it seem more 
reasonable 

[PATCH] D141310: [clang] add -Wcompare-function-pointers

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

I have some notes about `--icf={safe,all}`: 
https://maskray.me/blog/2020-11-15-explain-gnu-linker-options#icfall-and---icfsafe

I think this diagnostic has some value (and I appreciate that it can be 
implemented so little code!). Some `--icf=all` issues like using function 
addresses as map keys cannot be detected, though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141310

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

[PATCH] D141310: [clang] add -Wcompare-function-pointers

[Adrian Dole via Phabricator via cfe-commits]

adriandole added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7006
+def warn_typecheck_comparison_of_function_pointers : Warning<
+  "comparison of function pointers (%0 and %1)">,
+  InGroup, DefaultIgnore;

aaron.ballman wrote:
> adriandole wrote:
> > aaron.ballman wrote:
> > > cjdb wrote:
> > > > This diagnostic feels very bare bones, and doesn't explain why the user 
> > > > should care. It would be good to rephrase the message so that it can 
> > > > incorporate some kind of reason too.
> > > Agreed -- diagnostic wording is usually trying to tell the user what they 
> > > did wrong to guide them how to fix it, but this reads more like the 
> > > diagnostic explains what the code does. This one will be especially 
> > > interesting because people would naturally expect that comparison to work 
> > > per the standard. And if enabling ICF breaks that then ICF is 
> > > non-conforming because it makes valid code silently invalid. I think this 
> > > is an ICF bug (it may still be worth warning users about though because 
> > > you can use newer Clang with an older lld and hit the issue even if lld 
> > > addresses this issue).
> > > 
> > > CC @lhames @sbc100 @ruiu in to try to scare up an lld code owner to see 
> > > if this is known and if there's something that can be done on the lld 
> > > side to not break valid code.
> > There's already an ICF option that doesn't break valid code: `-icf=safe`. 
> > Only if you explicitly decide that you don't care about the results of 
> > function pointer comparisons would you use `-icf=all`, which enables more 
> > code folding to be done than the safe option.
> Ohhh interesting, so it's probably known that `-icf=all` will break code 
> because it's not conforming and thus isn't an lld bug at all. Do (most?) 
> other linkers also have the same functionality as `-icf=all`? I'm trying to 
> understand whether this should be added to clang at all as it seems like it's 
> a warning for a very particular usage pattern (a specific linker with a 
> specific option ), which make it more reasonable for clang-tidy instead of 
> the compiler.
ld.gold and mold have it (same name, `-icf=all`) as does MSVC (`/OPT:ICF`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141310

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

[PATCH] D141310: [clang] add -Wcompare-function-pointers

[Thorsten via Phabricator via cfe-commits]

tschuett added subscribers: MaskRay, tschuett.
tschuett added a comment.

@MaskRay

Dependent of the build flags, we decide which warnings to enable or not? Do we 
have other warnings for ThinLTO?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141310

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

[PATCH] D141310: [clang] add -Wcompare-function-pointers

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7007
+  "comparison of function pointers (%0 and %1)">,
+  InGroup, DefaultIgnore;
 def warn_typecheck_ordered_comparison_of_function_pointers : Warning<

cjdb wrote:
> aaron.ballman wrote:
> > cjdb wrote:
> > > cjdb wrote:
> > > > It's very rare that we set a warning to `DefaultIgnore`. What's the 
> > > > motivation for this?
> > > > This warning is disabled by default, since it's only relevant if ICF is 
> > > > explicitly enabled.
> > > 
> > > I see why now. Perhaps this warning should be enabled by default when ICF 
> > > is also enabled, and an error otherwise.
> > The problem is: ICF is an lld thing, it's not a Clang thing; so Clang has 
> > no idea if ICF will or won't be enabled.
> Okay, that sounds like we can't make it an error to turn this warning on when 
> ICF isn't enabled, but what about turning it on when the driver sees 
> `-icf=all`? Or does that bypass `clang_cc1` altogether?
The diagnostic serves no purpose unless the user is linking with `-icf=all`, so 
agreed we can't enable this by default. We might be able to do something by 
looking at linker flags passed through clang on to the driver, but it's not 
going to be perfect (users can link manually without invoking through the 
compiler, and I'm not certain what IDEs do when driving builds with Clang).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141310

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

[PATCH] D141310: [clang] add -Wcompare-function-pointers

[Christopher Di Bella via Phabricator via cfe-commits]

cjdb added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7007
+  "comparison of function pointers (%0 and %1)">,
+  InGroup, DefaultIgnore;
 def warn_typecheck_ordered_comparison_of_function_pointers : Warning<

aaron.ballman wrote:
> cjdb wrote:
> > cjdb wrote:
> > > It's very rare that we set a warning to `DefaultIgnore`. What's the 
> > > motivation for this?
> > > This warning is disabled by default, since it's only relevant if ICF is 
> > > explicitly enabled.
> > 
> > I see why now. Perhaps this warning should be enabled by default when ICF 
> > is also enabled, and an error otherwise.
> The problem is: ICF is an lld thing, it's not a Clang thing; so Clang has no 
> idea if ICF will or won't be enabled.
Okay, that sounds like we can't make it an error to turn this warning on when 
ICF isn't enabled, but what about turning it on when the driver sees 
`-icf=all`? Or does that bypass `clang_cc1` altogether?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141310

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

[PATCH] D141310: [clang] add -Wcompare-function-pointers

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7006
+def warn_typecheck_comparison_of_function_pointers : Warning<
+  "comparison of function pointers (%0 and %1)">,
+  InGroup, DefaultIgnore;

adriandole wrote:
> aaron.ballman wrote:
> > cjdb wrote:
> > > This diagnostic feels very bare bones, and doesn't explain why the user 
> > > should care. It would be good to rephrase the message so that it can 
> > > incorporate some kind of reason too.
> > Agreed -- diagnostic wording is usually trying to tell the user what they 
> > did wrong to guide them how to fix it, but this reads more like the 
> > diagnostic explains what the code does. This one will be especially 
> > interesting because people would naturally expect that comparison to work 
> > per the standard. And if enabling ICF breaks that then ICF is 
> > non-conforming because it makes valid code silently invalid. I think this 
> > is an ICF bug (it may still be worth warning users about though because you 
> > can use newer Clang with an older lld and hit the issue even if lld 
> > addresses this issue).
> > 
> > CC @lhames @sbc100 @ruiu in to try to scare up an lld code owner to see if 
> > this is known and if there's something that can be done on the lld side to 
> > not break valid code.
> There's already an ICF option that doesn't break valid code: `-icf=safe`. 
> Only if you explicitly decide that you don't care about the results of 
> function pointer comparisons would you use `-icf=all`, which enables more 
> code folding to be done than the safe option.
Ohhh interesting, so it's probably known that `-icf=all` will break code 
because it's not conforming and thus isn't an lld bug at all. Do (most?) other 
linkers also have the same functionality as `-icf=all`? I'm trying to 
understand whether this should be added to clang at all as it seems like it's a 
warning for a very particular usage pattern (a specific linker with a specific 
option ), which make it more reasonable for clang-tidy instead of the compiler.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141310

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

[PATCH] D141310: [clang] add -Wcompare-function-pointers

[Adrian Dole via Phabricator via cfe-commits]

adriandole added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7006
+def warn_typecheck_comparison_of_function_pointers : Warning<
+  "comparison of function pointers (%0 and %1)">,
+  InGroup, DefaultIgnore;

aaron.ballman wrote:
> cjdb wrote:
> > This diagnostic feels very bare bones, and doesn't explain why the user 
> > should care. It would be good to rephrase the message so that it can 
> > incorporate some kind of reason too.
> Agreed -- diagnostic wording is usually trying to tell the user what they did 
> wrong to guide them how to fix it, but this reads more like the diagnostic 
> explains what the code does. This one will be especially interesting because 
> people would naturally expect that comparison to work per the standard. And 
> if enabling ICF breaks that then ICF is non-conforming because it makes valid 
> code silently invalid. I think this is an ICF bug (it may still be worth 
> warning users about though because you can use newer Clang with an older lld 
> and hit the issue even if lld addresses this issue).
> 
> CC @lhames @sbc100 @ruiu in to try to scare up an lld code owner to see if 
> this is known and if there's something that can be done on the lld side to 
> not break valid code.
There's already an ICF option that doesn't break valid code: `-icf=safe`. Only 
if you explicitly decide that you don't care about the results of function 
pointer comparisons would you use `-icf=all`, which enables more code folding 
to be done than the safe option.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141310

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

[PATCH] D141310: [clang] add -Wcompare-function-pointers

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added subscribers: sbc100, ruiu, lhames.
aaron.ballman added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:451
+- Add ``-Wcompare-function-pointers`` to warn about comparisons that may have
+  their behavior change when using ``icf=all``.
 

This makes it more clear this is an lld-specific change.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7006
+def warn_typecheck_comparison_of_function_pointers : Warning<
+  "comparison of function pointers (%0 and %1)">,
+  InGroup, DefaultIgnore;

cjdb wrote:
> This diagnostic feels very bare bones, and doesn't explain why the user 
> should care. It would be good to rephrase the message so that it can 
> incorporate some kind of reason too.
Agreed -- diagnostic wording is usually trying to tell the user what they did 
wrong to guide them how to fix it, but this reads more like the diagnostic 
explains what the code does. This one will be especially interesting because 
people would naturally expect that comparison to work per the standard. And if 
enabling ICF breaks that then ICF is non-conforming because it makes valid code 
silently invalid. I think this is an ICF bug (it may still be worth warning 
users about though because you can use newer Clang with an older lld and hit 
the issue even if lld addresses this issue).

CC @lhames @sbc100 @ruiu in to try to scare up an lld code owner to see if this 
is known and if there's something that can be done on the lld side to not break 
valid code.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7007
+  "comparison of function pointers (%0 and %1)">,
+  InGroup, DefaultIgnore;
 def warn_typecheck_ordered_comparison_of_function_pointers : Warning<

cjdb wrote:
> cjdb wrote:
> > It's very rare that we set a warning to `DefaultIgnore`. What's the 
> > motivation for this?
> > This warning is disabled by default, since it's only relevant if ICF is 
> > explicitly enabled.
> 
> I see why now. Perhaps this warning should be enabled by default when ICF is 
> also enabled, and an error otherwise.
The problem is: ICF is an lld thing, it's not a Clang thing; so Clang has no 
idea if ICF will or won't be enabled.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141310

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

[PATCH] D141310: [clang] add -Wcompare-function-pointers

[Christopher Di Bella via Phabricator via cfe-commits]

cjdb added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7007
+  "comparison of function pointers (%0 and %1)">,
+  InGroup, DefaultIgnore;
 def warn_typecheck_ordered_comparison_of_function_pointers : Warning<

cjdb wrote:
> It's very rare that we set a warning to `DefaultIgnore`. What's the 
> motivation for this?
> This warning is disabled by default, since it's only relevant if ICF is 
> explicitly enabled.

I see why now. Perhaps this warning should be enabled by default when ICF is 
also enabled, and an error otherwise.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141310

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

[PATCH] D141310: [clang] add -Wcompare-function-pointers

[Christopher Di Bella via Phabricator via cfe-commits]

cjdb added a comment.

Thanks for working on this! Needs a bit of work, but we should be able to get 
this in very soon methinks.




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7006
+def warn_typecheck_comparison_of_function_pointers : Warning<
+  "comparison of function pointers (%0 and %1)">,
+  InGroup, DefaultIgnore;

This diagnostic feels very bare bones, and doesn't explain why the user should 
care. It would be good to rephrase the message so that it can incorporate some 
kind of reason too.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7007
+  "comparison of function pointers (%0 and %1)">,
+  InGroup, DefaultIgnore;
 def warn_typecheck_ordered_comparison_of_function_pointers : Warning<

It's very rare that we set a warning to `DefaultIgnore`. What's the motivation 
for this?



Comment at: clang/lib/Sema/SemaExpr.cpp:12715
+  if (!IsOrdered && LHSType->isFunctionPointerType() &&
+  RHSType->isFunctionPointerType() && !LHSIsNull && !RHSIsNull) {
+Diag(Loc, diag::warn_typecheck_comparison_of_function_pointers)

Braces sadly need to go, as this is a one-line statement.



Comment at: clang/test/SemaCXX/compare-function-pointer.cpp:9
 
-bool eq0 = a == b;
-bool ne0 = a != b;
+bool eq0 = a == b;  // expected-warning {{comparison of function pointers}}
+bool ne0 = a != b;  // expected-warning {{comparison of function pointers}}

Please be sure to check that the names are output too.



Comment at: clang/test/SemaCXX/compare-function-pointer.cpp:17
 
-bool eq1 = a == c;  // expected-error {{comparison of distinct pointer types}}
-bool ne1 = a != c;  // expected-error {{comparison of distinct pointer types}}
+bool eq1 = a == c;  // expected-error {{comparison of distinct pointer types}} 
expected-warning {{comparison of function pointers}}
+bool ne1 = a != c;  // expected-error {{comparison of distinct pointer types}} 
expected-warning {{comparison of function pointers}}

It doesn't make sense to me that we would emit a warning when we're already 
emitting an error.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141310

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

[PATCH] D141310: [clang] add -Wcompare-function-pointers

[Adrian Dole via Phabricator via cfe-commits]

adriandole updated this revision to Diff 487511.
adriandole added a comment.

Release note.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141310

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaExpr.cpp
  clang/test/SemaCXX/compare-function-pointer.cpp


Index: clang/test/SemaCXX/compare-function-pointer.cpp
===
--- clang/test/SemaCXX/compare-function-pointer.cpp
+++ clang/test/SemaCXX/compare-function-pointer.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -std=c++20 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++20 -verify 
-Wcompare-function-pointers %s
 
 using fp0_t = void (*)();
 using fp1_t = int (*)();
@@ -6,16 +6,16 @@
 extern fp0_t a, b;
 extern fp1_t c;
 
-bool eq0 = a == b;
-bool ne0 = a != b;
+bool eq0 = a == b;  // expected-warning {{comparison of function pointers}}
+bool ne0 = a != b;  // expected-warning {{comparison of function pointers}}
 bool lt0 = a < b;   // expected-warning {{ordered comparison of function 
pointers ('fp0_t' (aka 'void (*)()') and 'fp0_t')}}
 bool le0 = a <= b;  // expected-warning {{ordered comparison of function 
pointers}}
 bool gt0 = a > b;   // expected-warning {{ordered comparison of function 
pointers}}
 bool ge0 = a >= b;  // expected-warning {{ordered comparison of function 
pointers}}
 auto tw0 = a <=> b; // expected-error {{ordered comparison of function 
pointers}}
 
-bool eq1 = a == c;  // expected-error {{comparison of distinct pointer types}}
-bool ne1 = a != c;  // expected-error {{comparison of distinct pointer types}}
+bool eq1 = a == c;  // expected-error {{comparison of distinct pointer types}} 
expected-warning {{comparison of function pointers}}
+bool ne1 = a != c;  // expected-error {{comparison of distinct pointer types}} 
expected-warning {{comparison of function pointers}}
 bool lt1 = a < c;   // expected-warning {{ordered comparison of function 
pointers ('fp0_t' (aka 'void (*)()') and 'fp1_t' (aka 'int (*)()'))}}
 // expected-error@-1 {{comparison of distinct pointer 
types}}
 bool le1 = a <= c;  // expected-warning {{ordered comparison of function 
pointers}}
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -12711,6 +12711,13 @@
LHS.get()->getSourceRange());
   }
 
+  if (!IsOrdered && LHSType->isFunctionPointerType() &&
+  RHSType->isFunctionPointerType() && !LHSIsNull && !RHSIsNull) {
+Diag(Loc, diag::warn_typecheck_comparison_of_function_pointers)
+  << LHSType << 0
+  << LHS.get()->getSourceRange() << RHS.get()->getSourceRange();
+  }
+
   if (IsOrdered && LHSType->isFunctionPointerType() &&
   RHSType->isFunctionPointerType()) {
 // Valid unless a relational comparison of function pointers
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -7002,6 +7002,9 @@
   "%0 is %select{|in}2complete and "
   "%1 is %select{|in}3complete">,
   InGroup;
+def warn_typecheck_comparison_of_function_pointers : Warning<
+  "comparison of function pointers (%0 and %1)">,
+  InGroup, DefaultIgnore;
 def warn_typecheck_ordered_comparison_of_function_pointers : Warning<
   "ordered comparison of function pointers (%0 and %1)">,
   InGroup;
Index: clang/include/clang/Basic/DiagnosticGroups.td
===
--- clang/include/clang/Basic/DiagnosticGroups.td
+++ clang/include/clang/Basic/DiagnosticGroups.td
@@ -561,6 +561,7 @@
 def UnderalignedExceptionObject : DiagGroup<"underaligned-exception-object">;
 def DeprecatedObjCIsaUsage : DiagGroup<"deprecated-objc-isa-usage">;
 def ExplicitInitializeCall : DiagGroup<"explicit-initialize-call">;
+def CompareFunctionPointers : DiagGroup<"compare-function-pointers">;
 def OrderedCompareFunctionPointers : 
DiagGroup<"ordered-compare-function-pointers">;
 def PackedNonPod : DiagGroup<"packed-non-pod">;
 def Packed : DiagGroup<"packed", [PackedNonPod]>;
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -447,6 +447,8 @@
   ``#pragma clang __debug sloc_usage`` can also be used to request this report.
 - Clang no longer permits the keyword 'bool' in a concept declaration as a
   concepts-ts compatibility extension.
+- Add ``-Wcompare-function-pointers`` to warn about comparisons that may have
+  their behavior change when using ``icf=all``.
 
 Non-comprehensive list of changes in this release
 

[PATCH] D141310: [clang] add -Wcompare-function-pointers

[Adrian Dole via Phabricator via cfe-commits]

adriandole created this revision.
Herald added a project: All.
adriandole requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Using `icf=all` with lld can cause comparisons between function pointers that 
previously compared as unequal to compare as equal. This warning is disabled by 
default, since it's only relevant if ICF is explicitly enabled.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D141310

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaExpr.cpp
  clang/test/SemaCXX/compare-function-pointer.cpp


Index: clang/test/SemaCXX/compare-function-pointer.cpp
===
--- clang/test/SemaCXX/compare-function-pointer.cpp
+++ clang/test/SemaCXX/compare-function-pointer.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -std=c++20 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++20 -verify 
-Wcompare-function-pointers %s
 
 using fp0_t = void (*)();
 using fp1_t = int (*)();
@@ -6,16 +6,16 @@
 extern fp0_t a, b;
 extern fp1_t c;
 
-bool eq0 = a == b;
-bool ne0 = a != b;
+bool eq0 = a == b;  // expected-warning {{comparison of function pointers}}
+bool ne0 = a != b;  // expected-warning {{comparison of function pointers}}
 bool lt0 = a < b;   // expected-warning {{ordered comparison of function 
pointers ('fp0_t' (aka 'void (*)()') and 'fp0_t')}}
 bool le0 = a <= b;  // expected-warning {{ordered comparison of function 
pointers}}
 bool gt0 = a > b;   // expected-warning {{ordered comparison of function 
pointers}}
 bool ge0 = a >= b;  // expected-warning {{ordered comparison of function 
pointers}}
 auto tw0 = a <=> b; // expected-error {{ordered comparison of function 
pointers}}
 
-bool eq1 = a == c;  // expected-error {{comparison of distinct pointer types}}
-bool ne1 = a != c;  // expected-error {{comparison of distinct pointer types}}
+bool eq1 = a == c;  // expected-error {{comparison of distinct pointer types}} 
expected-warning {{comparison of function pointers}}
+bool ne1 = a != c;  // expected-error {{comparison of distinct pointer types}} 
expected-warning {{comparison of function pointers}}
 bool lt1 = a < c;   // expected-warning {{ordered comparison of function 
pointers ('fp0_t' (aka 'void (*)()') and 'fp1_t' (aka 'int (*)()'))}}
 // expected-error@-1 {{comparison of distinct pointer 
types}}
 bool le1 = a <= c;  // expected-warning {{ordered comparison of function 
pointers}}
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -12711,6 +12711,13 @@
LHS.get()->getSourceRange());
   }
 
+  if (!IsOrdered && LHSType->isFunctionPointerType() &&
+  RHSType->isFunctionPointerType() && !LHSIsNull && !RHSIsNull) {
+Diag(Loc, diag::warn_typecheck_comparison_of_function_pointers)
+  << LHSType << 0
+  << LHS.get()->getSourceRange() << RHS.get()->getSourceRange();
+  }
+
   if (IsOrdered && LHSType->isFunctionPointerType() &&
   RHSType->isFunctionPointerType()) {
 // Valid unless a relational comparison of function pointers
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -7002,6 +7002,9 @@
   "%0 is %select{|in}2complete and "
   "%1 is %select{|in}3complete">,
   InGroup;
+def warn_typecheck_comparison_of_function_pointers : Warning<
+  "comparison of function pointers (%0 and %1)">,
+  InGroup, DefaultIgnore;
 def warn_typecheck_ordered_comparison_of_function_pointers : Warning<
   "ordered comparison of function pointers (%0 and %1)">,
   InGroup;
Index: clang/include/clang/Basic/DiagnosticGroups.td
===
--- clang/include/clang/Basic/DiagnosticGroups.td
+++ clang/include/clang/Basic/DiagnosticGroups.td
@@ -561,6 +561,7 @@
 def UnderalignedExceptionObject : DiagGroup<"underaligned-exception-object">;
 def DeprecatedObjCIsaUsage : DiagGroup<"deprecated-objc-isa-usage">;
 def ExplicitInitializeCall : DiagGroup<"explicit-initialize-call">;
+def CompareFunctionPointers : DiagGroup<"compare-function-pointers">;
 def OrderedCompareFunctionPointers : 
DiagGroup<"ordered-compare-function-pointers">;
 def PackedNonPod : DiagGroup<"packed-non-pod">;
 def Packed : DiagGroup<"packed", [PackedNonPod]>;


Index: clang/test/SemaCXX/compare-function-pointer.cpp
===
--- clang/test/SemaCXX/compare-function-pointer.cpp
+++ clang/test/SemaCXX/compare-function-pointer.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -std=c++20 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++20 -verify