[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<CompareFunctionPointers>, 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<CompareFunctionPointers>, 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 to you to have this ignored by default?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141310