dcoughlin added a comment.
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 0x0867b44c bp 0x7ffe3b01d6f0 sp 0x7ffe3b01d6e8
dcoughlin added a comment.
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.
Repository:
rL LLVM
http://reviews.llvm.org
This revision was automatically updated to reflect the committed changes.
Closed by commit rL271907: [analyzer] Add checker for correct usage of MPI API
in C and C++. (authored by dcoughlin).
Changed prior to commit:
http://reviews.llvm.org/D12761?vs=54001&id=59742#toc
Repository:
rL LLVM
h
xazax.hun added a comment.
Anna, will you commit this, or do you want me to commit the patches?
http://reviews.llvm.org/D12761
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Alexander_Droste updated this revision to Diff 54001.
Alexander_Droste added a comment.
- 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 rela
zaks.anna accepted this revision.
zaks.anna added a comment.
This revision is now accepted and ready to land.
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 name
Alexander_Droste marked 20 inline comments as done.
Alexander_Droste added a comment.
http://reviews.llvm.org/D12761
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Alexander_Droste updated this revision to Diff 53982.
Alexander_Droste added a comment.
- 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 re
Alexander_Droste added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.cpp:96
@@ +95,3 @@
+if (const CallExpr *CE = clang::dyn_cast(SP->getStmt())) {
+
+ auto FuncIdentifier = CE->getDirectCallee()->getIdentifier();
zak
Alexander_Droste added inline comments.
Comment at: test/Analysis/MPIChecker.cpp:99
@@ +98,3 @@
+ MPI_Wait(&req, MPI_STATUS_IGNORE);
+}
+
Alexander_Droste wrote:
> zaks.anna wrote:
> > This are explaining the path on which the problem occurs; the users will
> >
Alexander_Droste added a comment.
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 enc
zaks.anna added a comment.
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 i
Alexander_Droste updated this revision to Diff 52148.
Alexander_Droste added a comment.
- check memory regions for `nullptr` in `checkDoubleNonblocking` and
`checkUnmatchedWaits` before they get passed to `dyn_cast`
http://reviews.llvm.org/D12761
Files:
lib/StaticAnalyzer/Checkers/CMakeLists
Alexander_Droste updated the summary for this revision.
Alexander_Droste updated this revision to Diff 52056.
Alexander_Droste added a comment.
- added test that uses wrapper functions around MPI functions
- added test to check behavior in case MPI functions are used in other
translation units
-
Alexander_Droste updated this revision to Diff 52059.
Alexander_Droste added a comment.
- fix typo
http://reviews.llvm.org/D12761
Files:
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.cpp
lib/Stat
Alexander_Droste marked 3 inline comments as done.
Comment at: lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.cpp:95
@@ +94,3 @@
+ if (Optional SP = N->getLocation().getAs()) {
+if (const CallExpr *CE = clang::dyn_cast(SP->getStmt())) {
+
zaks.anna wr
zaks.anna added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.cpp:95
@@ +94,3 @@
+ if (Optional SP = N->getLocation().getAs()) {
+if (const CallExpr *CE = clang::dyn_cast(SP->getStmt())) {
+
Would it be possible to identi
zaks.anna added inline comments.
Comment at: test/Analysis/MPIChecker.cpp:98
@@ +97,3 @@
+
+ MPI_Request req;
+ MPI_Ireduce(MPI_IN_PLACE, &buf, 1, MPI_DOUBLE, MPI_SUM, 0, MPI_COMM_WORLD,
&req); // expected-note{{Request is previously used by nonblocking call here.}}
---
Alexander_Droste marked 6 inline comments as done.
Comment at: lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.cpp:39
@@ +38,3 @@
+
+ if (Range.isValid())
+Report->addRange(Range);
`sourceRange` patch -> http://reviews.llvm.org/D18309
Alexander_Droste updated this revision to Diff 51179.
Alexander_Droste added a comment.
- remove `checkMissingWaitsGlobals` to prevent potential false positives
http://reviews.llvm.org/D12761
Files:
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/Checkers.td
lib/Sta
zaks.anna added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp:136
@@ +135,3 @@
+ auto NodeIt = G.eop_begin();
+ const auto NodeEndIt = G.eop_end();
+
Alexander_Droste wrote:
> zaks.anna wrote:
> > The analyzer does not do a
Alexander_Droste added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp:136
@@ +135,3 @@
+ auto NodeIt = G.eop_begin();
+ const auto NodeEndIt = G.eop_end();
+
zaks.anna wrote:
> The analyzer does not do a good job tracking glo
zaks.anna added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp:136
@@ +135,3 @@
+ auto NodeIt = G.eop_begin();
+ const auto NodeEndIt = G.eop_end();
+
The analyzer does not do a good job tracking global variables. You might g
Alexander_Droste added inline comments.
Comment at: test/Analysis/MPIChecker.cpp:97
@@ +96,3 @@
+
+MPI_Isend(&buf, 1, MPI_DOUBLE, rank + 1, 6, MPI_COMM_WORLD, &sendReq1); //
expected-note{{Request is previously used by nonblocking call here. }}
+MPI_Irecv(&buf, 1, MPI_DOU
Alexander_Droste updated this revision to Diff 50548.
Alexander_Droste marked an inline comment as done.
Alexander_Droste added a comment.
- omit superfluous arguments passed to `generateNonFatalErrorNode`
http://reviews.llvm.org/D12761
Files:
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/
Alexander_Droste marked 3 inline comments as done.
Alexander_Droste added a comment.
> I still have to do an overall pass over this checker, but it looks much
> better than the first version!
:D
Comment at: lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp:47
@@ +46,3 @@
zaks.anna added a comment.
Some comments inline. I still have to do an overall pass over this checker, but
it looks much better than the first version!
Comment at: lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp:47
@@ +46,3 @@
+BReporter->reportDoubleNonblocking(PreC
Alexander_Droste updated this revision to Diff 50526.
Alexander_Droste added a comment.
- use MPI mock header in integration test file
http://reviews.llvm.org/D12761
Files:
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/MPI-C
Alexander_Droste added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp:46
@@ +45,3 @@
+ExplodedNode *ErrorNode = Ctx.generateNonFatalErrorNode(State, &Tag);
+BReporter->reportDoubleNonblocking(PreCallEvent, *Req, MR, ExplNode);
+Ctx.
Alexander_Droste updated this revision to Diff 49028.
Alexander_Droste added a comment.
- removed commented out line
- removed dashes '–––' from comment in `MPIChecker.cpp` testfile
- (diff is created with `clang` as pwd)
http://reviews.llvm.org/D12761
Files:
lib/StaticAnalyzer/Checkers/CMake
Alexander_Droste added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.cpp:38
@@ +37,3 @@
+ Report->addRange(MPICallEvent.getSourceRange());
+ SourceRange Range = RequestRegion->sourceRange();
+ // util::sourceRange(RequestRegion);
--
Alexander_Droste updated this revision to Diff 49023.
Alexander_Droste marked 2 inline comments as done.
Alexander_Droste added a comment.
- fixed checkUnmatchedWaits (added ErrorNode)
- non fatal error node for double nonblocking
- renamed BugReporter variable to BReporter
- description why custo
Alexander_Droste marked 36 inline comments as done.
Comment at:
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerPathSensitive.cpp:79
@@ +78,3 @@
+ if (!ReqRegions.empty()) {
+Ctx.addTransition(State);
+ }
Alexander_Droste wrote:
> zaks.anna wr
Alexander_Droste added inline comments.
Comment at:
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp:43
@@ +42,3 @@
+private:
+ const std::unique_ptr CheckerSens;
+
zaks.anna wrote:
> Alexander_Droste wrote:
> > zaks.anna wrote:
> > > I'd stres
zaks.anna added inline comments.
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/Checkers.td:75
@@ -74,1 +74,3 @@
+def MPI : Package<"mpi">;
+
Alexander_Droste wrote:
> zaks.anna wrote:
> > This should probably go under the 'option' package. What do you thin
Alexander_Droste added a comment.
Hi Anna,
thanks for having a look once more!
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/Checkers.td:75
@@ -74,1 +74,3 @@
+def MPI : Package<"mpi">;
+
zaks.anna wrote:
> This should probably go under the 'option' pack
zaks.anna added a comment.
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.
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/Checkers.td:75
@@ -74,1 +74
Alexander_Droste updated this revision to Diff 45129.
Alexander_Droste added a comment.
- removed files not being part of this patch anymore
http://reviews.llvm.org/D12761
Files:
tools/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
tools/clang/lib/StaticAnalyzer/Checkers/Checkers.td
too
Alexander_Droste updated this revision to Diff 45126.
Alexander_Droste added a comment.
- 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://r
Alexander_Droste added inline comments.
Comment at:
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerPathSensitive.cpp:107
@@ +106,3 @@
+ State = State->remove(Req.first);
+}
+ }
zaks.anna wrote:
> Alexander_Droste wrote:
> > I need `addTra
xazax.hun added inline comments.
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Utility.h:54
@@ +53,3 @@
+/// \returns variable name for memory region
+std::string variableName(const clang::ento::MemRegion *MR);
+
zaks.anna wrote:
> Since this tak
Alexander_Droste added a comment.
> 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, th
alexfh added a comment.
In http://reviews.llvm.org/D12761#304823, @Alexander_Droste wrote:
> 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
>
Alexander_Droste added a comment.
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 ana
alexfh added a comment.
In http://reviews.llvm.org/D12761#304651, @Alexander_Droste wrote:
> @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-sensitiv
Alexander_Droste added a comment.
@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
alexfh added a comment.
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.
http://reviews.llvm.org/D12761
___
zaks.anna added a comment.
This patch contains a mix of checks, some are path sensitive and some are AST
checks. See MPICheckerAST.cpp for the latter.
http://reviews.llvm.org/D12761
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lis
alexfh added a comment.
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
zaks.anna added a comment.
I've been mostly looking at the path sensitive checks. Maybe clang-tidy team
would be interested in reviewing the syntactic checks.
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Container.h:31
@@ +30,2 @@
+
+#endif
A
Alexander_Droste added a comment.
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 necess
Alexander_Droste marked 26 inline comments as done.
Alexander_Droste added a comment.
http://reviews.llvm.org/D12761
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Alexander_Droste added inline comments.
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPITypes.h:67
@@ +66,3 @@
+ // Every time a request is 'set' a new 'RequestId' gets created.
+ // Therefore, the 'UserKind' does not need to be profiled.
+ const int RequestI
Alexander_Droste added a comment.
Sorry about the delay.
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPITypes.h:67
@@ +66,3 @@
+ // Every time a request is 'set' a new 'RequestId' gets created.
+ // Therefore, the 'UserKind' does not need to be profiled.
+
zaks.anna added inline comments.
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPITypes.h:67
@@ +66,3 @@
+ // Every time a request is 'set' a new 'RequestId' gets created.
+ // Therefore, the 'UserKind' does not need to be profiled.
+ const int RequestId{Reque
Alexander_Droste added a 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.
I have watched it some months ago but I'm sure it is a good idea to revisit the
zaks.anna added a 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
Comment at:
Alexander_Droste added inline comments.
Comment at:
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerPathSensitive.cpp:35
@@ +34,3 @@
+ if (!isa(MR) ||
+ (ER && !isa(ER->getSuperRegion(
+return;
Thanks for the hint! I tried `iterBinding
dcoughlin added inline comments.
Comment at:
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPICheckerPathSensitive.cpp:34
@@ +33,3 @@
+ if (MR->getBaseRegion()->getAs())
+return;
+
You may be able to use `StoreManager::iterBindings()`, which iterates o
Alexander_Droste added a comment.
In http://reviews.llvm.org/D12761#250296, @zaks.anna wrote:
> > 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 c
zaks.anna added a comment.
> 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 fun
Alexander_Droste added inline comments.
Comment at:
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.cpp:137
@@ +136,3 @@
+
+ // This is never reached...
+ llvm::outs() << ""
Alexander_Droste wrote:
> I figured out what the pro
Alexander_Droste marked 2 inline comments as done.
Comment at:
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.cpp:137
@@ +136,3 @@
+
+ // This is never reached...
+ llvm::outs() << ""
I figured out what the problem about the
Alexander_Droste marked 2 inline comments as done.
Alexander_Droste added a comment.
In http://reviews.llvm.org/D12761#248382, @zaks.anna wrote:
> > > 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.
>
zaks.anna added a comment.
> > 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 th
Alexander_Droste marked 9 inline comments as done.
Alexander_Droste added a comment.
Thanks for the review!
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/Checkers.td:524
@@ +523,3 @@
+def MPIChecker : Checker<"MPI-Checker">,
+ HelpText<"Checks MPI code written in C">,
+
zaks.anna requested changes to this revision.
zaks.anna added a comment.
This revision now requires changes to proceed.
Thanks for working on this!
Comments inline.
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/Checkers.td:524
@@ +523,3 @@
+def MPIChecker : Checker<"MPI-C
Alexander_Droste added inline comments.
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Utility.cpp:71
@@ +70,3 @@
+ if (FR) {
+VariableName = VR->getDecl()->getNameAsString() + "." +
+ FR->getDecl()->getNameAsString();
This
Alexander_Droste marked 3 inline comments as done.
Alexander_Droste added a comment.
http://reviews.llvm.org/D12761
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Alexander_Droste updated this revision to Diff 34664.
Alexander_Droste added a comment.
- corrected some typos
http://reviews.llvm.org/D12761
Files:
tools/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
tools/clang/lib/StaticAnalyzer/Checkers/Checkers.td
tools/clang/lib/StaticAnalyzer/Ch
Alexander_Droste updated this revision to Diff 34647.
Alexander_Droste marked 2 inline comments as done.
Alexander_Droste added a comment.
- fixed integration test file: tools/clang/test/Analysis/MPIChecker.c
- mocked types, constants, functions from mpi.h
- mocked types from stdint.h
- dropped -I
Alexander_Droste updated this revision to Diff 34633.
Alexander_Droste added a comment.
- changed header file extension to .h
- fixed variable naming
- adapted llvm include guard style
- fixed function comments
- included parameter names in function declarations
- fixed namespacing (mpi is now ins
Alexander_Droste marked 14 inline comments as done.
Comment at:
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Container.hpp:10-11
@@ +9,4 @@
+///
+/// \file
+/// This file defines convenience templates for C++ container class usage.
+///
Alexander_Droste wr
Alexander_Droste updated this revision to Diff 34640.
Alexander_Droste added a comment.
- fixed include guard for MPIBugReporter.h
- capitalized two variable names
http://reviews.llvm.org/D12761
Files:
tools/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
tools/clang/lib/StaticAnalyzer/Che
Alexander_Droste added inline comments.
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/Checkers.td:524
@@ +523,3 @@
+def MPIChecker : Checker<"MPI-Checker">,
+ HelpText<"Checks MPI code written in C">,
+ DescFile<"MPIChecker.cpp">;
gribozavr wrote:
> Does i
Alexander_Droste added inline comments.
Comment at:
tools/clang/lib/StaticAnalyzer/Checkers/MPI-Checker/Container.hpp:10-11
@@ +9,4 @@
+///
+/// \file
+/// This file defines convenience templates for C++ container class usage.
+///
gribozavr wrote:
> This file re
gribozavr added inline comments.
Comment at: tools/clang/lib/StaticAnalyzer/Checkers/Checkers.td:524
@@ +523,3 @@
+def MPIChecker : Checker<"MPI-Checker">,
+ HelpText<"Checks MPI code written in C">,
+ DescFile<"MPIChecker.cpp">;
Does it only works with C code,
77 matches
Mail list logo
