--analyze does not produce warning when ignoring return values

[noloader]

Bugzilla Link	10094
Resolution	WONTFIX
Resolved on	Jun 06, 2011 22:44
Version	unspecified
OS	MacOS X
Attachments	Test program from OpenSSL website, Test program from OpenSSL website, Test program from OpenSSL website
CC	@tkremenek

Extended Description

The attached program (taken from http://www.openssl.org/docs/crypto/EVP_DigestInit.html) does not generate a warning when ignoring return values. EVP_DigestInit_ex, EVP_DigestUpdate, EVP_DigestUpdate, EVP_DigestFinal_ex, adn EVP_MD_CTX_cleanup each return 1 for success (see http://www.openssl.org/docs/crypto/EVP_DigestInit.html).

[noloader]

assigned to @tkremenek

[noloader]

clang was built from sources. It appears the issue affects both 1.7 (installed with Mac OS X) and 3.0 (fetched from SVN):

jeffrey@newton~/Desktop/openssl-test$ uname -a
Darwin newton 10.7.0 Darwin Kernel Version 10.7.0: Sat Jan 29 15:17:16 PST 2011; root:xnu-1504.9.371/RELEASE_I386 i386
jeffrey@newton/Desktop/openssl-test$ clang --analyze mdtest.c
jeffrey@newton~/Desktop/openssl-test$
...
jeffrey@newton~/Desktop/openssl-test$ /Desktop/build/Debug+Asserts/bin/clang --version
clang version 3.0 (trunk 132696)
Target: x86_64-apple-darwin10.7.0
Thread model: posix
jeffrey@newton/Desktop/openssl-test$ /Desktop/build/Debug+Asserts/bin/clang --analyze mdtest.c
jeffrey@newton/Desktop/openssl-test$

[tkremenek]

I'm not certain what this PR is asking for. Is this a feature request? The analyzer doesn't flag warnings for this case because it is something already handled by the compiler. If the function prototype is annotated with the 'warn_unused_result' attribute, the compiler will warn about these cases when it is passed the -Wunused-value command line option.

[tkremenek]

If clang doesn't produce a warning here when passing -Wunused-value, it is because the OpenSSL headers are not properly annotated. If that is the case, please file a bug against OpenSSL.

[noloader]

If clang doesn't produce a warning here when passing -Wunused-value, it is
because the OpenSSL headers are not properly annotated. If that is the case,
please file a bug against OpenSSL.
Thanks for the quick response.

Please forgive my ignorance here - shouldn't a static analyzer 'just work' with out the need for instrumentation and switches? Its the impression I got from the man page (nothing else was stated):

   --analyze
       Run the Clang Static Analyzer.

Perhaps I'm off-base a bit, but I'd expect clang to consume -Wunused-value for GCC compatibility. I would not expect clang --analyze to need GCC's -Wunused-value to report a result. I guess what I'm expecting is, "Its an analyzer - report everything unless I turn something off".

[tkremenek]

If clang doesn't produce a warning here when passing -Wunused-value, it is
because the OpenSSL headers are not properly annotated. If that is the case,
please file a bug against OpenSSL.
Thanks for the quick response.

Please forgive my ignorance here - shouldn't a static analyzer 'just work' with
out the need for instrumentation and switches? Its the impression I got from
the man page (nothing else was stated):

   --analyze
       Run the Clang Static Analyzer.

Perhaps I'm off-base a bit, but I'd expect clang to consume -Wunused-value for
GCC compatibility. I would not expect clang --analyze to need GCC's
-Wunused-value to report a result. I guess what I'm expecting is, "Its an
analyzer - report everything unless I turn something off".

The analyzer and the compiler should be viewed as different tools that complement each other. The analyzer is not meant to duplicate all the warnings that the compiler already can warn about. It's meant to be activated to do "deeper analysis" to report issues that complement that already found by the compiler. The fact that the analyzer uses the clang frontend is a detail. The frontend warnings you see on the command line are a by-product of the analyzer using the frontend. The actual output of the analyzer, however, is the HTML or plist output (viewable in Xcode, or possibly other IDEs).

Duplicating warnings in the analyzer that the compiler already does is counter-productive. The main problems is that it can opt users into warnings that they don't want (e.g., if they don't compiler their code with the appropriate warning, why would they want to see it with the analyzer?) and if they want the warning they will see duplicate warnings for the same problem.

There are other issues. Compiler warnings have fine-grain control that are baked into the compiler driver options. There is a known workflow for these issues. The analyzer has similar knobs, but they are different, because they correspond to a different tool. It's arguably bad that there are different knobs, but users should not need to understand both knobs to control the same warning. Further, the analyzer doesn't just "report everything." The analyzer has a default set of warnings that it emits, but there are opt-in warnings, just like there are with the compiler.

Finally, with respect to this given warning, how does the analyzer know that these function return values should not be ignored? That's certainly not a valid assumption for all functions. Unless these functions are annotated (which I don't know if they are or not), the analyzer isn't going to be able to do any better job than the compiler here. If it warned about every unused function call result, there would be tons of warning spew on real code. That's the purpose of the attribute: to capture the intention of the APIs and to indicate that stronger checking is necessary.

Annotations are a mixed bag. Yes, they require more effort, in this case on the API writer's part, but they are sometimes essentially when it comes to extracting the most signal from a warning. The analyzer (and compiler) only makes aggressive assumptions when it can; if it is too aggressive, you get far too many false positives for the tool to be useful.

The only way the analyzer might be able to help here, for functions that aren't explicitly annotated, is to do global statistical checking of return values. For example, if a called function's return value is checking 90% of the time, that's a good indicator that it's return value needs to be checked. That's a heuristic that is better suited for the analyzer, not the compiler, but that's completely different than just having the analyzer always issue this kind of warning. In this case, the analyzer would be adding value beyond what the compiler could warn about.

I'm fine with re-appropriating this PR for this last case, but I don't think that's what you intended.

[noloader]

If clang doesn't produce a warning here when passing -Wunused-value, it is
because the OpenSSL headers are not properly annotated. If that is the case,
please file a bug against OpenSSL.
Thanks for the quick response.

Please forgive my ignorance here - shouldn't a static analyzer 'just work' with
out the need for instrumentation and switches? Its the impression I got from
the man page (nothing else was stated):

   --analyze
       Run the Clang Static Analyzer.

Perhaps I'm off-base a bit, but I'd expect clang to consume -Wunused-value for
GCC compatibility. I would not expect clang --analyze to need GCC's
-Wunused-value to report a result. I guess what I'm expecting is, "Its an
analyzer - report everything unless I turn something off".

The analyzer and the compiler should be viewed as different tools that
complement each other. The analyzer is not meant to duplicate all the warnings
that the compiler already can warn about. It's meant to be activated to do
"deeper analysis" to report issues that complement that already found by the
compiler. The fact that the analyzer uses the clang frontend is a detail. The
frontend warnings you see on the command line are a by-product of the analyzer
using the frontend. The actual output of the analyzer, however, is the HTML or
plist output (viewable in Xcode, or possibly other IDEs).

[SNIP]
Thanks Ted. I was not aware there was a tight coupling.

I'm fine with re-appropriating this PR for this last case, but I don't think
that's what you intended.
If I'm the only guy asking, I would would probably say, "tough rocks".

Jeff