This is an archive of the discontinued LLVM Phabricator instance.

[clang] add -Wcompare-function-pointers
Needs ReviewPublic

Authored by adriandole on Jan 9 2023, 11:11 AM.

Details

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.

Diff Detail

Event Timeline

adriandole created this revision.Jan 9 2023, 11:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 9 2023, 11:11 AM
adriandole requested review of this revision.Jan 9 2023, 11:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 9 2023, 11:11 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Release note.

cjdb added a comment.Jan 10 2023, 4:58 PM

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
7013

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.

7014

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.

19–20

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

cjdb added inline comments.Jan 10 2023, 4:59 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
7014

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 added inline comments.
clang/docs/ReleaseNotes.rst
454

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

clang/include/clang/Basic/DiagnosticSemaKinds.td
7013

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.

7014

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 added inline comments.Jan 11 2023, 9:51 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
7013

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 added inline comments.Jan 11 2023, 10:20 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
7013

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 added inline comments.Jan 11 2023, 10:31 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
7014

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 added inline comments.Jan 11 2023, 10:43 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
7014

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

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

adriandole added inline comments.Jan 11 2023, 1:11 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
7013

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

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 added inline comments.Jan 13 2023, 5:35 AM
clang/docs/ReleaseNotes.rst
454

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
566–567

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
7013

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

7014

@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 added inline comments.Jan 13 2023, 10:26 AM
clang/include/clang/Basic/DiagnosticGroups.td
566–567

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

  • More helpful diagnostic message.
  • Check variable and type names are printed in tests.
  • Enable -Wordered-function-pointer-comparison when this warning is enabled.
adriandole marked 5 inline comments as done.Jan 13 2023, 1:00 PM
MaskRay added inline comments.Jan 13 2023, 3:40 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
7014

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

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

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

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

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.

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

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
19–20

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

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.

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

adriandole marked 2 inline comments as done.Jan 19 2023, 2:57 PM

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 added a comment.Apr 20 2023, 3:17 PM

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

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.

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.