Page MenuHomePhabricator

[analyzer] Do not analyze bison-generated files
ClosedPublic

Authored by george.karpenkov on Feb 16 2018, 4:14 PM.

Details

Summary

Bison/YACC generated files result in a very large number of (presumably) false positives from the analyzer.
These false positives are "true" in a sense of the information analyzer sees: assuming that the lexer can return any token at any point a number of uninitialized reads does occur.
(naturally, the analyzer can not capture a complex invariant that certain tokens can only occur under certain conditions).

Current fix simply stops analysis on those files.
I have examined a very large number of such auto-generated files, and they do all start with such a comment.
Conversely, user code is very unlikely to contain such a comment.

rdar://33608161

Diff Detail

Repository
rC Clang

Event Timeline

Looks good, just a nit inline.

lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
522

Could we use Buffer->getBuffer().startswith(...) instead?

Actually, after posting this I've realized that this behavior is somewhat inconsistent, and we should still produce an empty plist when requested to: just as we do for files with no generated reports.

lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
522

we could if there was such a method? getBuffer() returns a MutableArrayRef

Actually, after posting this I've realized that this behavior is somewhat inconsistent, and we should still produce an empty plist when requested to: just as we do for files with no generated reports.

Actually, after thinking about this some more, the answer is those plists should not be produced, as they are not produced in similar cases (disableAllChecks is set, etc).

NoQ added inline comments.Feb 23 2018, 7:15 PM
lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
522

Hmm okay. So is the buffer actually null-terminated?

Actually, after posting this I've realized that this behavior is somewhat inconsistent, and we should still produce an empty plist when requested to: just as we do for files with no generated reports.

Actually, after thinking about this some more, the answer is those plists should not be produced, as they are not produced in similar cases (disableAllChecks is set, etc).

This seems to be about the contract that the analyzer promises to tools that ingest the plists. Will those tools be OK with missing plists? What do they expect?

a.sidorin added inline comments.Feb 26 2018, 7:21 AM
lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
522

Looks like this method of the base class is overridden with different return result. However, we are still allowed to use parent class methods:

llvm::WritableMemoryBuffer *WBuf;
llvm::MemoryBuffer *MBuf;
StringRef Str = MBuf->MemoryBuffer::getBuffer();
Str = WBuf->MemoryBuffer::getBuffer();
george.karpenkov marked an inline comment as done.

Always produces plists now, less brittle string check.

NoQ added inline comments.Feb 26 2018, 12:49 PM
lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
585

It seems that previously it didn't do the timer stop thing and whatever is below when analysis is skipped, now it does. Is this an intended or an accidental change?

lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
585

Intended, as "whatever is below" is the code producing the plist.

NoQ added inline comments.Feb 26 2018, 1:11 PM
lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
585

I mean, is it intended even in the Opts->DisableAllChecks mode?

585

Emm, wait, there's another return above. So that's dead code.

george.karpenkov marked an inline comment as done.
george.karpenkov added inline comments.
lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
585

I mean, is it intended even in the Opts->DisableAllChecks mode?

Yes, to me it seems logical to have less code paths and to always perform same wrapping, lest the clients expect that.

NoQ accepted this revision.Feb 26 2018, 1:32 PM

Yeah, looks correct. And they'd have to deal with some of these empty plists anyway now.

This revision is now accepted and ready to land.Feb 26 2018, 1:32 PM
This revision was automatically updated to reflect the committed changes.