Page MenuHomePhabricator

[clang] Eliminate relational function pointer comparisons in all C++ modes
Needs ReviewPublic

Authored by mizvekov on Jun 21 2021, 7:12 PM.

Details

Reviewers
rsmith
Summary

Word on the grapevine is that the committee has unanimous
agreement on eliminating relational function pointer comparisons.
There isn't any wording on it or anything, yet, but we must jump the gun here
and just do away with it pronto.

Signed-off-by: Matheus Izvekov <mizvekov@gmail.com>

Diff Detail

Unit TestsFailed

TimeTest
2,020 msx64 debian > AddressSanitizer-x86_64-linux-dynamic.TestCases/Posix::coverage-module-unloaded.cpp
Script: -- : 'RUN: at line 3'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only -m64 -shared-libasan -fsanitize-coverage=func,trace-pc-guard -DSHARED /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/asan/TestCases/Posix/coverage-module-unloaded.cpp -shared -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/asan/X86_64LinuxDynamicConfig/TestCases/Posix/Output/libcoverage-module-unloaded.cpp.dynamic1.so -fPIC
4,050 msx64 debian > AddressSanitizer-x86_64-linux.TestCases/Posix::coverage-module-unloaded.cpp
Script: -- : 'RUN: at line 3'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only -m64 -fsanitize-coverage=func,trace-pc-guard -DSHARED /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/asan/TestCases/Posix/coverage-module-unloaded.cpp -shared -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/asan/X86_64LinuxConfig/TestCases/Posix/Output/libcoverage-module-unloaded.cpp.dynamic1.so -fPIC
230 msx64 debian > Clang.FixIt::fixit.cpp
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -cc1 -internal-isystem /var/lib/buildkite-agent/builds/llvm-project/build/lib/clang/13.0.0/include -nostdsysteminc -pedantic -Wall -Wno-unused-but-set-variable -Wno-comment -verify -fcxx-exceptions -x c++ -std=c++98 /var/lib/buildkite-agent/builds/llvm-project/clang/test/FixIt/fixit.cpp
30 msx64 debian > Clang.Parser::cxx-template-argument.cpp
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -cc1 -internal-isystem /var/lib/buildkite-agent/builds/llvm-project/build/lib/clang/13.0.0/include -nostdsysteminc -fsyntax-only -verify /var/lib/buildkite-agent/builds/llvm-project/clang/test/Parser/cxx-template-argument.cpp
30 msx64 debian > Clang.Sema::compare.c
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -cc1 -internal-isystem /var/lib/buildkite-agent/builds/llvm-project/build/lib/clang/13.0.0/include -nostdsysteminc -triple x86_64-apple-darwin -fsyntax-only -pedantic -verify -Wsign-compare -Wtautological-constant-in-range-compare /var/lib/buildkite-agent/builds/llvm-project/clang/test/Sema/compare.c -Wno-unreachable-code
View Full Test Results (13 Failed)

Event Timeline

mizvekov requested review of this revision.Jun 21 2021, 7:12 PM
mizvekov created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJun 21 2021, 7:12 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
mizvekov edited the summary of this revision. (Show Details)Jun 21 2021, 7:37 PM
mizvekov edited the summary of this revision. (Show Details)Jun 21 2021, 7:58 PM
mizvekov updated this revision to Diff 353596.Jun 22 2021, 3:47 AM
  • Update more tests.
  • Slightly improved error recovery: deduce type for non-three-way relational comparisons.
Herald added a project: Restricted Project. · View Herald TranscriptJun 22 2021, 3:47 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
mizvekov updated this revision to Diff 353651.Jun 22 2021, 7:59 AM
  • Add one more test case.
  • Formating
