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

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

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

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

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

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/

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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 >

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

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

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

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

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

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

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

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 :

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

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

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

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

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

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:

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

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

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

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

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

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

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

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:

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

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:

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

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

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

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

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

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

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

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

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

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