Page MenuHomePhabricator

MPITypeMismatchCheck for Clang-Tidy
ClosedPublic

Authored by Alexander_Droste on Jul 3 2016, 1:55 PM.

Details

Summary

This check verifies if buffer type and MPI (Message Passing Interface)
datatype pairs match. All MPI datatypes defined by the MPI standard (3.1)
are verified by this check. User defined typedefs, custom MPI datatypes and
null pointer constants are skipped, in the course of verification.

Instructions on how to apply the check can be found at: https://github.com/0ax1/MPI-Checker/tree/master/examples

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
alexfh added inline comments.Jul 7 2016, 6:04 AM
clang-tidy/misc/MpiTypeMismatchCheck.cpp
147

What about this comment?

152

What about this comment? I find it convenient to mark comments "Done" when I address them, so it's easier to track the action items.

259

Diagnostics are not complete sentences, so they should not start with a capital letter and should not end with a period.

259

Please use formatting placeholders instead of concatenation: diag("buffer type '%0' does not match ...") << BufferTypeName << ....;.

271

It looks like you're trying to avoid unnecessary allocations by passing this buffer into the calls below. It would probably be even more efficient, if you moved this variable out of the loop.

clang-tidy/misc/MpiTypeMismatchCheck.h
128

These static members and corresponding methods don't have to be in the class. My comment might have been ambiguous, but I was actually suggesting to change the methods that use these maps to free-standing static functions in the .cpp file and make these maps static local variables in the corresponding functions.

This revision now requires changes to proceed.Jul 7 2016, 6:04 AM
Alexander_Droste edited edge metadata.
  • make static functions free functions
  • make static containers local to their corresponding functions
  • only assign the buffer type name in case of an error
  • fix capitalization and punctuation in error diagnostic
  • usage of formatting placeholders instead of concatenation
  • move buffer type name variable out of the loop
Alexander_Droste marked 22 inline comments as done.Jul 7 2016, 10:47 AM
Alexander_Droste added inline comments.
clang-tidy/misc/MpiTypeMismatchCheck.cpp
154

Sure, I can do that. I marked all addressed comments with done.

273

I removed the unnecessary single word comments.

273

Further, the variable now only gets assigned in case of an error.

clang-tidy/misc/MpiTypeMismatchCheck.h
61

I see, this removes a lot of verbosity.

Alexander_Droste marked 4 inline comments as done.Jul 7 2016, 2:28 PM
Alexander_Droste edited edge metadata.
  • fix typo
  • simplify argumentType() function

Thanks for addressing the comments. The code looks better now. My main concern at this point is that the name and the location of the check: "misc" is a place for the stuff we can't find a better place for, but here I guess we clearly want a separate module and a subdirectory for MPI-related checks (MPITidyModule, "mpi/"). The check name should be updated accordingly: "mpi-type-mismatch".

A few more comments inline.

clang-tidy/misc/MpiTypeMismatchCheck.cpp
213

llvm::StringMap should be more efficient here.

219

getQualifiedNameAsString returns a std::string, which will be destroyed at the end of this statement. TypedefToCompare will be a dangling reference then. Please change StringRef to std::string here.

Another question is whether you need getQualifiedNameAsString, which is rather expensive. Maybe you could use getName (it returns a StringRef and is relatively cheap) and additionally check that the name is in the global namespace?

221

Repeated lookups into a map are wasteful. Please store the result of find and use it to get the value.

alexfh requested changes to this revision.Jul 19 2016, 5:23 AM
alexfh edited edge metadata.
This revision now requires changes to proceed.Jul 19 2016, 5:23 AM
Alexander_Droste marked 2 inline comments as done.Jul 19 2016, 10:54 AM

Thanks for looking over this once more. I'll set up an extra MPI folder and rename the check.

One comment inline.

clang-tidy/misc/MpiTypeMismatchCheck.cpp
219

No, it seems I can also simply use getName. Why is it necessary to check if the name is in the global namespace? How would that check look like?

alexfh added inline comments.Jul 19 2016, 6:07 PM
clang-tidy/misc/MpiTypeMismatchCheck.cpp
219

Verifying that the name is in the global namespace would make the check stricter, but there might not be a realistic case, where it would prevent a false positive. We could try without it.

Alexander_Droste edited edge metadata.
  • use llvm::StringMap instead of std::map
  • getQualifiedNameAsString -> getName
  • remove redundant map lookup
  • create MPIi module
  • replace misc with mpi within the check

I created an MPI module based on how misc is set up. Somehow the
mpi-type-mismatch is not found when the tests are run or when I try to
apply the check on a project but I am assuming you see at first glance what
ingredient missing.

Alexander_Droste marked 5 inline comments as done.Jul 21 2016, 4:10 PM
alexfh requested changes to this revision.Jul 21 2016, 5:37 PM
alexfh edited edge metadata.
alexfh added inline comments.
clang-tidy/mpi/MPITidyModule.cpp
30 ↗(On Diff #64985)

nit: s/lint/clang-tidy/ ;)

