[clang] add -Wcompare-function-pointers

[adriandole]

Summary

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.

[adriandole]

Release note.

[cjdb]

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

clang/include/clang/Basic/DiagnosticSemaKinds.td

7006

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.

7007

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

clang/lib/Sema/SemaExpr.cpp

12715

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

clang/test/SemaCXX/compare-function-pointer.cpp

9–10

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

17–18

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

[cjdb]

clang/include/clang/Basic/DiagnosticSemaKinds.td

7007

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.

[aaron.ballman]

clang/docs/ReleaseNotes.rst

451 ↗

(On Diff #487511)

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

clang/include/clang/Basic/DiagnosticSemaKinds.td

7006

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.

7007

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.

[adriandole]

clang/include/clang/Basic/DiagnosticSemaKinds.td

7006

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.

[aaron.ballman]

clang/include/clang/Basic/DiagnosticSemaKinds.td

7006

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.

[cjdb]

clang/include/clang/Basic/DiagnosticSemaKinds.td

7007

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?

[aaron.ballman]

clang/include/clang/Basic/DiagnosticSemaKinds.td

7007

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

[tschuett]

@MaskRay

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

[adriandole]

clang/include/clang/Basic/DiagnosticSemaKinds.td

7006

ld.gold and mold have it (same name, -icf=all) as does MSVC (/OPT:ICF).

[MaskRay]

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.

[aaron.ballman]

clang/docs/ReleaseNotes.rst

451 ↗

(On Diff #487511)

Changing this suggestion to: "that may have their behavior change when enabling the identical code folding optimization feature of some linkers."

clang/include/clang/Basic/DiagnosticGroups.td

565

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.

clang/include/clang/Basic/DiagnosticSemaKinds.td

7006

Thanks for the details, that makes this seem more reasonable in Clang itself IMO.

7007

@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?

[adriandole]

clang/include/clang/Basic/DiagnosticGroups.td

565

Makes sense, and it also fixes the issue of raising both this warning and -Wordered-compare-function-pointers on the same line. Will update.

[adriandole]

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

[MaskRay]

clang/include/clang/Basic/DiagnosticSemaKinds.td

7007

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

[dblaikie]

@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?

[aaron.ballman]

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.

[dblaikie]

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.

[dblaikie]

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.

[adriandole]

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

[dblaikie]

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.

[aaron.ballman]

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?

clang/test/SemaCXX/compare-function-pointer.cpp

17–18

+1, this feels like we're adding more heat without any light.

[dblaikie]

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.

[adriandole]

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

[aaron.ballman]

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.*&patternType=standard&sm=1&groupBy=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.)

[cjdb]

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

[dblaikie]

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.

[aaron.ballman]

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.

clang/include/clang/Basic/DiagnosticGroups.td

 def ObjCFlexibleArray : DiagGroup<"objc-flexible-array">;
 def ObjCBoxing : DiagGroup<"objc-boxing">;
 def CompletionHandler : DiagGroup<"completion-handler">;
 def CalledOnceParameter : DiagGroup<"called-once-parameter", [CompletionHandler]>;
 def OpenCLUnsupportedRGBA: DiagGroup<"opencl-unsupported-rgba">;
 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]>;
 def Padded : DiagGroup<"padded">;
 def UnalignedAccess : DiagGroup<"unaligned-access">;
 
 def PessimizingMove : DiagGroup<"pessimizing-move">;
 def ReturnStdMove : DiagGroup<"return-std-move">;
 

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def err_typecheck_three_way_comparison_of_pointer_and_zero : Error<
   "three-way comparison between pointer and zero">;
 def ext_typecheck_compare_complete_incomplete_pointers : Extension<
   "pointer comparisons before C11 "
   "need to be between two complete or two incomplete types; "
   "%0 is %select{|in}2complete and "
   "%1 is %select{|in}3complete">,
   InGroup<C11>;
+def warn_typecheck_comparison_of_function_pointers : Warning<
+  "comparison of function pointers (%0 and %1)">,
+  InGroup<CompareFunctionPointers>, DefaultIgnore;
 def warn_typecheck_ordered_comparison_of_function_pointers : Warning<
   "ordered comparison of function pointers (%0 and %1)">,
   InGroup<OrderedCompareFunctionPointers>;
 def ext_typecheck_ordered_comparison_of_function_pointers : ExtWarn<
   "ordered comparison of function pointers (%0 and %1)">,
   InGroup<OrderedCompareFunctionPointers>;
 def err_typecheck_ordered_comparison_of_function_pointers : Error<
   "ordered comparison of function pointers (%0 and %1)">;

clang/lib/Sema/SemaExpr.cpp

if (!IsOrdered && LHSIsNull != RHSIsNull) {
     if (RHSIsNull)
       DiagnoseAlwaysNonNullPointer(LHS.get(), RHSNullKind, IsEquality,
                                    RHS.get()->getSourceRange());
     else
       DiagnoseAlwaysNonNullPointer(RHS.get(), LHSNullKind, IsEquality,
                                    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
     bool IsError = Opc == BO_Cmp;
     auto DiagID =
         IsError ? diag::err_typecheck_ordered_comparison_of_function_pointers
         : getLangOpts().CPlusPlus
             ? diag::warn_typecheck_ordered_comparison_of_function_pointers

clang/test/SemaCXX/compare-function-pointer.cpp

-// 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 (*)();
 
 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}}
                     // expected-error@-1 {{comparison of distinct pointer types}}
 bool gt1 = a > c;   // expected-warning {{ordered comparison of function pointers}}
                     // expected-error@-1 {{comparison of distinct pointer types}}
 bool ge1 = a >= c;  // expected-warning {{ordered comparison of function pointers}}
                     // expected-error@-1 {{comparison of distinct pointer types}}
 auto tw1 = a <=> c; // expected-error {{ordered comparison of function pointers}}