[cfe-commits] r172883 - in /cfe/trunk: include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h lib/StaticAnalyzer/Core/BugReporter.cpp test/Analysis/diagnostics/false-positive-suppression.c test/Analysis/diagnostics/include/ test/Analysis/diagnostics/include/sys/ test/Analysis/diagnostics/include/sys/queue.h
Anna Zaks
ganna at apple.com
Mon Jan 28 14:22:08 PST 2013
On Jan 18, 2013, at 7:19 PM, Anna Zaks <ganna at apple.com> wrote:
>
> On Jan 18, 2013, at 6:23 PM, Jordan Rose <jordan_rose at apple.com> wrote:
>
>> This is what BugReporterVisitors and BugReport::markInvalid are supposed to be used for.
>
>> Admittedly that means we do more work before invalidation, but it also means that hardcoded exceptions don't end up in BugReport. You could do this by implementing a custom BugReporterVisitor::getEndPath.
>>
>> (In my mind, BugReporterVisitors are like the Checkers of the diagnostic machinery.)
>>
>> What do you think?
>>
>
> We have markInvalid and call it from the visitors because we HAVE to visit the path for the suppression you've implemented. As you mention below, it is an overkill for this type of suppression. Also, it doesn't really traverse/visit the path at all (just looks at the enclosing Stmt). But I also agree that not having a separate check is a plus and the performance would be dwarfed by the analyzes time. Possibly, if we add the logic to a visitor, we could detect things like "goes through a macro" in the future..
>
I've tried refactoring and that lead to more reasons why visitors are an awkward option (at least the way they are used right now).
Previously, the visitors were used to enhance the diagnostic path (Extensive or Minimal). I am guessing that previous to the FP suppression work, they were never called when the generation scheme was PathDiagnosticConsumer::None. So we special cased that and now run the visitors on all the nodes but the last one for None generation scheme, just to get the invalidation info. We were not 100% consistent, since we are not calling the visitors for the getEndPath().
I would need to change the logic to run the visitors for the getEndPath() to get my suppression working, which is somewhat strange as the EndPath is not going to be used. But this would be consistent with the rest of invalidation visitor logic.
Is it possible to separate the regular visitors from the invalidation visitors and call them separately - the set of invalidation visitors would be called in all cases and the diagnostic enhancing visitors would be only called for report enhancements?
Cheers,
Anna.
>>
>> On Jan 18, 2013, at 18:18 , Anna Zaks <ganna at apple.com> wrote:
>>
>>> Author: zaks
>>> Date: Fri Jan 18 20:18:15 2013
>>> New Revision: 172883
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=172883&view=rev
>>> Log:
>>> [analyzer] Suppress warnings coming out of macros defined in sys/queue.h
>>>
>>> Suppress the warning by just not emitting the report. The sink node
>>> would get generated, which is fine since we did reach a bad state.
>>>
>>> Motivation
>>>
>>> Due to the way code is structured in some of these macros, we do not
>>> reason correctly about it and report false positives. Specifically, the
>>> following loop reports a use-after-free. Because of the way the code is
>>> structured inside of the macro, the analyzer assumes that the list can
>>> have cycles, so you end up with use-after-free in the loop, that is
>>> safely deleting elements of the list. (The user does not have a way to
>>> teach the analyzer about shape of data structures.)
>>>
>>> SLIST_FOREACH_SAFE(item, &ctx->example_list, example_le, tmpitem) {
>>> if (item->index == 3) { // if you remove each time, no complaints
>>> assert((&ctx->example_list)->slh_first == item);
>>> SLIST_REMOVE(&ctx->example_list, item, example_s, example_le);
>>> free(item);
>>> }
>>> }
>>>
>>> Added:
>>> cfe/trunk/test/Analysis/diagnostics/false-positive-suppression.c
>>> cfe/trunk/test/Analysis/diagnostics/include/
>>> cfe/trunk/test/Analysis/diagnostics/include/sys/
>>> cfe/trunk/test/Analysis/diagnostics/include/sys/queue.h
>>> Modified:
>>> cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
>>> cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp
>>>
>>> Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h?rev=172883&r1=172882&r2=172883&view=diff
>>> ==============================================================================
>>> --- cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h (original)
>>> +++ cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h Fri Jan 18 20:18:15 2013
>>> @@ -457,6 +457,11 @@
>>>
>>> void Register(BugType *BT);
>>>
>>> + /// \brief Suppress reports that might lead to known false positives.
>>> + ///
>>> + /// Currently this suppresses reports based on locations of bugs.
>>> + bool suppressReport(BugReport *R);
>>> +
>>> /// \brief Add the given report to the set of reports tracked by BugReporter.
>>> ///
>>> /// The reports are usually generated by the checkers. Further, they are
>>>
>>> Modified: cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp?rev=172883&r1=172882&r2=172883&view=diff
>>> ==============================================================================
>>> --- cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp (original)
>>> +++ cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp Fri Jan 18 20:18:15 2013
>>> @@ -2137,7 +2137,32 @@
>>> BugTypes = F.add(BugTypes, BT);
>>> }
>>>
>>> +bool BugReporter::suppressReport(BugReport *R) {
>>> + const Stmt *S = R->getStmt();
>>> + if (!S)
>>> + return false;
>>> +
>>> + // Here we suppress false positives coming from system macros. This list is
>>> + // based on known issues.
>>> +
>>> + // Skip reports within the sys/queue.h macros as we do not have the ability to
>>> + // reason about data structure shapes.
>>> + SourceManager &SM = getSourceManager();
>>> + SourceLocation Loc = S->getLocStart();
>>> + while (Loc.isMacroID()) {
>>> + if (SM.isInSystemMacro(Loc) &&
>>> + (SM.getFilename(SM.getSpellingLoc(Loc)).endswith("sys/queue.h")))
>>> + return true;
>>> + Loc = SM.getSpellingLoc(Loc);
>>> + }
>>> +
>>> + return false;
>>> +}
>>> +
>>> void BugReporter::emitReport(BugReport* R) {
>>> + if (suppressReport(R))
>>> + return;
>>> +
>>> // Compute the bug report's hash to determine its equivalence class.
>>> llvm::FoldingSetNodeID ID;
>>> R->Profile(ID);
>>>
>>> Added: cfe/trunk/test/Analysis/diagnostics/false-positive-suppression.c
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/diagnostics/false-positive-suppression.c?rev=172883&view=auto
>>> ==============================================================================
>>> --- cfe/trunk/test/Analysis/diagnostics/false-positive-suppression.c (added)
>>> +++ cfe/trunk/test/Analysis/diagnostics/false-positive-suppression.c Fri Jan 18 20:18:15 2013
>>> @@ -0,0 +1,23 @@
>>> +// RUN: %clang_cc1 -analyze -analyzer-checker=core,unix -verify %s
>>> +// expected-no-diagnostics
>>> +
>>> +#include "include/sys/queue.h"
>>> +
>>> +typedef __typeof(sizeof(int)) size_t;
>>> +void *malloc(size_t);
>>> +
>>> +int radar12491259() {
>>> + int *p = malloc(12);
>>> + FREE_POINTER(p);
>>> + FREE_POINTER(p); // no-warning: we are suppressing errors coming from sys/queue macros.
>>> + return 0;
>>> +}
>>> +
>>> +#define MYMACRO(p) FREE_POINTER(p)
>>> +
>>> +int radar12491259_inside_macro() {
>>> + int *p = malloc(12);
>>> + MYMACRO(p);
>>> + MYMACRO(p); // no-warning: we are suppressing errors coming from sys/queue macros.
>>> + return 0;
>>> +}
>>>
>>> Added: cfe/trunk/test/Analysis/diagnostics/include/sys/queue.h
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/diagnostics/include/sys/queue.h?rev=172883&view=auto
>>> ==============================================================================
>>> --- cfe/trunk/test/Analysis/diagnostics/include/sys/queue.h (added)
>>> +++ cfe/trunk/test/Analysis/diagnostics/include/sys/queue.h Fri Jan 18 20:18:15 2013
>>> @@ -0,0 +1,5 @@
>>> +#pragma clang system_header
>>> +
>>> +void free(void *);
>>> +#define FREE_POINTER(x) free(x)
>>> +
>>>
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130128/429446d7/attachment.html>
More information about the cfe-commits
mailing list