mizvekov added inline comments.Jun 22 2021, 8:15 AM
clang/test/Parser/cxx-template-argument.cpp
28 ↗(On Diff #353651)

So here we are recovering from the parser error into this type check error.
Maybe there is something that could be improved as a follow up task so we don't get a double error.

There isn't any wording on it or anything, yet, but we must jump the gun here and just do away with it pronto.

I think "must" is the wrong word here. "Might as well"? Anyway, I agree with this general idea, FWLIW.

clang/lib/Sema/SemaExpr.cpp
11815

Peanut gallery says: Is QualType() the right "placeholder" to return here? IIUC, this is the situation where we've diagnosed an ill-formed expression and are just trying to do error-recovery: if the expression looks like x < y then we assume the programmer wants it to return bool, and if the expression looks like x <=> y then we assume the programmer wants it to return... QualType()? Is that the same thing we'd do for e.g. x + y or undeclaredfunction(x)? (If so, good, I think.)

clang/test/Parser/cxx-template-argument.cpp
28 ↗(On Diff #353651)

Since this is a parsing test, not a semantics test, I think it should avoid doing anything sketchy semantic-wise. It should just be rewritten as something like

struct RHS {
    friend void operator==(void(*)(), RHS) {}
    friend void operator>=(void(*)(), RHS) {}
};
(void)(&t<int>==RHS());
(void)(&t<int>>=RHS());
(void)(&t<S<int>>==RHS());
(void)(&t<S<int>>>=RHS());
clang/test/SemaCXX/compare-function-pointer.cpp
13

I believe you should also test these same cases for a OP c where c is a different function pointer type, e.g. int (*c)();

clang/test/SemaTemplate/resolve-single-template-id.cpp
73–75 ↗(On Diff #353651)

Cast (void)(x < y) here to suppress one of these irrelevant warnings.
The combination of warning "expr always evaluates to false" and erroring "expr is ill-formed" is also silly, but I suppose we can't do much about it.

mizvekov added inline comments.Jun 22 2021, 9:03 AM
clang/lib/Sema/SemaExpr.cpp
11815

So right now, this is the same we are doing for three-way comparison between builtin types we do not support / recognize. Check just below in this same function. Just look for the call to getComparisonCategoryForBuiltinCmp.

I agree completely with you and I was going to raise the same point, but I would prefer we changed both places at once instead of fixing it just here, so I think this should be a follow up task: Find something more appropriate to return here.

clang/test/Parser/cxx-template-argument.cpp
28 ↗(On Diff #353651)

I like that idea.

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

Sounds good.

clang/test/SemaTemplate/resolve-single-template-id.cpp
73–75 ↗(On Diff #353651)

I tried avoid changing the original test because I am not sure what the original intention was, but I agree in principle the two errors already give enough indication that the compiler is figuring out what is happening here correctly.

mizvekov updated this revision to Diff 353805.Jun 22 2021, 3:07 PM

Implement 2/3 of Arthur's suggestions.

mizvekov added inline comments.Jun 22 2021, 3:35 PM
clang/test/SemaTemplate/resolve-single-template-id.cpp
73–75 ↗(On Diff #353651)

Taking another look at this test, it is doing too much weird stuff like this all over the place, and I am a bit hesitant to make such a change because the test scope is really not clear here...

I would be more open to suppressing this warning by passing a command line flag, but even then I would wait for a second opinion.

Some crazy talk on my part here, but I wonder a bit if this is somehow bothering us because there are so many diagnostics thrown in such small amount of code. Perhaps a bit like how we might feel pointing someone out for too many errors is not very social?
Suppose the user writes this 10 character statement and he gets one error and two warnings, that might offend him a little bit, maybe?

mizvekov marked 2 inline comments as done.Jun 23 2021, 2:44 PM

I would prefer to split this into two changes:

  1. Stop providing a builtin overload candidate R operator<=>(P, P) where P is a function pointer type, to be consistent with the behavior of the built-in <=> operator.
  2. Stop providing </<=/>/>= support for function pointer types.

I think (1) is a (relatively) safe change that we can make immediately with very little roll-out concern. <=> is new, and the built-in candidate would never work anyway, so this is unlikely to cause problems for any real code, and has had clear and unanimous support in C++ committee discussions. So let's do that.

I think (2) is a lot riskier for existing code, and I think there's at least some chance that the C++ committee will end up effectively asking for change (1) but not change (2). It also looks like this part of the change will require a matching change in the standard library, because std::less on function pointer types would presumably still be required to work. So I think a safer approach here would be to add an enabled-by-default warning for relational comparisons of function pointers now, and return to this later to make the comparisons ill-formed if it still looks like the C++ committee is moving in that direction.

Does that sound reasonable?

clang/lib/Sema/SemaExpr.cpp
11807
11815

A null QualType is in this case treated as an indication that the comparison is invalid, recovery failed, and the callee has already produced a diagnostic. (It's a bit sad that we don't generally use a separate TypeResult for this kind of thing in order to distinguish between the "no type" answer and the "invalid and I've produced a diagnostic" answer, but if we wanted to change that I think we should aim to do it holistically across all of Clang.)

clang/lib/Sema/SemaOverload.cpp
9226–9228

An /*IsOrdered*/ comment here and below (or use of an enum) would aid readability.

I would prefer to split this into two changes:
...
Does that sound reasonable?

Yeah that is fine, totally understand ;)

clang/lib/Sema/SemaExpr.cpp
11815

Hmm. My thinking here is that it would be totally appropriate to error recovery with a 'strong_ordering' type, since that can decay into all the other orderings, making it kind of a safe bet that it would not cause further problems.

The idea of a separate type for error recovery is intriguing. But I wonder if it would be appropriate in some cases, but not all of them.

  • The cases where all the options are equally bad and we have no good guesses would fit the bill, but this is not the case here.
  • When we have a good safe bet, we can in principle just use that type. But maybe it would make sense to add a bit to it that means something like "but don't take this too seriously", which could be used to selectively suppress some diagnostics downstream.

First part of the split implemented at: https://reviews.llvm.org/D104892
I will reuse this DR for the second part.

mizvekov updated this revision to Diff 354920.Jun 28 2021, 9:05 AM
  • Rebase after split.
  • Add tests which I had forgotten, covering the builtin candidates for the relational operators other than spaceship.
mizvekov updated this revision to Diff 354924.Jun 28 2021, 9:13 AM
  • Fix missing period as pointed by rsmith.
mizvekov marked 2 inline comments as done.Jun 28 2021, 9:13 AM
joerg added a subscriber: joerg.Jun 30 2021, 2:53 PM

Is there any reason for breaking C++03 code here? I don't see the advantage to that at all.