Page MenuHomePhabricator

MPIBufferDerefCheck for Clang-Tidy
ClosedPublic

Authored by Alexander_Droste on Jul 23 2016, 7:02 AM.

Details

Summary

This check verifies if a buffer passed to an MPI (Message Passing Interface)
function is sufficiently dereferenced. Buffers should be passed as a single
pointer or array. As MPI function signatures specify void * for their buffer
types, insufficiently dereferenced buffers can be passed, like for example
as double pointers or multidimensional arrays, without a compiler warning
emitted.

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

Diff Detail

Repository
rL LLVM

Event Timeline

Alexander_Droste retitled this revision from to MPIBufferDerefCheck for Clang-Tidy.
Alexander_Droste updated this object.
Alexander_Droste added a reviewer: alexfh.
Alexander_Droste added a subscriber: cfe-commits.
  • make = count accurate for mpi-buffer-deref.rst
  • fix two MPIdatatype tags
alexfh edited reviewers, added: hokein; removed: alexfh.Jul 25 2016, 8:56 AM
  • remove trailing whitespace
hokein edited edge metadata.Aug 1 2016, 1:38 AM

Looks like the dependency of this patch D21962 got reverted by some reasons.

clang-tidy/mpi/BufferDerefCheck.cpp
63 ↗(On Diff #65359)

Could you add some comments to describe what these statements do? It's confused when reading the magic number parameter. The same below.

109 ↗(On Diff #65359)

size_t

Hi, thanks for having a look!

clang-tidy/mpi/BufferDerefCheck.cpp
63 ↗(On Diff #65359)

Sure, I'll extend the comment.

109 ↗(On Diff #65359)

Hmm, this loop iterates the indirections in reverse order and exits the loop if i gets smaller than 0. size_t would therefore not work. I think it's safe to assume that the number of indirections will not exceed INT_MAX. How about an explicit cast? I can't use rbegin and rend, as the current index is needed for:

if (i > 0) {
  IndirectionDesc += "->";
}

Looks like the dependency of this patch D21962 got reverted by some reasons.

I know, I updated the patch after that, in order to fix the problem.

Alexander_Droste marked 3 inline comments as done.Aug 3 2016, 11:10 PM
Alexander_Droste edited edge metadata.
  • clarify lambda argument with comment
  • explicit cast to int for the number of indirections in for loop
  • add needed extra line for docs
hokein added inline comments.Aug 5 2016, 3:55 AM
clang-tidy/mpi/BufferDerefCheck.cpp
87 ↗(On Diff #66761)

check whether BufferType is nullptr

110 ↗(On Diff #66761)

I think you can simplify the code as below:

for (auto Iter = Indirections.rbegin(); Iter != Indirections.rend(); ++Iter) {
   if (!IndirectionDesc.empty()) IndirectionDesc += "->";
   if (Indirections[i] == IndirectionType::Pointer) {
      IndirectionDesc += "pointer";
   } else {
      IndirectionDesc += "array";
   }
}
docs/clang-tidy/checks/list.rst
113 ↗(On Diff #66761)

Please also mention the check in docs/ReleaseNotes.rst. The same to mpi-type-mismatch.

Alexander_Droste marked 4 inline comments as done.Aug 5 2016, 6:27 AM
Alexander_Droste added inline comments.
clang-tidy/mpi/BufferDerefCheck.cpp
87 ↗(On Diff #66761)

Null pointer types are not added to the BufferType vector in addBuffer.

if (CE->getArg(BufferIdx)->isNullPointerConstant(
            *Result.Context, Expr::NPC_ValueDependentIsNull) ||
110 ↗(On Diff #66761)

Good idea, that looks better.

docs/clang-tidy/checks/list.rst
113 ↗(On Diff #66761)

Done.

Alexander_Droste marked 3 inline comments as done.
  • add release docs
  • simplify reverse for loop -> rbegin, rend
Alexander_Droste marked an inline comment as done.Aug 5 2016, 9:31 AM
Alexander_Droste added inline comments.
clang-tidy/mpi/BufferDerefCheck.cpp
88 ↗(On Diff #66940)

I just realized, you meant sth. different. I'll update the patch to check for nullptr in addBuffer.

Alexander_Droste marked an inline comment as done.
  • check for nullptr in addBuffer
hokein accepted this revision.Aug 8 2016, 8:33 AM
hokein edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Aug 8 2016, 8:33 AM

Great; thanks again for the review!

Do you have commit access now?

No but I guess it would be a good idea to ask for commit access.
I'll proceed like suggested here: http://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access.

This revision was automatically updated to reflect the committed changes.