Page MenuHomePhabricator

[clang-tidy] new check: bugprone-posix-return
ClosedPublic

Authored by jcai19 on Jun 20 2019, 1:53 PM.

Details

Summary

Checks if any calls to posix functions (except posix_openpt) expect negative return values.
These functions return either 0 on success or an errno on failure, which is positive only.

Diff Detail

Repository
rL LLVM

Event Timeline

jcai19 created this revision.Jun 20 2019, 1:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 20 2019, 1:53 PM
lebedev.ri retitled this revision from Add a check on expected return values of posix functions (except posix_openpt) in Android module. to [clang-tidy] Add a check on expected return values of posix functions (except posix_openpt) in Android module..Jun 20 2019, 1:58 PM
lebedev.ri added reviewers: JonasToth, gribozavr.

Why is this in android module?
Is that android-specific behavior, or posix?

just a few drive-by nits/comments from me. as usual, not super familiar with clang-tidy, so i won't be able to stamp this.

thanks!

clang-tools-extra/clang-tidy/android/PosixReturnCheck.cpp
23 ↗(On Diff #205897)

should we also try to catch <= ${negative_value} (or < ${negative_value})?

29 ↗(On Diff #205897)

similarly, should we complain about != ${negative_value}?

36 ↗(On Diff #205897)

nit: please clang-format

39 ↗(On Diff #205897)

would it be helpful to add fixits for simple cases? e.g. we can probably offer to replace posix_whatever() < 0 with posix_whatever() > 0 or != 0

clang-tools-extra/test/clang-tidy/android-posix-return.cpp
57 ↗(On Diff #205897)

nit: no space in Macro (), please

Why is this in android module?
Is that android-specific behavior, or posix?

I implemented it for Andriod as requested, but it would be completely fine to move it if it is better to place the check at a different location. Where do you suggest we should move this check to? Thanks.

Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/android/PosixReturnCheck.h
1 ↗(On Diff #205897)

Please use - before *- C++ -* to fill line. See other checks as example.

clang-tools-extra/docs/ReleaseNotes.rst
199 ↗(On Diff #205897)

Please move into new checks list (in alphabetical order).

202 ↗(On Diff #205897)

calls? posix -> POSIX. Please enclose posix_openpt in double back-ticks.

clang-tools-extra/docs/clang-tidy/checks/android-posix-return.rst
4 ↗(On Diff #205897)

Please make same length as name.

6 ↗(On Diff #205897)

Please make same changes as in Release Notes.

7 ↗(On Diff #205897)

Please enclose 0 and errno into double back-ticks.

16 ↗(On Diff #205897)

Please add colon at end.

jcai19 updated this revision to Diff 206273.Jun 24 2019, 11:36 AM

Fix typos and formatting issues.

Why is this in android module?
Is that android-specific behavior, or posix?

I implemented it for Andriod as requested, but it would be completely fine to move it if it is better to place the check at a different location. Where do you suggest we should move this check to? Thanks.

Either misc or bugprone or a new posix module.

jcai19 marked 10 inline comments as done.Jun 24 2019, 11:49 AM
jcai19 added inline comments.
clang-tools-extra/clang-tidy/android/PosixReturnCheck.cpp
23 ↗(On Diff #205897)

Sounds good. Will update the patch to catch these two cases as well.

29 ↗(On Diff #205897)

Sounds good. Will update the patch to catch these additional cases as well.

39 ↗(On Diff #205897)

While this fix is handy, I am not sure whether it will be safe enough under all circumstances. For example, is it possible in the code block following the check, the program calls another POSIX function and alter the errno before its value it checked? In that case, maybe the proper fix should be something as follows and fixing it by changing the binary operator may obscure it:

int ret = posix_whatever();
if (ret != 0)

jcai19 added a comment.EditedJun 24 2019, 1:42 PM

Why is this in android module?
Is that android-specific behavior, or posix?

I implemented it for Andriod as requested, but it would be completely fine to move it if it is better to place the check at a different location. Where do you suggest we should move this check to? Thanks.

Either misc or bugprone or a new posix module.

Sounds good. Thanks for the clarification. The check has been moved to 'bugprone' module.

gribozavr accepted this revision.Jun 25 2019, 6:46 AM
gribozavr added inline comments.
clang-tools-extra/docs/clang-tidy/checks/android-posix-return.rst
21 ↗(On Diff #206273)

why not if (posix_fadvise() != 0) ?

Otherwise it looks like adding a variable is necessary for a fix.

clang-tools-extra/test/clang-tidy/android-posix-return.cpp
8 ↗(On Diff #206273)

typedef decltype(sizeof(char)) size_t;

67 ↗(On Diff #206273)

What about

if (posix_fadvise() >= 0) { ... } else { ... }

?

This revision is now accepted and ready to land.Jun 25 2019, 6:46 AM
jcai19 updated this revision to Diff 206586.Jun 25 2019, 11:27 PM
jcai19 marked an inline comment as done.

Move the check from android to bugprone.

jcai19 updated this revision to Diff 206588.Jun 25 2019, 11:45 PM
jcai19 marked 4 inline comments as done.

Update a test case.

jcai19 marked an inline comment as done.Jun 25 2019, 11:45 PM
jcai19 added inline comments.
clang-tools-extra/clang-tidy/android/PosixReturnCheck.cpp
39 ↗(On Diff #205897)

After some offline discussion, I agree fixing the simple cases should be fine as programers should verify if the fixes proposed by clang-tidy are correct.

jcai19 marked 2 inline comments as done.Jun 25 2019, 11:47 PM
jcai19 retitled this revision from [clang-tidy] Add a check on expected return values of posix functions (except posix_openpt) in Android module. to [clang-tidy] new check: bugprone-posix-return.Jun 25 2019, 11:49 PM
gribozavr added inline comments.Jun 26 2019, 2:48 AM
clang-tools-extra/clang-tidy/bugprone/PosixReturnCheck.cpp
49 ↗(On Diff #206588)

Please add tests for fix-its.

clang-tools-extra/test/clang-tidy/bugprone-posix-return.cpp
96 ↗(On Diff #206588)

Shouldn't there be a warning in the else branch?

jcai19 marked 2 inline comments as done.Jun 26 2019, 11:22 AM
jcai19 added inline comments.
clang-tools-extra/clang-tidy/bugprone/PosixReturnCheck.cpp
49 ↗(On Diff #206588)

The fix-it is triggered when cases like posix_*(...) < 0 are detected (except posix_openpt) , which are currently covered in the unit test. The test runtime will make sure fix-its are applied.

clang-tools-extra/test/clang-tidy/bugprone-posix-return.cpp
96 ↗(On Diff #206588)

This check only matches calls to the POSIX functions in the global scope, not member functions or functions defined within namespaces. Although in the global scope, this particular case will still pass as there won't be a `<` binary operator for the else branch so no matching will happen.

gribozavr added inline comments.Jun 26 2019, 11:56 AM
clang-tools-extra/clang-tidy/bugprone/PosixReturnCheck.cpp
49 ↗(On Diff #206588)

Could you add CHECK-FIXES lines to validate the fixit directly?

clang-tools-extra/test/clang-tidy/bugprone-posix-return.cpp
96 ↗(On Diff #206588)

Sorry, yes, that's what I meant -- if we warn on < 0, then we should warn on the else branch of >=. I just commented on the wrong test -- sorry about that.

jcai19 updated this revision to Diff 206946.Jun 27 2019, 2:59 PM

Update the check to catch redundant code and update the warning message to be more speicific.

jcai19 marked 2 inline comments as done.Jun 27 2019, 3:04 PM
jcai19 added inline comments.
clang-tools-extra/test/clang-tidy/bugprone-posix-return.cpp
96 ↗(On Diff #206588)

Thanks for the clarification. Actually >= 0 is always true since the call return 0 on success and a positive value on error, so the else branch will never be reached. I have update my check and the warning message to reflect this. Good catch!

jcai19 updated this revision to Diff 206947.Jun 27 2019, 3:07 PM

Update test names.

gribozavr added inline comments.Jun 28 2019, 6:29 AM
clang-tools-extra/clang-tidy/bugprone/PosixReturnCheck.cpp
61 ↗(On Diff #206947)

80 columns, please.

jcai19 updated this revision to Diff 207102.Jun 28 2019, 10:53 AM

Update warning messages.

jcai19 marked 2 inline comments as done.Jun 28 2019, 10:54 AM
gribozavr accepted this revision.Jul 1 2019, 5:53 AM

Good improvements!

clang-tools-extra/clang-tidy/bugprone/PosixReturnCheck.cpp
21 ↗(On Diff #207102)

Please clang-format (the whole file).

56 ↗(On Diff #207102)

"non-negative"

"the comparison always evaluates to false because %0 always returns non-negative values"

62 ↗(On Diff #207102)

I'd suggest "the comparison always evaluates to true because %0 always returns a non-negative value"

jcai19 updated this revision to Diff 207397.Jul 1 2019, 1:32 PM

clang-format

jcai19 updated this revision to Diff 207398.Jul 1 2019, 1:35 PM
jcai19 marked 2 inline comments as done.

Update warning messages.

jcai19 marked an inline comment as done.Jul 1 2019, 1:37 PM
gribozavr accepted this revision.Jul 1 2019, 1:43 PM

Thanks! Do you have commit access? Do you want me to commit this patch for you?

jcai19 updated this revision to Diff 207401.Jul 1 2019, 1:44 PM

Update CHECK-MESSAGES messages accordingly.

jcai19 added a comment.Jul 2 2019, 1:45 PM

Thanks! Do you have commit access? Do you want me to commit this patch for you?

Totally missed your message yesterday. Sorry about that. I do not have the access so it will be great if you can commit the change. Thanks a lot.

Committed revision 365007.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 3 2019, 2:20 AM