clang-tidy/mpi/TypeMismatchCheck.cpp
11 ↗(On Diff #64985)

Since the header is used outside of its library, it should be moved to clang/include/StaticAnalyzer/....

50 ↗(On Diff #64985)

nit: Please remove the empty line.

185 ↗(On Diff #64985)

I think, getName can be used here as well.

256 ↗(On Diff #64985)

Explicit captures would make the code more transparent.

256 ↗(On Diff #64985)

It's not clear what this lambda does. Please add a comment.

clang-tidy/mpi/TypeMismatchCheck.h
52 ↗(On Diff #64985)

With just one caller and two lines of implementation this method doesn't make the code any better. Please just inline it.

This revision now requires changes to proceed.Jul 21 2016, 5:37 PM
Alexander_Droste marked 7 inline comments as done.Jul 22 2016, 4:21 AM
Alexander_Droste edited edge metadata.
  • use new MPIFunctionClassifier.h path
  • change getAsCXXRecordDecl()->getNameAsString() -> getAsCXXRecordDecl()->getName()
  • add comment to addPair lambda
  • rename ArgumentExpression to BufferExprs
  • remove superfluous empty lines

Still, the type mismatch is somehow not found/listed by clang-tidy.
When invoking $LLVM_TRUNK/build/.../clang-tidy -checks='*' -list-checks | ag mpi
it does not show up. Something seems to be missing, in order to make the mpi module available.

Still, the type mismatch is somehow not found/listed by clang-tidy.
When invoking $LLVM_TRUNK/build/.../clang-tidy -checks='*' -list-checks | ag mpi
it does not show up. Something seems to be missing, in order to make the mpi module available.

You need to add this to ClangTidyMain.cpp:

// This anchor is used to force the linker to link the MPIModule.
extern volatile int MPIModuleAnchorSource;
static int LLVM_ATTRIBUTE_UNUSED MPIModuleAnchorDestination =
    MPIModuleAnchorSource;

A few more nits. Please ensure the tests pass once you get the mpi module linking to the clang-tidy binary.

docs/clang-tidy/checks/mpi-type-mismatch.rst
4 ↗(On Diff #65052)

Remove extra =s.

12 ↗(On Diff #65052)

Please make this a code-block (see other .rst files for an example).

12 ↗(On Diff #65052)

Comments should be valid English prose including correct capitalization and punctuation (http://llvm.org/docs/CodingStandards.html#commenting). In code snippets inside documentation as well.

test/clang-tidy/Inputs/mpi-type-mismatch/mpimock.h
8 ↗(On Diff #65052)

Use proper capitalization and punctuation, please.

test/clang-tidy/mpi-type-mismatch.cpp
12 ↗(On Diff #65052)

Please leave the first CHECK-MESSAGES as is, but remove the common suffix [mpi-type-mismatch] from all other messages. This makes the test a bit easier to read.

alexfh requested changes to this revision.Jul 22 2016, 6:18 AM
alexfh edited edge metadata.
This revision now requires changes to proceed.Jul 22 2016, 6:18 AM
Alexander_Droste marked 5 inline comments as done.Jul 22 2016, 7:07 AM

You need to add this to ClangTidyMain.cpp:

Thanks for pointing this out and reviewing the code once more!

Alexander_Droste edited edge metadata.

This update addresses all requested changes. All integration tests are still passing.

Alexander_Droste edited edge metadata.
  • fix typo
alexfh accepted this revision.Jul 23 2016, 12:04 AM
alexfh edited edge metadata.

Looks good with one comment.

clang-tidy/mpi/TypeMismatchCheck.cpp
237 ↗(On Diff #65074)
This revision is now accepted and ready to land.Jul 23 2016, 12:04 AM
Alexander_Droste edited edge metadata.
Alexander_Droste marked an inline comment as done.
  • use parentheses instead of braces

Do you have commit access?

Do you have commit access?

No, I don't have commit access.

alexfh added a project: Restricted Project.Jul 25 2016, 8:37 AM
This revision was automatically updated to reflect the committed changes.
Alexander_Droste removed rL LLVM as the repository for this revision.

Hi, thanks for the notification! Obviously, on some systems char is unsigned by default
which is why the check now tolerates distinct signedness for char pairs.
http://blog.cdleary.com/2012/11/arm-chars-are-unsigned-by-default/
In addition, I removed the trailing white space from the test file.

Alexander_Droste reopened this revision.Aug 1 2016, 5:20 AM
This revision is now accepted and ready to land.Aug 1 2016, 5:20 AM
alexfh added a comment.Aug 2 2016, 1:21 PM

Running tests, will submit shortly.

docs/clang-tidy/checks/mpi-type-mismatch.rst
13 ↗(On Diff #65404)

This doesn't parse - an extra newline is needed. I'll fix this myself.

This revision was automatically updated to reflect the committed changes.

Thanks for re-committing this patch!