Page MenuHomePhabricator

Diagnose -Wunused-value in constant evaluation context
Needs ReviewPublic

Authored by ychen on Jun 8 2021, 5:50 PM.

Details

Summary

GCC/MSVC diagnoses in such cases, probably it makes sense to do the
same for Clang.
https://godbolt.org/z/7zxb8Tx96

It actually generated the warning internally, however since
Sema::DiagnoseUnusedExprResult called Sema::DiagRuntimeBehavior which
claims that "Relevant diagnostics should be produced by constant
evaluation.". So directly diagnose in Sema::DiagnoseUnusedExprResult
for constant evaluation context cases.

Diff Detail

Unit TestsFailed

TimeTest
1,930 msx64 debian > libFuzzer.libFuzzer::dataflow.test
Script: -- : 'RUN: at line 5'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/lib/fuzzer -m64 -c -fno-sanitize=all -fsanitize=dataflow /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/fuzzer/../../lib/fuzzer/dataflow/DataFlow.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/dataflow.test.tmp-DataFlow.o
119,690 msx64 debian > libFuzzer.libFuzzer::only-some-bytes-fork.test
Script: -- : 'RUN: at line 5'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/lib/fuzzer -m64 -c -fno-sanitize=all -fsanitize=dataflow /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/fuzzer/../../lib/fuzzer/dataflow/DataFlow.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/only-some-bytes-fork.test.tmp-DataFlow.o
8,119 msx64 debian > libFuzzer.libFuzzer::only-some-bytes.test
Script: -- : 'RUN: at line 5'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/lib/fuzzer -m64 -c -fno-sanitize=all -fsanitize=dataflow /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/fuzzer/../../lib/fuzzer/dataflow/DataFlow.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/only-some-bytes.test.tmp-DataFlow.o

Event Timeline

ychen requested review of this revision.Jun 8 2021, 5:50 PM
ychen created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2021, 5:50 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
rsmith added a comment.Jun 8 2021, 6:37 PM

This will diagnose unused values in unreachable code in constant-evaluated contexts; that doesn't seem quite right. For example, in:

void f() {
  new double[false ? (1, 2) : 3][false ? (1, 2) : 3];
}

... we'll diagnose that the 1 is unused in only one of the two array bounds, because one of them is a constant-evaluated context and the other is not.

I'd suggest we start by adding a separate DiagIfReachable function that does the same thing as DiagRuntimeBehavior except without the context check, with the intent to eventually refine it to check for reachability even in expressions for which we don't build a CFG (eg, in the initializer of a global or in a constant expression).

ychen updated this revision to Diff 351343.Jun 10 2021, 11:00 PM
  • Add DiagIfReachable and use it in Sema::DiagnoseUnusedExprResult.
  • Update tests.

This will diagnose unused values in unreachable code in constant-evaluated contexts; that doesn't seem quite right. For example, in:

void f() {
  new double[false ? (1, 2) : 3][false ? (1, 2) : 3];
}

... we'll diagnose that the 1 is unused in only one of the two array bounds, because one of them is a constant-evaluated context and the other is not.

I'd suggest we start by adding a separate DiagIfReachable function that does the same thing as DiagRuntimeBehavior except without the context check, with the intent to eventually refine it to check for reachability even in expressions for which we don't build a CFG (eg, in the initializer of a global or in a constant expression).

@rsmith Thanks for the pointer! Comments addressed. For the reachability check in contexts without CFG, it seems that some delayed processing is needed since constant evaluation happens later, however, I'm not sure about that.

I've given this some more thought, and I think it's only the "constant evaluated" check that we want to bypass in this case. It's common to use sizeof or decltype with a comma operator in order to put an expression in a SFINAE context, and I don't think we should warn on those cases. So I think we should still do the context check for DiagIfReachable, but only bail out for unevaluated contexts, not for constant-evaluated contexts. Does that seem reasonable?

clang/test/SemaCXX/warn-unused-value.cpp
148

Please add a FIXME saying that we shouldn't diagnose the unreachable constant expression here.

ychen updated this revision to Diff 355054.Mon, Jun 28, 3:56 PM
  • Add test case for 'constexpr if'
ychen added a comment.Mon, Jun 28, 3:57 PM

I've given this some more thought, and I think it's only the "constant evaluated" check that we want to bypass in this case. It's common to use sizeof or decltype with a comma operator in order to put an expression in a SFINAE context, and I don't think we should warn on those cases. So I think we should still do the context check for DiagIfReachable, but only bail out for unevaluated contexts, not for constant-evaluated contexts. Does that seem reasonable?

Yeah, it is reasonable and seems already in place (https://reviews.llvm.org/rG78ecb8737d97a038770af9f6a1ad1d7925c87eb7).

aaron.ballman added inline comments.Thu, Jul 15, 7:44 AM
clang/include/clang/Sema/Sema.h
5077
clang/test/Sema/i-c-e.c
77–78

I think this diagnostic is kind of unfortunate because it increases my confusion -- the expression result is most assuredly *not* unused in these cases because it's used in the definition of the type.

clang/test/SemaCXX/warn-unused-value.cpp
145

This is another one that tripped me up due to the diagnostic wording.

ychen updated this revision to Diff 360514.Wed, Jul 21, 10:36 AM
ychen marked an inline comment as done.
  • fix comment
clang/test/Sema/i-c-e.c
77–78

the expression result is most assuredly *not* unused in these cases because it's used in the definition of the type.

Do you mean "1" is used in the definition of the type? The warning is for "1" in this case. If I change 1 to any other number, the type of comma3 should not change, I think that means 1 is not used.

aaron.ballman added inline comments.Thu, Jul 22, 5:25 AM
clang/test/Sema/i-c-e.c
77–78

Do you mean "1" is used in the definition of the type? The warning is for "1" in this case. If I change 1 to any other number, the type of comma3 should not change, I think that means 1 is not used.

I agree that the 1 is unused.

The trouble is that the diagnostic doesn't make that clear -- it just says the expression result is unused, which makes it sound like the entire comma expression result value is unused (which is not the case). I think the diagnostic should more clearly specify *what* is unused in this case, otherwise users may think the diagnostic is a false positive.