This is an archive of the discontinued LLVM Phabricator instance.

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

Alexander_Droste retitled this revision from to MPITypeMismatchCheck for Clang-Tidy.
Alexander_Droste updated this object.
Alexander_Droste added a reviewer: alexfh.
Alexander_Droste added a subscriber: cfe-commits.

Hi Alexander,

this is the check that verifies buffer type / MPI datatype inconsistencies
that was originally part of http://reviews.llvm.org/D12761.

docs/clang-tidy/checks/list.rst
63

Will be removed.

test/clang-tidy/misc-mpi-type-mismatch.cpp
4

This header needs to moved but I need some guidance where to put it so that the test file can access it. Using relative paths within the include seems not to be possible.

  • change comment style to LLVM style
  • remove 'misc-m-p-i-type-mismatch' entry from list.rst (misc-mpi-type-mismatch is the correct one)
  • fix some comments
  • fix comments
  • remove superflous clang::
alexfh edited edge metadata.Jul 4 2016, 5:14 AM

Please re-generate the diff with full-context (http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface).

A bunch of style comments.

clang-tidy/misc/MpiTypeMismatchCheck.cpp
87

I don't think this needs to be a method of the check class. I'd say a static bool isMPIDataType(StringRef Type) is a proper interface, and it can have the list of types as a static local variable.

123

Since this method doesn't need any members of the class, it should be a static function instead.

131

Just return true;

139

Looks like you can use getText from clang/include/clang/Tooling/FixIt.h.

147

Since this method doesn't need any members of the class, it should be a static function instead.

152
223

Should probably be a static function as well. ComplexCXXMatches can be a static local variable instead.

Same applies for the other similar methods.

234

Please use actual language options (e.g. from MatchResult).

263

Use ArrayRef<const Type *> instead.

269
272

Please use proper capitalization and punctuation in comments.

http://llvm.org/docs/CodingStandards.html#commenting

"When writing comments, write them as English prose, which means they should use proper capitalization, punctuation, etc."

Actually, I'm not sure these comments are helpful.

273

Use const auto * when the type of the variable is obvious (e.g. when the initializer literally spells it, like here).

277

else should be on the same line as the preceding }.

test/clang-tidy/misc-mpi-type-mismatch.cpp
5

We usually put headers needed for tests to a subdiretory (named after the test name) of the Inputs directory. See test/clang-tidy/modernize-loop-convert-extra.cpp, for example.

alexfh requested changes to this revision.Jul 4 2016, 5:14 AM
alexfh edited edge metadata.
This revision now requires changes to proceed.Jul 4 2016, 5:14 AM
Alexander_Droste edited edge metadata.

Thanks for the review!

This update should address the requested changes:

  • functions and variables are made static if possible
  • usage of ArrayRef<const Type *> instead of SmallVector<..> &
  • assignment init instead of braced init
  • usage of tooling::fixit::getText instead of custom function
  • no else after return
  • remove superfluous single word comments
  • const auto * if the deduced type is obvious
  • add MPI mock header to test/clang-tidy/Inputs/misc-mpi-type-mismatch/mpimock.h
  • pass LangOptions from the AST matcher result
  • return bool in while loop directly instead of assigning true and calling break
  • generate the diff with full context (git diff -U999999 master)
Alexander_Droste edited edge metadata.
  • remove TODO
alexfh requested changes to this revision.Jul 7 2016, 6:04 AM
alexfh edited edge metadata.
alexfh added inline comments.Jul 7 2016, 6:04 AM
clang-tidy/misc/MpiTypeMismatchCheck.cpp
148

What about this comment?

153

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.

260

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

260

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

272

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
129

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!