This is an archive of the discontinued LLVM Phabricator instance.

MPI-Checker patch for Clang Static Analyzer
ClosedPublic

Authored by Alexander_Droste on Sep 10 2015, 7:41 AM.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Thanks for the review!

tools/clang/lib/StaticAnalyzer/Checkers/Checkers.td
524 ↗(On Diff #34664)

I ran the checker on CombBLAS.
All reports were correct and nothing crashed. -> changed the help text

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Container.h
30 ↗(On Diff #34664)

I think it would belong to <algorithm>. As this is part of libc++, how shall I proceed?

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.cpp
25 ↗(On Diff #34664)

I originally intended to additionally use warnings but then decided
to mark all reports as error types. I simply forgot to remove the warning
constant from the code.

114 ↗(On Diff #34664)

Yes, the last user can be in another file.
I'll look into the visitor implementation.

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerPathSensitive.cpp
24 ↗(On Diff #34664)

I changed the parameter name to PreCallEvent.

32 ↗(On Diff #34664)

The problem about symbolic regions is rather in collectUsedMemRegions() than
in checkDoubleNonblocking() why the guard is again used in checkWaitUsage().
From my understanding, it is not possible to find out for a symbolic region what element count and subregions (in case of an array) it has. All elements of a request array are supposed to be marked as waited in checkWaitUsage() if the wait function used is MPI_Waitall(). MPI_Waitall() receives an array of requests as an argument, in order to wait for all requests (or rather nonblocking operations) to complete. collectUsedMemRegions() iterates over the array to collect all single requests used. Therefore, the region is cannot be symbolic.

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPITypes.h
64 ↗(On Diff #34664)

I updated the comment.

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Utility.h
15 ↗(On Diff #34664)

Ok, I leave this unmodified for the moment.

tools/clang/test/Analysis/MPIChecker.c
114 ↗(On Diff #34664)
  1. Yes, that's not very common but possible.
  2. Does this change involve the use of a BugReporterVisitor?!

You're right, it's very similar.

Alexander_Droste edited edge metadata.
Alexander_Droste marked 4 inline comments as done.
  • capitalized bug types
  • made "MPI Error" constant a member variable
  • removed unused "MPI Warning" constant
  • renamed precallevent parameter
  • cxx type matching support
  • null pointer type matching support
  • changed help description text to "Checks MPI code"

It's more user friendly to report this issue at the last point where the request is available rather than the last line of the function.
This looks similar to leak report checking. Is it?

Yes, that's not very common but possible.
Does this change involve the use of a BugReporterVisitor?!

You should register/report the diagnostic at check::DeadSymbols event instead of check:: EndFunction; that way the error will be reported earlier.

You're right, it's very similar.

Alexander_Droste marked 2 inline comments as done.Sep 18 2015, 2:13 AM

It's more user friendly to report this issue at the last point where the request is available rather than the last line of the function.
This looks similar to leak report checking. Is it?

Yes, that's not very common but possible.
Does this change involve the use of a BugReporterVisitor?!

You should register/report the diagnostic at check::DeadSymbols event instead of check:: EndFunction; that way the error will be reported earlier.

You're right, it's very similar.

Sorry, I should have been more precise. The check tests if a request is in use by a nonblocking
call at the end of a function. But the request can still be alive after return. You can think of it as
a pointer for which the memory must be freed in the function it was allocated. But the pointer
can exist in global space outside a function. Multiple functions can use the same request.
So the symbol is not necessarily dead at the end of a function.

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerPathSensitive.cpp
33 ↗(On Diff #34982)

Is that assumption correct?

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPITypes.h
65 ↗(On Diff #34982)

The LastUser defines the state of the request and is used in the diagnostic.
As the report will make use of the BugReportVisitor because the LastUser can
be in another file, should I change the state marking to make use of an enum?
Currently the function classifier is used, to check what kind of function
the LastUser is, to obtain the requests state. Would this fail if the LastUser is from another file?

Alexander_Droste updated this revision to Diff 35175.EditedSep 19 2015, 11:36 AM
Alexander_Droste edited edge metadata.
Alexander_Droste marked an inline comment as done.

Meanwhile, I sketched a BugReportVisitor class for the 'double wait' check called
DoubleWaitVisitor which I modeled as a nested class of MPIBugReporter.
Currently, the retrieval of requests always fails in VisitNode(). (see // This is never reached...)
I'm not sure what's wrong, as I set up the class in the same way as presented in other checkers.
Maybe I overlooked something.

Further, I have some questions about the BugReportVisitor mechanism:

Why does the BugReportVisitor have a Profile() function?

// Why is this pattern used in most BugReporterVisitorImpl subclasses in the Profile() function?
static int X = 0;
ID.AddPointer(&X);

Does the function calling VisitNode() traverse backwards in the graph, as long as nullptr is returned?

Alexander_Droste marked 2 inline comments as done.Sep 20 2015, 11:43 AM
Alexander_Droste added inline comments.
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.cpp
137 ↗(On Diff #35175)

I figured out what the problem about the request retrieval is:
REGISTER_MAP_WITH_PROGRAMSTATE(RequestMap, const clang::ento::clang::mpi::Request)
The documentation states: "The macro should not be used inside namespaces, or for traits that must
be accessible from more than one translation unit." I think I need some advice here, how to make the
same RequestMap accessible in multiple translation units.

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.cpp
137 ↗(On Diff #35175)

How about this:
We could capture the PathDiagnosticLocation of the LastUser in each mpi::Request as a member. The RequestMap could then be local to MPICheckerPathSensitive.cpp and no visitors would be needed. The bug reporter class would then not need to know anything about the request map. The only function I can't find to realise this concept would be something like: Report->addPathDiagnosticLocation().

Sorry, I should have been more precise. The check tests if a request is in use by a nonblocking
call at the end of a function. But the request can still be alive after return. You can think of it as
a pointer for which the memory must be freed in the function it was allocated. But the pointer
can exist in global space outside a function. Multiple functions can use the same request.
So the symbol is not necessarily dead at the end of a function.

Is it the API requirement that the wait is performed in the same function?

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.cpp
137 ↗(On Diff #35175)

Almost all other checkers occupy a single file per checker. This is not much more complicated than the others so you could do the same. This would probably be better for consistency, but I do not have such a strong opinion about it and see why you might want to have separate files.

See TaintManager.h for the example of how to share the program state maps across translation units.

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPITypes.h
66 ↗(On Diff #35175)

Look at the ways the other checkers reference a node that occurred earlier along the symbolic execution path. For example, malloc checker tracks the state of each pointer, which could be "Allocated", "Freed"... When we report a leak, for example, the state would be "Allocated" and the symbol would be dead. The BugReporterVisitor walks up the symbolic path on which the issue occurred and prints an extra note at the place where the node is allocated. It knows at which node the pointer is allocated because that is where the state of the pointer changes from untracked to "Allocated".

Sorry, I should have been more precise. The check tests if a request is in use by a nonblocking
call at the end of a function. But the request can still be alive after return. You can think of it as
a pointer for which the memory must be freed in the function it was allocated. But the pointer
can exist in global space outside a function. Multiple functions can use the same request.
So the symbol is not necessarily dead at the end of a function.

Is it the API requirement that the wait is performed in the same function?

That's a good catch. It is no API requirement that a nonblocking call is matched by a wait within
the scope of a function. I think for C this would be reasonable but for CPP it is not sufficient,
as the matching could be realised with RAII or lambdas. I will change the check so that
it makes use of the checkDeadSymbols callback.

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.cpp
137 ↗(On Diff #35175)

Thanks for pointing out TaintManager.h. I'll try to derive a solution from that.

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPITypes.h
66 ↗(On Diff #35175)

Ok, I will use the same pattern. Thanks for the explanation.

dcoughlin added inline comments.Sep 24 2015, 6:06 PM
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerPathSensitive.cpp
34 ↗(On Diff #35175)

You may be able to use StoreManager::iterBindings(), which iterates over symbolic bindings in the store, to handle symbolic regions. See StackAddrEscapeChecker::checkEndFunction() for an example.

  • changed RequestMap so that the same is used across translation units
  • usage of a BugReporterVisitor in reportMissingWait, reportDoubleWait, reportDoubleNonblocking (reportUnmatchedWait needs no visitor)
  • based missingWait check on checkDeadSymbol callback
  • doubleWaits & doubleNonblocking errors where the 'same' function uses a request twice (e.g. in a loop) got a specific diagnostic, to make the report easily understandable
  • fine grained guard in checkDoubleNonblocking & checkWaitUsage
  • generalized variable name retrieval (for memory region)
  • handle multidimensional arrays in variable name retrieval (for memory region)
  • added/adapted integration tests
  • removed some helper functions in MPIBugReporter and Utility
  • removed UtilityTest.cpp

One thing that is still not perfect, is where the last part of the diagnostic
is presented for missing waits. This does not consistently seem to be the end of the scope
of the request (the point where the symbol dies). I compared this with the malloc checker which has
the same behavior. The last diagnostic seems to be presented after the next statement below the last function,
marked by the BugReportVisitor, using the request/memory. I think it would be nice if the last
part of the diagnostic would be presented at the end of the scope.

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerPathSensitive.cpp
35 ↗(On Diff #35718)

Thanks for the hint! I tried iterBindings but can't get it to add more functionality.
It seems to me that only TypedRegions are iterable.

  • dynamic init of MPICheckerPathSensitive in checkDeadSymbols()
  • updated RequestMap comment

We gave a talk about building checkers a while back; even though it does not talk about the bug reporter visitors, it might be worth watching if you haven't already seen it.

http://llvm.org/devmtg/2012-11/
Building a Checker in 24 hours

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerPathSensitive.cpp
68 ↗(On Diff #35800)

There is no reason to add an empty transition to the graph. Maybe you want to get the predecessor instead?

106 ↗(On Diff #35800)

generateNonFatalErrorNode will add a transition to the exploded graph, which means that you should only generate it if there is an error and you do not need to call addTransition later in the function.

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPITypes.h
67 ↗(On Diff #35800)

Still it looks like you have several pieces of information in the state that are redundant.. Looks like you've added more now.

For example, why do we need RequestId? Each report will only talk about a single request, is this not the case?

Do you absolutely need to store LastUser and VariableName?

Note that storing in the state is very expensive; on the other hand, we can afford to perform costly analysis on post-processing of an error report. This is why all other checkers store minimal information in the state, usually just a single enum and determine all the other information during the walk over the error path in BugReporterVisitor. The visitor will visit all nodes that you visited while reporting this bug, so it should have access to all the information you need.

We gave a talk about building checkers a while back; even though it does not talk about the bug reporter visitors, it might be worth watching if you haven't already seen it.

I have watched it some months ago but I'm sure it is a good idea to revisit the talk. Thanks for the idea!

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerPathSensitive.cpp
68 ↗(On Diff #35800)

Yes, that's better!

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPITypes.h
67 ↗(On Diff #35800)

I need the RequestID to distinct function calls of the same type
and location using the request multiple times along the path.
Like in a loop:

for int i ...
   nonblocking(.., request) // double nonblocking

Do you absolutely need to store LastUser and VariableName?

Absolutely not. :D I will remove the string member.
The variable name retrieval can be delayed to the point where the report is created.

I can also remove the enum, as the state of the request can be derived from the LastUser.
The reason why I added the enum member, is that I was concerned about that the CallEventRef
can be invalid to reference again if the call is inlined from an implementation defined in a header.
As this seems not to be the case, I can remove the redundancy.

This is why all other checkers store minimal information in the state, usually just a single enum and determine all the other information during the walk over the error path in BugReporterVisitor.

Ah, I get it. Until now that seemed a bit overly complicated to me.

zaks.anna added inline comments.Sep 30 2015, 1:51 PM
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPITypes.h
67 ↗(On Diff #35800)

I suspect the only thing you need in Request is the enum; everything else can be determined while visiting the path.

This should be in ento namespace not in the clang namespace.

Sorry about the delay.

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPITypes.h
67 ↗(On Diff #35800)

I think you're right that the enum is sufficient.
Thus for better diagnostics the source range
of the CallEventRef (LastUser)
should be obtained by the BugReportVisitor.
Is it possible to obtain a CallEventRef based on
a memory region? So conceptually this would mean
to find the last function call (before the error node) using
a specific region.

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPITypes.h
67 ↗(On Diff #35800)

I'll give this pattern a try:

const Stmt *S = nullptr;
ProgramPoint ProgLoc = N->getLocation();
if (Optional<StmtPoint> SP = ProgLoc.getAs<StmtPoint>()) {
  S = SP->getStmt();
}
// if S then S->getSourceRange

Looks like it could work.

  • reduced the Request class members to single 1 byte sized enum
  • implemented VisitNode for RequestNodeVisitor to find previous call event using the request
  • tolerate null pointer constants in type checking (any MPI type is accepted as a combination)
  • removed double wait check as it turned out that calling a wait on a request that was previously used by a wait is technically valid
  • minor improvement for variableName function
  • simplification of logic related to null pointer buffer (checkBufferTypeMatch)
  • detect missing waits on global variables
  • some cleanup
  • pass files being part of the diff with relative instead of absolute paths
Alexander_Droste marked 26 inline comments as done.Nov 2 2015, 6:14 AM

Hi,

I think the points mentioned should be all addressed now.
The only thing left is that some of the more generic functionality
maybe should be moved from MPI-Checker related files to others.
Therefor, I need some advice where to put these functions (if necessary).

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Container.h
31 ↗(On Diff #38900)

I just found /include/llvm/ADT/STLExtras.h. Wouldn't that be a good fit for the contains() function?

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerPathSensitive.cpp
107 ↗(On Diff #38900)

I need addTransition() later to take removed requests into account.

I've been mostly looking at the path sensitive checks. Maybe clang-tidy team would be interested in reviewing the syntactic checks.

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Container.h
31 ↗(On Diff #38900)

This sounds right.
I suggest submitting separate patches to the corresponding components.

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerPathSensitive.cpp
107 ↗(On Diff #38900)

What you have now, is generating a fork - two successors from the initial predecessor. You need to use the addTransition method that takes a predecessor and pass the ErrorNode there. You should also use the State from that node, not the initial state.

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Utility.h
54 ↗(On Diff #38900)

Since this takes no arguments but MemRegion, seems like it could go on MemRegion. Please, submit this as a separate patch.

alexfh added a comment.Dec 7 2015, 4:17 AM

Before I can tell you whether this checker makes sense as a clang-tidy check, can you please explain to me (don't want to deduce this from the code) what specific issues does this check target? Is this limited to a set of AST patterns or is there (or is going to be) some path-based or data flow analysis?

This patch contains a mix of checks, some are path sensitive and some are AST checks. See MPICheckerAST.cpp for the latter.

alexfh added a comment.Dec 7 2015, 3:55 PM

The only conclusion I can make is that this patch is huge and very inconvenient to review. I'm fine with moving the AST-based checks to clang-tidy, but I strongly prefer each separate check to go in a separate patch.

@anna
Thanks for having a look once more! I will submit these parts as separate patches.

@Alexander
This should be only about the AST-based checks, as Anna takes care of the path-sensitive ones.
I think this is not about moving the checks to clang-tidy because they are not linters but detect
errors. The AST-based functionality is roughly structured as followed:

MPIChecker.cpp : This is the entry point class where checkASTDecl is the only callback for AST-based
checks. There the TranslationUnitVisitor is used which traverses the complete TU passed.
During the traversal each check is executed in the course of the VisitCallExpr callback.
The AST-based checks are implemented in MPICheckerAST.(cpp|h). The TypeVisitor , contained
in TypeVisitor.h, is used as a helper class, in order to detect type properties.

This patch contains 3 AST-based checks (type mismatch, incorrect buffer referencing, invalid argument type)
which are described here in detail (see 3.1 AST-Based Checks) :
https://dl.acm.org/ft_gateway.cfm?id=2833159&ftid=1644251&dwn=1&CFID=566596797&CFTOKEN=98261226

alexfh added a comment.Dec 8 2015, 5:00 AM

@anna
Thanks for having a look once more! I will submit these parts as separate patches.

@Alexander
This should be only about the AST-based checks, as Anna takes care of the path-sensitive ones.
I think this is not about moving the checks to clang-tidy because they are not linters but detect
errors.

Well, many clang-tidy checks have a close to 100% true positive rate and also detect errors of different kinds (as opposed to purely stylistic issues). See http://clang.llvm.org/extra/clang-tidy/checks/misc-undelegated-constructor.html for an example. There's nothing that prevents adding more error-detecting checks to clang-tidy.

I also think, that true power of the static analyzer is the path-based analysis capability, but clang-tidy provides a more convenient way to implement AST- or preprocessor-based checks, so most of the AST-based checks should live there (unless they are generic and high-quality enough to be a compiler diagnostic).

The AST-based functionality is roughly structured as followed:

I'm not an expert in the static analyzer code, so I'm not the one to review even AST-based checks there. However, if you decide to move them to clang-tidy, I'll be happy to review that (I suggest introducing a top-level module for MPI-related checks and putting all relevant AST-based checks there).

MPIChecker.cpp : This is the entry point class where checkASTDecl is the only callback for AST-based
checks. There the TranslationUnitVisitor is used which traverses the complete TU passed.
During the traversal each check is executed in the course of the VisitCallExpr callback.
The AST-based checks are implemented in MPICheckerAST.(cpp|h). The TypeVisitor , contained
in TypeVisitor.h, is used as a helper class, in order to detect type properties.

This patch contains 3 AST-based checks (type mismatch, incorrect buffer referencing, invalid argument type)
which are described here in detail (see 3.1 AST-Based Checks) :
https://dl.acm.org/ft_gateway.cfm?id=2833159&ftid=1644251&dwn=1&CFID=566596797&CFTOKEN=98261226

Ah ok, I wasn't aware that clang-tidy is not restricted to checks which verify stylistic issues.
What makes it more convenient to integrate the checks in clang-tidy? Is it how the AST-Matcher
functionality can be accessed?

I'm not an expert in the static analyzer code, so I'm not the one to review even AST-based checks there.

The AST-based checks basically use zero functionality from the static analyzer codebase
except the entry callback and the functionality to generate bug reports.

There's nothing that prevents adding more error-detecting checks to clang-tidy.

Wrt. to the checks being part of this patch I have some concerns:

The first problem I see is that some functionlity is shared between the AST-based
and path-sensitive checks. But I think this should be restricted to the MPIFunctionClassifier class.

Another question I have is how this would affect the usability. At the moment
all detected bugs (AST-based and path-sensitive) appear in a single report,
generated by scan-build. Is there still a way to get the results in a single report
if the checks get moved to clang-tidy?

Would the implementation need to change to AST-Matcher based?
The point about the current implementation is that the AST-based checks
are also already backed by a range of tests defined in
tools/clang/test/Analysis/MPIChecker.cpp.

alexfh added a comment.Dec 9 2015, 4:18 AM

Ah ok, I wasn't aware that clang-tidy is not restricted to checks which verify stylistic issues.
What makes it more convenient to integrate the checks in clang-tidy? Is it how the AST-Matcher
functionality can be accessed?

Yes, less boilerplate code needed to work with AST matchers, powerful clang diagnostic infrastructure is used to report errors, also the add_new_check.py script ;)

I'm not an expert in the static analyzer code, so I'm not the one to review even AST-based checks there.

The AST-based checks basically use zero functionality from the static analyzer codebase
except the entry callback and the functionality to generate bug reports.

Well, that's one of the ways to say that I have little experience and interest in reviewing patches to the static analyzer ;)

There's nothing that prevents adding more error-detecting checks to clang-tidy.

Wrt. to the checks being part of this patch I have some concerns:

The first problem I see is that some functionlity is shared between the AST-based
and path-sensitive checks. But I think this should be restricted to the MPIFunctionClassifier class.

If this class stays in the static analyzer, the clang-tidy check code could probably just depend on the relevant library and use this class?

Another question I have is how this would affect the usability. At the moment
all detected bugs (AST-based and path-sensitive) appear in a single report,
generated by scan-build. Is there still a way to get the results in a single report
if the checks get moved to clang-tidy?

clang-tidy can run static analyzer checks. However, it only supports the plain text output format, which is sub-optimal for representing the results of the path-based checks. So the single report scenario is not easy to implement right now. Gábor Horváth proposed a solution for this a few months ago (PList output for clang-tidy), but a proper implementation of this seemed to require some changes in the clang's diagnostic subsystem.

Would the implementation need to change to AST-Matcher based?

That's not a 100% necessary step (you can always add a matcher for translationUnitDecl() and then traverse the AST using a RecursiveASTVisitor, for example), but this usually results in a more compact and easy to understand code.

The point about the current implementation is that the AST-based checks
are also already backed by a range of tests defined in
tools/clang/test/Analysis/MPIChecker.cpp.

Clang-tidy checks should also be backed by tests ;)

Yes, less boilerplate code needed to work with AST matchers, powerful clang diagnostic infrastructure is used to report errors, also the add_new_check.py script ;)

Pretty convincing advertisement line :).

If this class stays in the static analyzer, the clang-tidy check code could probably just depend on the relevant library and use this class?

Sounds straightforward.

That's not a 100% necessary step (you can always add a matcher for translationUnitDecl() and then traverse the AST using a RecursiveASTVisitor, for example), but this usually results in a more compact and easy to understand code.

I guess, in case I need to rewrite the code, I just wanted to know how the AST-Matchers work anyway. :)

Clang-tidy checks should also be backed by tests ;)

I kind of assumed that I need to rewrite the tests if the implementation gets changed.

I'll finish with the path-sensitive checks first and will submit the patch for the AST-based checks to clang-tidy after that.
Thanks for offering a review!

xazax.hun added inline comments.Jan 6 2016, 6:38 AM
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Utility.h
54 ↗(On Diff #38900)

A different implementation to achieve this functionality is proposed here: http://reviews.llvm.org/D15924
The proposed implementation is much lighter and has less functionality, it will not print the name of the struct and the index of the array (which might be unknown in most of the cases). Does that implementation works for you? Is that sufficient? Please review it.

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerPathSensitive.cpp
107 ↗(On Diff #38900)

Isn't this covered by the first case?
If there's an ErrorNode pass it to addTransition() else just update the state regarding removed requests.

  • removed AST related functionality from patch, as AST-based checks will be integrated into clang tidy
  • fix checkMissingWaits()
  • moved getVariableName() from patch (currently under review here: http://reviews.llvm.org/D16044)
  • moved is_contained() from patch (currently under review here: http://reviews.llvm.org/D16053)
  • removed files not being part of this patch anymore

Hi Alexander,

Sorry for the delay!

The patch should be rebased from the clang repo; for example, you could run "svn diff" from the clang directory. More comments inline.

tools/clang/lib/StaticAnalyzer/Checkers/Checkers.td
75 ↗(On Diff #45129)

This should probably go under the 'option' package. What do you think?

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.h
24 ↗(On Diff #45129)

Should this be clang::ento::mpi? Alternatively, you could place everything into a single file and have it live in anonymous namespace. This would also be more consistent with the existing checkers.

40 ↗(On Diff #45129)

Looks like some of the AST stuff was deleted and some was kept. Please, either remove all of it or keep all of it in.

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp
26 ↗(On Diff #45129)

nit: Please, remove the comments with "----" for consistency.

43 ↗(On Diff #45129)

I'd stress "path" instead of "sensitive" in the name. Also, this indirection is redundant if you remove the AST checks.

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerPathSensitive.cpp
43 ↗(On Diff #45129)

You should use 'generateErrorNode' or 'generateNonFatalErrorNode' to generate the node on which the error should be reported. If the error is non-fatal, you'd need to use the generated node below. Specifically, use it's state and pass it as the predecessor when adding a transition.

74 ↗(On Diff #45129)

This is called in a loop. Should you break once the first error is reported?

Also, as before you should use the CheckerContext API to produce a node on which the error should be reported.

79 ↗(On Diff #45129)

Don't forget to specify a predecessor here once the code above changes.

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerPathSensitive.h
87 ↗(On Diff #45129)

Please, use a name other than 'BugReporter' to avoid confusing it with the BugReporter in the analyzer.

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIFunctionClassifier.h
45 ↗(On Diff #45129)

Some of these classifier functions are not used either..

77 ↗(On Diff #45129)

MPIPointToCollTypes and MPICollToPointTypes do not seem to be used.

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPITypes.h
56 ↗(On Diff #45129)

Let's add some documentation on why you are not using the standard macro REGISTER_MAP_WITH_PROGRAMSTATE. (Might help to avoid confusion lear on.)

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Utility.cpp
21 ↗(On Diff #45129)

I'd like to remove the Utility.cpp completely by either making the helper functions static or moving them to other shared components.

40 ↗(On Diff #45129)

This is unused.

47 ↗(On Diff #45129)

This is not used..

tools/clang/test/Analysis/MPIChecker.cpp
112 ↗(On Diff #45129)

This is not testing the extra information provided by bug reporter visitor; you should use "// expected-note {...}"

Hi Anna,

thanks for having a look once more!

tools/clang/lib/StaticAnalyzer/Checkers/Checkers.td
75 ↗(On Diff #45129)

Do you mean the 'optin' package? I could not find an 'option' package in Checkers.td.

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.h
24 ↗(On Diff #45129)

I would prefer to put this into clang::ento::mpi, rather than having everything in a single file.

40 ↗(On Diff #45129)

Sorry about the inconsistent change. I will remove the AST related
functionality completety from this patch, as it will be part of the clang-tidy patch.

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp
43 ↗(On Diff #45129)

If sufficient, I would rename MPICheckerPathSensitive to MPICheckerPath then.
Do you mind the indirection?

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerPathSensitive.cpp
74 ↗(On Diff #45129)

I don't think break should be called after the first error is reported. Each memory region
represents a different request, why this should be rated as multiple errors.

79 ↗(On Diff #45129)

Will do.

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerPathSensitive.h
87 ↗(On Diff #45129)

I see, that could be confusing. Maybe renaming the variable to BReporter will do.

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIFunctionClassifier.h
45 ↗(On Diff #45129)

These model distinct MPI function classes. I agree that it would be better to remove the unused ones, in order to keep the interface as narrow as possible.

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPITypes.h
56 ↗(On Diff #45129)

Sure, I can do that.

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Utility.cpp
21 ↗(On Diff #45129)

So where shall we put sourceRange()? It is only used by the BugReporter class.
I could make it a member function of the Reporter class. Or would you prefer this
as a member function of MemRegion?

47 ↗(On Diff #45129)

These are also leftovers from the AST checks.

tools/clang/test/Analysis/MPIChecker.cpp
112 ↗(On Diff #45129)

I didn't know about expected-note, thanks!

zaks.anna added inline comments.Feb 3 2016, 2:58 PM
tools/clang/lib/StaticAnalyzer/Checkers/Checkers.td
75 ↗(On Diff #45129)

Yes, 'option'. I think it got spell checked.

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp
43 ↗(On Diff #45129)

I indirection buying us anything? I think it makes the code more difficult to follow,

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerPathSensitive.cpp
74 ↗(On Diff #45129)

You might be right, I am not 100% sure what's more user-friendly.. Presumably the fixes are different in each case? Could you add a test case where multiple errors on the same call site are reported? (Ups can use expected-warning@+1 to report multiple warnings on the same line.)

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Utility.cpp
21 ↗(On Diff #45129)

I don't see why we should't add it to MemRegion. Let's submit that as a separate patch. (If someone else has a better idea they'll tell us:))

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp
43 ↗(On Diff #45129)

I think, I like the entry point class being separated from the class which does the actual checking. But the necessity for dynamicInit() maybe outweighs that. I can collapse the two.

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerPathSensitive.cpp
74 ↗(On Diff #45129)

This is comparable to calling free on different elements, for which memory was never allocated. So each report informs about a distinct missing nonblocking call. I'll give expected-warning@+1 a try.

Alexander_Droste marked 36 inline comments as done.Feb 25 2016, 1:45 AM
Alexander_Droste added inline comments.
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerPathSensitive.cpp
79 ↗(On Diff #45129)

Changed this to the same ErrorNode/State pattern like in checkMissingWaits.

tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIFunctionClassifier.h
45 ↗(On Diff #45129)

I'd like to keep some of the classes/identifiers that are not used in this patch
but will be in the clang-tidy patch, as discussed with Alexander Kornienko.

tools/clang/test/Analysis/MPIChecker.cpp
112 ↗(On Diff #45129)

I changed the plain comments to // expected-note{{..}} but all tests using an expected note fail by claiming:

error: 'note' diagnostics expected but not seen:
File .../MPIChecker.cpp Line 112: Request is previously used by nonblocking call here.

Though, running the MPI-Checker on this function, the HTML report contains this exact note
at the expected position. The output is produced at the end of MPIBugReporter::RequestNodeVisitor::VisitNode.

Is there something missing, in order to make this an expected note?

Alexander_Droste marked 2 inline comments as done.
  • fixed checkUnmatchedWaits (added ErrorNode)
  • non fatal error node for double nonblocking
  • renamed BugReporter variable to BReporter
  • description why custom RequestMap is used
  • collapse pathsensitivechecker with mpichecker
  • mpi-checker move to optin package
  • moved sourceRange() memregion.cpp/h
  • remove Utility.cpp/h
  • put everything into clang::ento
  • remove '––––' from comments
  • test case checking for multiple missing nonblocking calls in the same line (// expected-warning-re 1+{{Request {{.*}} has..)
  • removed some of the mpi classifications/identifiers
lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.cpp
39

The MemRegion sourceRange() patch is not under review yet. I will submit the patch for after http://reviews.llvm.org/D16044 got accepted.

  • removed commented out line
  • removed dashes '–––' from comment in MPIChecker.cpp testfile
  • (diff is created with clang as pwd)
lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp
47

This should actually be reportDoubleNonblocking(..., ErrorNode); instead of ExplNode, right?

  • use MPI mock header in integration test file

Some comments inline. I still have to do an overall pass over this checker, but it looks much better than the first version!

lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp
48

Correct, it should be ErrorNode. Moreover, if you do not modify the State and use the state from the predecessor node, which is the case here, you do not need to pass a State to it. You do not need to pass the tag either in his case - a page generated for your checker will be used. By default the CheckerContext will use the state of the predecessor node and that default tag. (If you want to pass your tag, it's fine.)

You do need to pass the state if you modify the state as in the next example.

85

Here you do need to pass State.

136

I do not think I've seen this code before. If you add new functionality, please submit it in a new commit. Otherwise, there is a high chance it will go unnoticed.

Are there examples of the new warnings it caught?

checkEndAnalysis is rarely used by checkers, so you might not need it here. Specifically, you should be notified by the checkDeadSymbols before a symbol dies even if it dies because it's the end of path.

Alexander_Droste marked 3 inline comments as done.Mar 13 2016, 3:40 AM

I still have to do an overall pass over this checker, but it looks much better than the first version!

:D

lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp
48

Ah I see, this is covered by the default parameters.

136

Yes, I added this at some point after the initial patch, after I realized that the following test case fails:

MPI_Request sendReq1;
void missingWait2() { // Check missing wait for global variable.
  int rank = 0;
  double buf = 0;
  MPI_Comm_rank(MPI_COMM_WORLD, &rank);
  MPI_Ireduce(MPI_IN_PLACE, &buf, 1, MPI_DOUBLE, MPI_SUM, 0, MPI_COMM_WORLD, &sendReq1);
} // expected-warning{{Request 'sendReq1' has no matching wait.}}

if the checker solely relies on checkDeadSymbols. I just rechecked this behavior by commenting out

BReporter->reportMissingWait(Req.second, Req.first, *NodeIt);

in checkMissingWaitsGlobals and the test still fails then. Are global variables really covered by checkDeadSymbols?

Alexander_Droste marked an inline comment as done.
  • omit superfluous arguments passed to generateNonFatalErrorNode
test/Analysis/MPIChecker.cpp
97

All the expected-notes still fail, even though the diagnostics are presented in a report.

zaks.anna added inline comments.Mar 13 2016, 10:13 AM
lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp
136

The analyzer does not do a good job tracking global variables. You might get false positives, specifically, where the variable is released in another translation unit or by calling function that the analyzer does not inline.

lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp
136

So shall we remove or keep the function?

zaks.anna added inline comments.Mar 21 2016, 8:48 AM
lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp
136

I think you will get false positives of the nature I describe above if you keep it. If so, you should remove it. The user experience is much better if you avoid false positives.

  • remove checkMissingWaitsGlobals to prevent potential false positives
Alexander_Droste marked 6 inline comments as done.Mar 21 2016, 9:43 AM
Alexander_Droste added inline comments.
lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.cpp
39

sourceRange patch -> http://reviews.llvm.org/D18309

lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp
136

I removed the function.

zaks.anna added inline comments.Mar 28 2016, 8:09 PM
test/Analysis/MPIChecker.cpp
99

Do you see the notes in the report? I think you should pass "-analyzer-output=text" to see the notes.

zaks.anna added inline comments.Mar 28 2016, 11:09 PM
lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.cpp
96

Would it be possible to identify the "interesting node" by just looking at how the state you track changes. This function will be called when we walk the path upward from the error node. For example, you can notice that in the previous state, the Request did not exist and in this state it is Nonblocking. That would allow you to conclude that between those 2 states a function was called. (This is the trick we use in the other checkers (ex: malloc); it simplifies implementation and makes it more robust - we do not need to pattern match as much.)

lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp
87

Can you add a test cases for this report when there are multiple ReqRegions? Looks like all of the test cases with MPI_Waitall succeed. (I test a case where one of the regions has a match in Waitall and one does not as well as when several/all do not have a match.)

test/Analysis/MPIChecker.cpp
114

Are we allowed to call reduce -> wait -> reduce with the same req? If so, let's test for that.

Also, can calls to reduce and wait occur in different functions? If so, we need to test cases where they occur in different functions that are all in the same translation unit - all of the implementations are visible to the analyzer. And also test the cases where we pass the request to a function, whose body is not visible to the analyzer, which is the case when the function is defined in another translation unit.

Alexander_Droste marked 3 inline comments as done.Mar 30 2016, 3:02 AM
Alexander_Droste added inline comments.
lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.cpp
96

I think this should work but would slightly change the way diagnostics are presented in a specific case. If more than 2 nonblocking calls are using a request in sequence, the expected note (Request is previously used by nonblocking call here.) would not point to the previous but to the first nonblocking call of that sequence. What do you think; should the VisitNode function be changed regardless or should it stay as it is because of this specific case?

lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp
87

Sure, I'll add those tests. The case where none of the requests is matched should be covered by missingNonBlockingMultiple.

test/Analysis/MPIChecker.cpp
99

If I pass -analyzer-output=text I see the notes in the report. But adding that flag also produces a lot of 'undesired' output like: Loop condition is true. Entering loop body.

114

Are we allowed to call reduce -> wait -> reduce with the same req? If so, let's test for that.

Yes, that's allowed. I'll add a test.

Also, can calls to reduce and wait occur in different functions?

This is also possible.

And also test the cases where we pass the request to a function, whose body is not visible to the analyzer, which is the case when the function is defined in another translation unit.

I would then simply create a new pair of .cpp and .h files in the test folder where I define those functions so that the MPI-Checker tests can use them.

Alexander_Droste updated this object.
  • added test that uses wrapper functions around MPI functions
  • added test to check behavior in case MPI functions are used in other translation units
  • added more MPI_Waitall tests -> missingNonBlockingWaitall*
  • check memory regions for nullptr in checkDoubleNonblocking and checkUnmatchedWaits before they get passed to dyn_cast<ElementRegion>

Alexander,

This patch is in a pretty good shape. I am fine with committing it and iterating with smaller updates in tree if it is more convenient for you.

One task that I would like to very strongly encourage is running this on a lot of code. You will find a lot of issues this way that are hard to discover during code review. Both false positives and crashes.

Thanks!
Anna.

lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.cpp
97

The advantage of using the state is that it will be much more robust to any further changes to the compiler/checker because you will not be pattern matching the AST but instead will be checking the state, which the core reasoning is based on. One example that comes to mind is indirect calls. You will reduce the amount of code here as well, simplifying maintainability. This is the pattern we use in other checkers as well, so there is a remote chance we could introduce a new simplified API that will do the walk for the checker writers.

With respect to your example. Does it come up in practice? Wouldn't you warn on the second nonblocking request anyway? Could you add such an example to the tests? (Would be good in any case. If you leave the code as is, you can point to that example as the motivation.)

lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp
88

This is done, right?

test/Analysis/MPIChecker.cpp
100

This are explaining the path on which the problem occurs; the users will see them as well. There should not be a lot of those, you do not have a lot of conditions. Would it be reasonable to change the tests to incorporate those. Other alternative is to have another tests file that tests the notes in that mode.

What do you think?

115

I would then simply create a new pair of .cpp and .h files in the test folder
where I define those functions so that the MPI-Checker tests can use them.

You do not have to do that. You could just declare the functions and not define them. It will be equivalent to having the definitions in the other TUs.

alexfh removed a subscriber: alexfh.Apr 12 2016, 12:10 PM

Hi Anna,

I am fine with committing it and iterating with smaller updates in tree if it is more convenient for you.

This sounds good! The last thing I'll change before are the improvements you pointed out.

One task that I would like to very strongly encourage is running this on a lot of code.

Good idea. I'll do that.

Thanks a lot for all the time and effort you invested into the review!

lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp
88

Yep.

test/Analysis/MPIChecker.cpp
100

I'm fine with adding the notes to this test file.

115

Like you suggested, I'll simply declare the functions without definition.

test/Analysis/MPIChecker.cpp
100

I had a look once more how much output this actually produces. If -analyzer-output=text is passed, 42 notes are missing. I'll add another test file which will test for these notes.

lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.cpp
97

I'll change this to the pattern you suggested.

With respect to your example. Does it come up in practice?

It's for sure a little contrived.

Wouldn't you warn on the second nonblocking request anyway?

Yes.

Could you add such an example to the tests?

Sure.

  • added test file to test for note diagnostics
  • changed BugReportVisitor to detect request usage purely based on state and existence of a request
  • added test that showcases a triple nonblocking usage of a request
Alexander_Droste marked 20 inline comments as done.Apr 16 2016, 4:12 AM
zaks.anna accepted this revision.Apr 16 2016, 11:37 AM
zaks.anna edited edge metadata.

Awesome!

Thank you for investing SO MUCH time into improving the checker and addressing the review comments.
Do you have commit access?

Nit: Please, use lower case letters for test names to be consistent with the rest.

This revision is now accepted and ready to land.Apr 16 2016, 11:37 AM
Alexander_Droste edited edge metadata.
  • lower case letters for test filenames

Yeah; I'm excited to see that this code will now be part of LLVM. Thanks to everybody reviewing the patch!
I don't have commit access. Can you commit the bundle of related patches for me?

Anna, will you commit this, or do you want me to commit the patches?

This revision was automatically updated to reflect the committed changes.

This doesn't compile under gcc so it broke the bots. The fix is to move the specialization of clang::ento::ProgramStateTrait for RequestMapImpl out of the global namespace and into clang::ento. I will apply and recommit.

Fixed the compilation issues with gcc in r271977 r271981, but it is still failing with Address Sanitizer diagnostics:

18751==ERROR: AddressSanitizer: stack-use-after-return on address 0x7f0c695ebc70 at pc 0x00000867b44c bp 0x7ffe3b01d6f0 sp 0x7ffe3b01d6e8

READ of size 8 at 0x7f0c695ebc70 thread T0

#0 0x867b44b in getSourceManager /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:448:46
#1 0x867b44b in clang::ento::BugReporter::emitReport(std::__1::unique_ptr<clang::ento::BugReport, std::__1::default_delete<clang::ento::BugReport> >) /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/lib/StaticAnalyzer/Core/BugReporter.cpp:3240
#2 0x8636355 in clang::ento::mpi::MPIBugReporter::reportUnmatchedWait(clang::ento::CallEvent const&, clang::ento::MemRegion const*, clang::ento::ExplodedNode const*) const /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.cpp:82:3
#3 0x843e1ee in clang::ento::mpi::MPIChecker::checkUnmatchedWaits(clang::ento::CallEvent const&, clang::ento::CheckerContext&) const /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp:88:7
#4 0x8447dc8 in checkPreCall /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.h:36:5
#5 0x8447dc8 in void clang::ento::check::PreCall::_checkCall<clang::ento::mpi::MPIChecker>(void*, clang::ento::CallEvent const&, clang::ento::CheckerContext&) /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/include/clang/StaticAnalyzer/Core/Checker.h:168
#6 0x86db2f2 in operator() /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h:59:12
#7 0x86db2f2 in runChecker /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp:269
#8 0x86db2f2 in expandGraphWithCheckers<(anonymous namespace)::CheckCallContext> /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp:123
#9 0x86db2f2 in clang::ento::CheckerManager::runCheckersForCallEvent(bool, clang::ento::ExplodedNodeSet&, clang::ento::ExplodedNodeSet const&, clang::ento::CallEvent const&, clang::ento::ExprEngine&, bool) /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp:285
#10 0x8782a84 in runCheckersForPreCall /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/include/clang/StaticAnalyzer/Core/CheckerManager.h:252:5
#11 0x8782a84 in clang::ento::ExprEngine::evalCall(clang::ento::ExplodedNodeSet&, clang::ento::ExplodedNode*, clang::ento::CallEvent const&) /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:509
#12 0x878205b in clang::ento::ExprEngine::VisitCallExpr(clang::CallExpr const*, clang::ento::ExplodedNode*, clang::ento::ExplodedNodeSet&) /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:487:5
#13 0x872a0d8 in clang::ento::ExprEngine::Visit(clang::Stmt const*, clang::ento::ExplodedNode*, clang::ento::ExplodedNodeSet&) /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp:1104:7
#14 0x87210bf in clang::ento::ExprEngine::ProcessStmt(clang::CFGStmt, clang::ento::ExplodedNode*) /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp:448:5
#15 0x87207ae in clang::ento::ExprEngine::processCFGElement(clang::CFGElement, clang::ento::ExplodedNode*, unsigned int, clang::ento::NodeBuilderContext*) /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp:297:7
#16 0x86fc7a6 in clang::ento::CoreEngine::HandlePostStmt(clang::CFGBlock const*, unsigned int, clang::ento::ExplodedNode*) /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp:515:5
#17 0x86fafed in clang::ento::CoreEngine::dispatchWorkItem(clang::ento::ExplodedNode*, clang::ProgramPoint, clang::ento::WorkListUnit const&) /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp:274:7
#18 0x86f9891 in clang::ento::CoreEngine::ExecuteWorkList(clang::LocationContext const*, unsigned int, llvm::IntrusiveRefCntPtr<clang::ento::ProgramState const>) /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/lib/StaticAnalyzer/Core/CoreEngine.cpp:231:5
#19 0x67f1ab5 in ExecuteWorkList /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h:109:12
#20 0x67f1ab5 in (anonymous namespace)::AnalysisConsumer::ActionExprEngine(clang::Decl*, bool, clang::ento::ExprEngine::InliningModes, llvm::DenseSet<clang::Decl const*, llvm::DenseMapInfo<clang::Decl const*> >*) /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:664
#21 0x67f0ae2 in RunPathSensitiveChecks /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:694:5
#22 0x67f0ae2 in (anonymous namespace)::AnalysisConsumer::HandleCode(clang::Decl*, unsigned int, clang::ento::ExprEngine::InliningModes, llvm::DenseSet<clang::Decl const*, llvm::DenseMapInfo<clang::Decl const*> >*) /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:632
#23 0x67dc135 in HandleDeclsCallGraph /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:495:5
#24 0x67dc135 in (anonymous namespace)::AnalysisConsumer::HandleTranslationUnit(clang::ASTContext&) /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:547
#25 0x687dae4 in clang::ParseAST(clang::Sema&, bool, bool) /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/lib/Parse/ParseAST.cpp:167:3
#26 0x51c090b in clang::FrontendAction::Execute() /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/lib/Frontend/FrontendAction.cpp:457:8
#27 0x510f614 in clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/lib/Frontend/CompilerInstance.cpp:876:7
#28 0x537fa92 in clang::ExecuteCompilerInvocation(clang::CompilerInstance*) /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:241:18
#29 0x8fc2f2 in cc1_main(llvm::ArrayRef<char const*>, char const*, void*) /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/tools/driver/cc1_main.cpp:116:13
#30 0x8f6a88 in ExecuteCC1Tool /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/tools/driver/driver.cpp:301:12
#31 0x8f6a88 in main /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/tools/driver/driver.cpp:381
#32 0x7f0c6c208f44 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21f44)
#33 0x816952 in _start (/mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm_build_asan/bin/clang-3.9+0x816952)

Address 0x7f0c695ebc70 is located in stack of thread T0 at offset 1136 in frame

  #0 0x67f130f in (anonymous namespace)::AnalysisConsumer::ActionExprEngine(clang::Decl*, bool, clang::ento::ExprEngine::InliningModes, llvm::DenseSet<clang::Decl const*, llvm::DenseMapInfo<clang::Decl const*> >*) /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:644

This frame has 5 object(s):
  [32, 184) 'P.i'
  [256, 260) 'FD.i'
  [272, 296) 'ref.tmp.i'
  [336, 344) 'agg.tmp.i'
  [368, 1248) 'Eng' <== Memory access at offset 1136 is inside this variable

HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext

(longjmp and C++ exceptions *are* supported)

SUMMARY: AddressSanitizer: stack-use-after-return /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:448:46 in getSourceManager
Shadow bytes around the buggy address:

0x0fe20d2b5730: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
0x0fe20d2b5740: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
0x0fe20d2b5750: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
0x0fe20d2b5760: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
0x0fe20d2b5770: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5

>0x0fe20d2b5780: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5[f5]f5

0x0fe20d2b5790: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
0x0fe20d2b57a0: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
0x0fe20d2b57b0: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
0x0fe20d2b57c0: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
0x0fe20d2b57d0: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5

Shadow byte legend (one shadow byte represents 8 application bytes):

Addressable:           00
Partially addressable: 01 02 03 04 05 06 07 
Heap left redzone:       fa
Heap right redzone:      fb
Freed heap region:       fd
Stack left redzone:      f1
Stack mid redzone:       f2
Stack right redzone:     f3
Stack partial redzone:   f4
Stack after return:      f5
Stack use after scope:   f8
Global redzone:          f9
Global init order:       f6
Poisoned by user:        f7
Container overflow:      fc
Array cookie:            ac
Intra object redzone:    bb
ASan internal:           fe
Left alloca redzone:     ca
Right alloca redzone:    cb

18751==ABORTING


Testing: 0
FAIL: Clang :: Analysis/mpicheckernotes.cpp (317 of 9433)