[PATCH] D33304: [clang-tidy][Part1] Add a new module Android and three new checks.
alexfh accepted this revision. alexfh added a comment. This revision is now accepted and ready to land. LG with one nit. Comment at: clang-tidy/android/FileOpenFlagCheck.cpp:24 + +bool HasCloseOnExecFlag(const Expr *Flags, const SourceManager , +const LangOptions ) { nit: "Function names should be verb phrases (as they represent actions), and command-like function should be imperative. The name should be camel case, and start with a lower case letter (e.g. openFile() or isFoo())." (http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly) https://reviews.llvm.org/D33304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33304: [clang-tidy][Part1] Add a new module Android and three new checks.
yawanng updated this revision to Diff 103068. yawanng marked 3 inline comments as done. https://reviews.llvm.org/D33304 Files: clang-tidy/CMakeLists.txt clang-tidy/android/AndroidTidyModule.cpp clang-tidy/android/CMakeLists.txt clang-tidy/android/FileOpenFlagCheck.cpp clang-tidy/android/FileOpenFlagCheck.h clang-tidy/plugin/CMakeLists.txt clang-tidy/tool/CMakeLists.txt clang-tidy/tool/ClangTidyMain.cpp docs/ReleaseNotes.rst docs/clang-tidy/checks/android-file-open-flag.rst docs/clang-tidy/checks/list.rst docs/clang-tidy/index.rst test/clang-tidy/android-file-open-flag.cpp unittests/clang-tidy/CMakeLists.txt Index: unittests/clang-tidy/CMakeLists.txt === --- unittests/clang-tidy/CMakeLists.txt +++ unittests/clang-tidy/CMakeLists.txt @@ -25,6 +25,7 @@ clangFrontend clangLex clangTidy + clangTidyAndroidModule clangTidyGoogleModule clangTidyLLVMModule clangTidyMiscModule Index: test/clang-tidy/android-file-open-flag.cpp === --- /dev/null +++ test/clang-tidy/android-file-open-flag.cpp @@ -0,0 +1,110 @@ +// RUN: %check_clang_tidy %s android-file-open-flag %t + +#define O_RDWR 1 +#define O_EXCL 2 +#define __O_CLOEXEC 3 +#define O_CLOEXEC __O_CLOEXEC + +extern "C" int open(const char *fn, int flags, ...); +extern "C" int open64(const char *fn, int flags, ...); +extern "C" int openat(int dirfd, const char *pathname, int flags, ...); + +void a() { + open("filename", O_RDWR); + // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: 'open' should use O_CLOEXEC where possible [android-file-open-flag] + // CHECK-FIXES: O_RDWR | O_CLOEXEC + open("filename", O_RDWR | O_EXCL); + // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: 'open' should use O_CLOEXEC where + // CHECK-FIXES: O_RDWR | O_EXCL | O_CLOEXEC +} + +void b() { + open64("filename", O_RDWR); + // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: 'open64' should use O_CLOEXEC where possible [android-file-open-flag] + // CHECK-FIXES: O_RDWR | O_CLOEXEC + open64("filename", O_RDWR | O_EXCL); + // CHECK-MESSAGES: :[[@LINE-1]]:37: warning: 'open64' should use O_CLOEXEC where + // CHECK-FIXES: O_RDWR | O_EXCL | O_CLOEXEC +} + +void c() { + openat(0, "filename", O_RDWR); + // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: 'openat' should use O_CLOEXEC where possible [android-file-open-flag] + // CHECK-FIXES: O_RDWR | O_CLOEXEC + openat(0, "filename", O_RDWR | O_EXCL); + // CHECK-MESSAGES: :[[@LINE-1]]:40: warning: 'openat' should use O_CLOEXEC where + // CHECK-FIXES: O_RDWR | O_EXCL | O_CLOEXEC +} + +void f() { + open("filename", 3); + // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 'open' should use O_CLOEXEC where possible [android-file-open-flag] + // CHECK-FIXES: 3 | O_CLOEXEC + open64("filename", 3); + // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: 'open64' should use O_CLOEXEC where possible [android-file-open-flag] + // CHECK-FIXES: 3 | O_CLOEXEC + openat(0, "filename", 3); + // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: 'openat' should use O_CLOEXEC where possible [android-file-open-flag] + // CHECK-FIXES: 3 | O_CLOEXEC + + int flag = 3; + open("filename", flag); + // CHECK-MESSAGES-NOT: warning: + open64("filename", flag); + // CHECK-MESSAGES-NOT: warning: + openat(0, "filename", flag); + // CHECK-MESSAGES-NOT: warning: +} + +namespace i { +int open(const char *pathname, int flags, ...); +int open64(const char *pathname, int flags, ...); +int openat(int dirfd, const char *pathname, int flags, ...); + +void d() { + open("filename", O_RDWR); + // CHECK-MESSAGES-NOT: warning: + open64("filename", O_RDWR); + // CHECK-MESSAGES-NOT: warning: + openat(0, "filename", O_RDWR); + // CHECK-MESSAGES-NOT: warning: +} + +} // namespace i + +void e() { + open("filename", O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: + open("filename", O_RDWR | O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: + open("filename", O_RDWR | O_CLOEXEC | O_EXCL); + // CHECK-MESSAGES-NOT: warning: + open64("filename", O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: + open64("filename", O_RDWR | O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: + open64("filename", O_RDWR | O_CLOEXEC | O_EXCL); + // CHECK-MESSAGES-NOT: warning: + openat(0, "filename", O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: + openat(0, "filename", O_RDWR | O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: + openat(0, "filename", O_RDWR | O_CLOEXEC | O_EXCL); + // CHECK-MESSAGES-NOT: warning: +} + +class G { +public: + int open(const char *pathname, int flags, ...); + int open64(const char *pathname, int flags, ...); + int openat(int dirfd, const char *pathname, int flags, ...); + + void h() { +open("filename", O_RDWR); +// CHECK-MESSAGES-NOT: warning: +open64("filename", O_RDWR); +// CHECK-MESSAGES-NOT: warning: +openat(0, "filename", O_RDWR); +// CHECK-MESSAGES-NOT: warning: + } +}; Index:
[PATCH] D33304: [clang-tidy][Part1] Add a new module Android and three new checks.
hokein accepted this revision. hokein added a comment. The code looks good to me now, I'd wait to see whether @alexfh has further comments before submitting the patch. Thanks for your patience with the review ;) Comment at: clang-tidy/android/FileOpenFlagCheck.cpp:61 + SourceLocation EndLoc = + Lexer::getLocForEndOfToken(FlagArg->getLocEnd(), 0, SM, getLangOpts()); + yawanng wrote: > hokein wrote: > > Instead of using getLangOpts(), you should use > > `Result.Context.getLangOpts()`. > May I ask what's the difference between the `Result.Context.getLangOpts()` > and `getLangOpts()`? Does `Result.Context.getLangOpts()` have a longer > lifetime than `getLangOpts()`? IMO they are the same fundamentally, except `Result.Context.getLangOpts()` can save a copy. We usually use `Result.Context.getLangOpts()` inside `ClangTidyCheck::check(const MatchFinder::MatchResult )`. Comment at: clang-tidy/android/FileOpenFlagCheck.cpp:79 +std::pairExpansionInfo = SM.getDecomposedLoc(Loc); +auto MacroName = +SM.getBufferData(ExpansionInfo.first) yawanng wrote: > hokein wrote: > > You could use `Lexer::getImmediateMacroName(Loc, SM, Opts);` to get the > > marco name, or doesn't it work here? > `getImmediateMacroName` sometimes cannot return the `O_CLOEXEC `, like > > #define O_CLOEXEC _O_CLOEXEC > #define _O_CLOEXEC 1 > > `getImmediateMacroName` will return `_O_CLOEXEC`, whereas `O_CLOEXEC` is what > we need. I change this to directly get the source text if it's a macro, > which seems to be simpler. Ah, I see, thanks for the clarification! Comment at: clang-tidy/android/FileOpenFlagCheck.cpp:22 +namespace { +bool checkFlags(const Expr *Flags, const SourceManager , +const LangOptions ) { `checkFlags` name is a bit of ambiguous, maybe rename it to "HasCloseOnExecFlag"? Notice that you use the string "O_CLOEXEC" several times in the code, I'd suggest to use a "CloseOnExecFlag" variable. Comment at: clang-tidy/android/FileOpenFlagCheck.cpp:33 + +return (MacroName == "O_CLOEXEC"); + } Nit: The outermost () is not needed. https://reviews.llvm.org/D33304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33304: [clang-tidy][Part1] Add a new module Android and three new checks.
yawanng updated this revision to Diff 102690. yawanng added a comment. Format change. https://reviews.llvm.org/D33304 Files: clang-tidy/CMakeLists.txt clang-tidy/android/AndroidTidyModule.cpp clang-tidy/android/CMakeLists.txt clang-tidy/android/FileOpenFlagCheck.cpp clang-tidy/android/FileOpenFlagCheck.h clang-tidy/plugin/CMakeLists.txt clang-tidy/tool/CMakeLists.txt clang-tidy/tool/ClangTidyMain.cpp docs/ReleaseNotes.rst docs/clang-tidy/checks/android-file-open-flag.rst docs/clang-tidy/checks/list.rst docs/clang-tidy/index.rst test/clang-tidy/android-file-open-flag.cpp unittests/clang-tidy/CMakeLists.txt Index: unittests/clang-tidy/CMakeLists.txt === --- unittests/clang-tidy/CMakeLists.txt +++ unittests/clang-tidy/CMakeLists.txt @@ -25,6 +25,7 @@ clangFrontend clangLex clangTidy + clangTidyAndroidModule clangTidyGoogleModule clangTidyLLVMModule clangTidyMiscModule Index: test/clang-tidy/android-file-open-flag.cpp === --- /dev/null +++ test/clang-tidy/android-file-open-flag.cpp @@ -0,0 +1,110 @@ +// RUN: %check_clang_tidy %s android-file-open-flag %t + +#define O_RDWR 1 +#define O_EXCL 2 +#define __O_CLOEXEC 3 +#define O_CLOEXEC __O_CLOEXEC + +extern "C" int open(const char *fn, int flags, ...); +extern "C" int open64(const char *fn, int flags, ...); +extern "C" int openat(int dirfd, const char *pathname, int flags, ...); + +void a() { + open("filename", O_RDWR); + // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: 'open' should use O_CLOEXEC where possible [android-file-open-flag] + // CHECK-FIXES: O_RDWR | O_CLOEXEC + open("filename", O_RDWR | O_EXCL); + // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: 'open' should use O_CLOEXEC where + // CHECK-FIXES: O_RDWR | O_EXCL | O_CLOEXEC +} + +void b() { + open64("filename", O_RDWR); + // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: 'open64' should use O_CLOEXEC where possible [android-file-open-flag] + // CHECK-FIXES: O_RDWR | O_CLOEXEC + open64("filename", O_RDWR | O_EXCL); + // CHECK-MESSAGES: :[[@LINE-1]]:37: warning: 'open64' should use O_CLOEXEC where + // CHECK-FIXES: O_RDWR | O_EXCL | O_CLOEXEC +} + +void c() { + openat(0, "filename", O_RDWR); + // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: 'openat' should use O_CLOEXEC where possible [android-file-open-flag] + // CHECK-FIXES: O_RDWR | O_CLOEXEC + openat(0, "filename", O_RDWR | O_EXCL); + // CHECK-MESSAGES: :[[@LINE-1]]:40: warning: 'openat' should use O_CLOEXEC where + // CHECK-FIXES: O_RDWR | O_EXCL | O_CLOEXEC +} + +void f() { + open("filename", 3); + // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 'open' should use O_CLOEXEC where possible [android-file-open-flag] + // CHECK-FIXES: 3 | O_CLOEXEC + open64("filename", 3); + // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: 'open64' should use O_CLOEXEC where possible [android-file-open-flag] + // CHECK-FIXES: 3 | O_CLOEXEC + openat(0, "filename", 3); + // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: 'openat' should use O_CLOEXEC where possible [android-file-open-flag] + // CHECK-FIXES: 3 | O_CLOEXEC + + int flag = 3; + open("filename", flag); + // CHECK-MESSAGES-NOT: warning: + open64("filename", flag); + // CHECK-MESSAGES-NOT: warning: + openat(0, "filename", flag); + // CHECK-MESSAGES-NOT: warning: +} + +namespace i { +int open(const char *pathname, int flags, ...); +int open64(const char *pathname, int flags, ...); +int openat(int dirfd, const char *pathname, int flags, ...); + +void d() { + open("filename", O_RDWR); + // CHECK-MESSAGES-NOT: warning: + open64("filename", O_RDWR); + // CHECK-MESSAGES-NOT: warning: + openat(0, "filename", O_RDWR); + // CHECK-MESSAGES-NOT: warning: +} + +} // namespace i + +void e() { + open("filename", O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: + open("filename", O_RDWR | O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: + open("filename", O_RDWR | O_CLOEXEC | O_EXCL); + // CHECK-MESSAGES-NOT: warning: + open64("filename", O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: + open64("filename", O_RDWR | O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: + open64("filename", O_RDWR | O_CLOEXEC | O_EXCL); + // CHECK-MESSAGES-NOT: warning: + openat(0, "filename", O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: + openat(0, "filename", O_RDWR | O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: + openat(0, "filename", O_RDWR | O_CLOEXEC | O_EXCL); + // CHECK-MESSAGES-NOT: warning: +} + +class G { +public: + int open(const char *pathname, int flags, ...); + int open64(const char *pathname, int flags, ...); + int openat(int dirfd, const char *pathname, int flags, ...); + + void h() { +open("filename", O_RDWR); +// CHECK-MESSAGES-NOT: warning: +open64("filename", O_RDWR); +// CHECK-MESSAGES-NOT: warning: +openat(0, "filename", O_RDWR); +// CHECK-MESSAGES-NOT: warning: + } +}; Index:
[PATCH] D33304: [clang-tidy][Part1] Add a new module Android and three new checks.
yawanng updated this revision to Diff 102577. yawanng marked 7 inline comments as done. https://reviews.llvm.org/D33304 Files: clang-tidy/CMakeLists.txt clang-tidy/android/AndroidTidyModule.cpp clang-tidy/android/CMakeLists.txt clang-tidy/android/FileOpenFlagCheck.cpp clang-tidy/android/FileOpenFlagCheck.h clang-tidy/plugin/CMakeLists.txt clang-tidy/tool/CMakeLists.txt clang-tidy/tool/ClangTidyMain.cpp clang-tidy/tool/run-clang-tidy.py docs/ReleaseNotes.rst docs/clang-tidy/checks/android-file-open-flag.rst docs/clang-tidy/checks/list.rst docs/clang-tidy/index.rst test/clang-tidy/android-file-open-flag.cpp unittests/clang-tidy/CMakeLists.txt Index: unittests/clang-tidy/CMakeLists.txt === --- unittests/clang-tidy/CMakeLists.txt +++ unittests/clang-tidy/CMakeLists.txt @@ -25,6 +25,7 @@ clangFrontend clangLex clangTidy + clangTidyAndroidModule clangTidyGoogleModule clangTidyLLVMModule clangTidyMiscModule Index: test/clang-tidy/android-file-open-flag.cpp === --- /dev/null +++ test/clang-tidy/android-file-open-flag.cpp @@ -0,0 +1,110 @@ +// RUN: %check_clang_tidy %s android-file-open-flag %t + +#define O_RDWR 1 +#define O_EXCL 2 +#define __O_CLOEXEC 3 +#define O_CLOEXEC __O_CLOEXEC + +extern "C" int open(const char *fn, int flags, ...); +extern "C" int open64(const char *fn, int flags, ...); +extern "C" int openat(int dirfd, const char *pathname, int flags, ...); + +void a() { + open("filename", O_RDWR); + // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: 'open' should use O_CLOEXEC where possible [android-file-open-flag] + // CHECK-FIXES: O_RDWR | O_CLOEXEC + open("filename", O_RDWR | O_EXCL); + // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: 'open' should use O_CLOEXEC where + // CHECK-FIXES: O_RDWR | O_EXCL | O_CLOEXEC +} + +void b() { + open64("filename", O_RDWR); + // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: 'open64' should use O_CLOEXEC where possible [android-file-open-flag] + // CHECK-FIXES: O_RDWR | O_CLOEXEC + open64("filename", O_RDWR | O_EXCL); + // CHECK-MESSAGES: :[[@LINE-1]]:37: warning: 'open64' should use O_CLOEXEC where + // CHECK-FIXES: O_RDWR | O_EXCL | O_CLOEXEC +} + +void c() { + openat(0, "filename", O_RDWR); + // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: 'openat' should use O_CLOEXEC where possible [android-file-open-flag] + // CHECK-FIXES: O_RDWR | O_CLOEXEC + openat(0, "filename", O_RDWR | O_EXCL); + // CHECK-MESSAGES: :[[@LINE-1]]:40: warning: 'openat' should use O_CLOEXEC where + // CHECK-FIXES: O_RDWR | O_EXCL | O_CLOEXEC +} + +void f() { + open("filename", 3); + // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 'open' should use O_CLOEXEC where possible [android-file-open-flag] + // CHECK-FIXES: 3 | O_CLOEXEC + open64("filename", 3); + // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: 'open64' should use O_CLOEXEC where possible [android-file-open-flag] + // CHECK-FIXES: 3 | O_CLOEXEC + openat(0, "filename", 3); + // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: 'openat' should use O_CLOEXEC where possible [android-file-open-flag] + // CHECK-FIXES: 3 | O_CLOEXEC + + int flag = 3; + open("filename", flag); + // CHECK-MESSAGES-NOT: warning: + open64("filename", flag); + // CHECK-MESSAGES-NOT: warning: + openat(0, "filename", flag); + // CHECK-MESSAGES-NOT: warning: +} + +namespace i { +int open(const char *pathname, int flags, ...); +int open64(const char *pathname, int flags, ...); +int openat(int dirfd, const char *pathname, int flags, ...); + +void d() { + open("filename", O_RDWR); + // CHECK-MESSAGES-NOT: warning: + open64("filename", O_RDWR); + // CHECK-MESSAGES-NOT: warning: + openat(0, "filename", O_RDWR); + // CHECK-MESSAGES-NOT: warning: +} + +} // namespace i + +void e() { + open("filename", O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: + open("filename", O_RDWR | O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: + open("filename", O_RDWR | O_CLOEXEC | O_EXCL); + // CHECK-MESSAGES-NOT: warning: + open64("filename", O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: + open64("filename", O_RDWR | O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: + open64("filename", O_RDWR | O_CLOEXEC | O_EXCL); + // CHECK-MESSAGES-NOT: warning: + openat(0, "filename", O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: + openat(0, "filename", O_RDWR | O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: + openat(0, "filename", O_RDWR | O_CLOEXEC | O_EXCL); + // CHECK-MESSAGES-NOT: warning: +} + +class G { +public: + int open(const char *pathname, int flags, ...); + int open64(const char *pathname, int flags, ...); + int openat(int dirfd, const char *pathname, int flags, ...); + + void h() { +open("filename", O_RDWR); +// CHECK-MESSAGES-NOT: warning: +open64("filename", O_RDWR); +// CHECK-MESSAGES-NOT: warning: +openat(0, "filename", O_RDWR); +// CHECK-MESSAGES-NOT: warning: +
[PATCH] D33304: [clang-tidy][Part1] Add a new module Android and three new checks.
yawanng added inline comments. Comment at: clang-tidy/android/FileOpenFlagCheck.cpp:61 + SourceLocation EndLoc = + Lexer::getLocForEndOfToken(FlagArg->getLocEnd(), 0, SM, getLangOpts()); + hokein wrote: > Instead of using getLangOpts(), you should use `Result.Context.getLangOpts()`. May I ask what's the difference between the `Result.Context.getLangOpts()` and `getLangOpts()`? Does `Result.Context.getLangOpts()` have a longer lifetime than `getLangOpts()`? Comment at: clang-tidy/android/FileOpenFlagCheck.cpp:79 +std::pairExpansionInfo = SM.getDecomposedLoc(Loc); +auto MacroName = +SM.getBufferData(ExpansionInfo.first) hokein wrote: > You could use `Lexer::getImmediateMacroName(Loc, SM, Opts);` to get the marco > name, or doesn't it work here? `getImmediateMacroName` sometimes cannot return the `O_CLOEXEC `, like #define O_CLOEXEC _O_CLOEXEC #define _O_CLOEXEC 1 `getImmediateMacroName` will return `_O_CLOEXEC`, whereas `O_CLOEXEC` is what we need. I change this to directly get the source text if it's a macro, which seems to be simpler. https://reviews.llvm.org/D33304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33304: [clang-tidy][Part1] Add a new module Android and three new checks.
hokein added inline comments. Comment at: clang-tidy/android/FileOpenFlagCheck.cpp:28 + hasParameter(1, hasType(isInteger())), + hasAnyName("open", "open64")) + .bind("funcDecl"))) I'd put the `hasAnyName` matcher in front of `hasParameter` which follows the order of function declaration. The same below. Comment at: clang-tidy/android/FileOpenFlagCheck.cpp:46 + const Expr *FlagArg; + if ((MatchedCall = Result.Nodes.getNodeAs("openFn"))) +FlagArg = MatchedCall->getArg(1); How about ``` const Expr *FlagArg = nullptr; if (const auto* OpenFnCall = Result.Nodes.getXXX()) { // ... } else if (const auto* OpenAtFnCall = Result.Nodes.getXX()) { // ... } assert(FlagArg); ``` ? Comment at: clang-tidy/android/FileOpenFlagCheck.cpp:61 + SourceLocation EndLoc = + Lexer::getLocForEndOfToken(FlagArg->getLocEnd(), 0, SM, getLangOpts()); + Instead of using getLangOpts(), you should use `Result.Context.getLangOpts()`. Comment at: clang-tidy/android/FileOpenFlagCheck.cpp:67 + +bool FileOpenFlagCheck::checkFlags(const Expr *Flags, const SourceManager ) { + bool IsFlagIn; No need to be a class member method, put it inside this translation unit. Comment at: clang-tidy/android/FileOpenFlagCheck.cpp:79 +std::pairExpansionInfo = SM.getDecomposedLoc(Loc); +auto MacroName = +SM.getBufferData(ExpansionInfo.first) You could use `Lexer::getImmediateMacroName(Loc, SM, Opts);` to get the marco name, or doesn't it work here? Comment at: clang-tidy/android/FileOpenFlagCheck.cpp:84 + +IsFlagIn = (MacroName == "O_CLOEXEC"); + I think you can use early return here. With that you don't need to maintain the flag variable `IsFlagIn`, so the code structure like: ``` if (isa<..>) { return ...; } if (isa()) { // ... return ...; } retrun false; ``` Comment at: clang-tidy/android/FileOpenFlagCheck.cpp:88 + // If it's a binary OR operation. + else if ((isa(Flags)) && + (cast(Flags)->getOpcode() == You can use `if (const auto* BO = dyn_cast(Flags))`, so that you don't call `cast(Flags)` multiple times below. Comment at: test/clang-tidy/android-file-open-flag.cpp:76 +void e() { + open("filename", O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: I would add tests where `O_CLOEXEC` is not at the end of parameter 2 expression like `open("filename", O_RDWR | O_CLOEXEC | O_EXCL)`. https://reviews.llvm.org/D33304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33304: [clang-tidy][Part1] Add a new module Android and three new checks.
yawanng added inline comments. Comment at: docs/clang-tidy/checks/android-file-open-flag.rst:6 + +A common source of security bugs has been code that opens file without using +the ``O_CLOEXEC`` flag. Without that flag, an opened sensitive file would alexfh wrote: > I'm not sure about using of "has been" here. Maybe just use present tense? > You might want to consult a nearby native speaker though. Alternatively, you > might want to rephrase this as "Opening files using POSIX ``open`` or similar > system calls without specifying the ``O_CLOEXEC`` flag is a common source of > security bugs." I copied these sentences from the requests written by a native speaker, I think. But after talking with another native speaker, yes, present tense seems to be better here. Thanks :) https://reviews.llvm.org/D33304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33304: [clang-tidy][Part1] Add a new module Android and three new checks.
yawanng updated this revision to Diff 102064. yawanng marked 6 inline comments as done. https://reviews.llvm.org/D33304 Files: clang-tidy/CMakeLists.txt clang-tidy/android/AndroidTidyModule.cpp clang-tidy/android/CMakeLists.txt clang-tidy/android/FileOpenFlagCheck.cpp clang-tidy/android/FileOpenFlagCheck.h clang-tidy/plugin/CMakeLists.txt clang-tidy/tool/CMakeLists.txt clang-tidy/tool/ClangTidyMain.cpp docs/ReleaseNotes.rst docs/clang-tidy/checks/android-file-open-flag.rst docs/clang-tidy/checks/list.rst docs/clang-tidy/index.rst test/clang-tidy/android-file-open-flag.cpp unittests/clang-tidy/CMakeLists.txt Index: unittests/clang-tidy/CMakeLists.txt === --- unittests/clang-tidy/CMakeLists.txt +++ unittests/clang-tidy/CMakeLists.txt @@ -25,6 +25,7 @@ clangFrontend clangLex clangTidy + clangTidyAndroidModule clangTidyGoogleModule clangTidyLLVMModule clangTidyMiscModule Index: test/clang-tidy/android-file-open-flag.cpp === --- /dev/null +++ test/clang-tidy/android-file-open-flag.cpp @@ -0,0 +1,104 @@ +// RUN: %check_clang_tidy %s android-file-open-flag %t + +#define O_RDWR 1 +#define O_EXCL 2 +#define __O_CLOEXEC 3 +#define O_CLOEXEC __O_CLOEXEC + +extern "C" int open(const char *fn, int flags, ...); +extern "C" int open64(const char *fn, int flags, ...); +extern "C" int openat(int dirfd, const char *pathname, int flags, ...); + +void a() { + open("filename", O_RDWR); + // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: 'open' should use O_CLOEXEC where possible [android-file-open-flag] + // CHECK-FIXES: O_RDWR | O_CLOEXEC + open("filename", O_RDWR | O_EXCL); + // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: 'open' should use O_CLOEXEC where + // CHECK-FIXES: O_RDWR | O_EXCL | O_CLOEXEC +} + +void b() { + open64("filename", O_RDWR); + // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: 'open64' should use O_CLOEXEC where possible [android-file-open-flag] + // CHECK-FIXES: O_RDWR | O_CLOEXEC + open64("filename", O_RDWR | O_EXCL); + // CHECK-MESSAGES: :[[@LINE-1]]:37: warning: 'open64' should use O_CLOEXEC where + // CHECK-FIXES: O_RDWR | O_EXCL | O_CLOEXEC +} + +void c() { + openat(0, "filename", O_RDWR); + // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: 'openat' should use O_CLOEXEC where possible [android-file-open-flag] + // CHECK-FIXES: O_RDWR | O_CLOEXEC + openat(0, "filename", O_RDWR | O_EXCL); + // CHECK-MESSAGES: :[[@LINE-1]]:40: warning: 'openat' should use O_CLOEXEC where + // CHECK-FIXES: O_RDWR | O_EXCL | O_CLOEXEC +} + +void f() { + open("filename", 3); + // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 'open' should use O_CLOEXEC where possible [android-file-open-flag] + // CHECK-FIXES: 3 | O_CLOEXEC + open64("filename", 3); + // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: 'open64' should use O_CLOEXEC where possible [android-file-open-flag] + // CHECK-FIXES: 3 | O_CLOEXEC + openat(0, "filename", 3); + // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: 'openat' should use O_CLOEXEC where possible [android-file-open-flag] + // CHECK-FIXES: 3 | O_CLOEXEC + + int flag = 3; + open("filename", flag); + // CHECK-MESSAGES-NOT: warning: + open64("filename", flag); + // CHECK-MESSAGES-NOT: warning: + openat(0, "filename", flag); + // CHECK-MESSAGES-NOT: warning: +} + +namespace i { +int open(const char *pathname, int flags, ...); +int open64(const char *pathname, int flags, ...); +int openat(int dirfd, const char *pathname, int flags, ...); + +void d() { + open("filename", O_RDWR); + // CHECK-MESSAGES-NOT: warning: + open64("filename", O_RDWR); + // CHECK-MESSAGES-NOT: warning: + openat(0, "filename", O_RDWR); + // CHECK-MESSAGES-NOT: warning: +} + +} // namespace i + +void e() { + open("filename", O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: + open("filename", O_RDWR | O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: + open64("filename", O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: + open64("filename", O_RDWR | O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: + openat(0, "filename", O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: + openat(0, "filename", O_RDWR | O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: +} + +class G { +public: + int open(const char *pathname, int flags, ...); + int open64(const char *pathname, int flags, ...); + int openat(int dirfd, const char *pathname, int flags, ...); + + void h() { +open("filename", O_RDWR); +// CHECK-MESSAGES-NOT: warning: +open64("filename", O_RDWR); +// CHECK-MESSAGES-NOT: warning: +openat(0, "filename", O_RDWR); +// CHECK-MESSAGES-NOT: warning: + } +}; Index: docs/clang-tidy/index.rst === --- docs/clang-tidy/index.rst +++ docs/clang-tidy/index.rst @@ -55,6 +55,7 @@ == = Name prefix
[PATCH] D33304: [clang-tidy][Part1] Add a new module Android and three new checks.
alexfh requested changes to this revision. alexfh added inline comments. This revision now requires changes to proceed. Comment at: clang-tidy/android/FileOpenFlagCheck.cpp:60 + + LangOptions LangOpts = getLangOpts(); + SourceRange FlagsRange(FlagArg->getLocStart(), FlagArg->getLocEnd()); No need for this variable, since it's just used once. Same below. Comment at: clang-tidy/android/FileOpenFlagCheck.cpp:62-65 + StringRef FlagsText = Lexer::getSourceText( + CharSourceRange::getTokenRange(FlagsRange), SM, LangOpts); + std::string ReplacementText = + (llvm::Twine(FlagsText) + " | " + O_CLOEXEC).str(); No need to replace the text with itself. Just insert the part that is missing. The more local the changes - the fewer are chances of conflicts. Comment at: clang-tidy/android/FileOpenFlagCheck.h:38 + + static constexpr const char *O_CLOEXEC = "O_CLOEXEC"; +}; This doesn't have to be a class member. It could be local to the function where it's used or to the .cc file. Comment at: docs/clang-tidy/checks/android-file-open-flag.rst:6 + +A common source of security bugs has been code that opens file without using +the ``O_CLOEXEC`` flag. Without that flag, an opened sensitive file would I'm not sure about using of "has been" here. Maybe just use present tense? You might want to consult a nearby native speaker though. Alternatively, you might want to rephrase this as "Opening files using POSIX ``open`` or similar system calls without specifying the ``O_CLOEXEC`` flag is a common source of security bugs." Comment at: docs/clang-tidy/checks/android-file-open-flag.rst:9 +remain open across a fork+exec to a lower-privileged SELinux domain, leaking +that sensitive data Functions including ``open()``, ``openat()``, and +``open64()`` must include ``O_CLOEXEC`` in their flags argument. Missing period before "Functions". Comment at: docs/clang-tidy/checks/android-file-open-flag.rst:9 +remain open across a fork+exec to a lower-privileged SELinux domain, leaking +that sensitive data Functions including ``open()``, ``openat()``, and +``open64()`` must include ``O_CLOEXEC`` in their flags argument. alexfh wrote: > Missing period before "Functions". Just "functions" is too broad. I'd say "`open`-like functions", "POSIX functions that open files" or something like that. https://reviews.llvm.org/D33304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33304: [clang-tidy][Part1] Add a new module Android and three new checks.
yawanng updated this revision to Diff 101831. yawanng marked an inline comment as done. https://reviews.llvm.org/D33304 Files: clang-tidy/CMakeLists.txt clang-tidy/android/AndroidTidyModule.cpp clang-tidy/android/CMakeLists.txt clang-tidy/android/FileOpenFlagCheck.cpp clang-tidy/android/FileOpenFlagCheck.h clang-tidy/plugin/CMakeLists.txt clang-tidy/tool/CMakeLists.txt clang-tidy/tool/ClangTidyMain.cpp docs/ReleaseNotes.rst docs/clang-tidy/checks/android-file-open-flag.rst docs/clang-tidy/checks/list.rst docs/clang-tidy/index.rst test/clang-tidy/android-file-open-flag.cpp unittests/clang-tidy/CMakeLists.txt Index: unittests/clang-tidy/CMakeLists.txt === --- unittests/clang-tidy/CMakeLists.txt +++ unittests/clang-tidy/CMakeLists.txt @@ -25,6 +25,7 @@ clangFrontend clangLex clangTidy + clangTidyAndroidModule clangTidyGoogleModule clangTidyLLVMModule clangTidyMiscModule Index: test/clang-tidy/android-file-open-flag.cpp === --- /dev/null +++ test/clang-tidy/android-file-open-flag.cpp @@ -0,0 +1,104 @@ +// RUN: %check_clang_tidy %s android-file-open-flag %t + +#define O_RDWR 1 +#define O_EXCL 2 +#define __O_CLOEXEC 3 +#define O_CLOEXEC __O_CLOEXEC + +extern "C" int open(const char *fn, int flags, ...); +extern "C" int open64(const char *fn, int flags, ...); +extern "C" int openat(int dirfd, const char *pathname, int flags, ...); + +void a() { + open("filename", O_RDWR); + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: open should use O_CLOEXEC where possible [android-file-open-flag] + // CHECK-FIXES: O_RDWR | O_CLOEXEC + open("filename", O_RDWR | O_EXCL); + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: open should use O_CLOEXEC where + // CHECK-FIXES: O_RDWR | O_EXCL | O_CLOEXEC +} + +void b() { + open64("filename", O_RDWR); + // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: open64 should use O_CLOEXEC where possible [android-file-open-flag] + // CHECK-FIXES: O_RDWR | O_CLOEXEC + open64("filename", O_RDWR | O_EXCL); + // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: open64 should use O_CLOEXEC where + // CHECK-FIXES: O_RDWR | O_EXCL | O_CLOEXEC +} + +void c() { + openat(0, "filename", O_RDWR); + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: openat should use O_CLOEXEC where possible [android-file-open-flag] + // CHECK-FIXES: O_RDWR | O_CLOEXEC + openat(0, "filename", O_RDWR | O_EXCL); + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: openat should use O_CLOEXEC where + // CHECK-FIXES: O_RDWR | O_EXCL | O_CLOEXEC +} + +void f() { + open("filename", 3); + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: open should use O_CLOEXEC where possible [android-file-open-flag] + // CHECK-FIXES: 3 | O_CLOEXEC + open64("filename", 3); + // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: open64 should use O_CLOEXEC where possible [android-file-open-flag] + // CHECK-FIXES: 3 | O_CLOEXEC + openat(0, "filename", 3); + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: openat should use O_CLOEXEC where possible [android-file-open-flag] + // CHECK-FIXES: 3 | O_CLOEXEC + + int flag = 3; + open("filename", flag); + // CHECK-MESSAGES-NOT: warning: + open64("filename", flag); + // CHECK-MESSAGES-NOT: warning: + openat(0, "filename", flag); + // CHECK-MESSAGES-NOT: warning: +} + +namespace i { +int open(const char *pathname, int flags, ...); +int open64(const char *pathname, int flags, ...); +int openat(int dirfd, const char *pathname, int flags, ...); + +void d() { + open("filename", O_RDWR); + // CHECK-MESSAGES-NOT: warning: + open64("filename", O_RDWR); + // CHECK-MESSAGES-NOT: warning: + openat(0, "filename", O_RDWR); + // CHECK-MESSAGES-NOT: warning: +} + +} // namespace i + +void e() { + open("filename", O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: + open("filename", O_RDWR | O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: + open64("filename", O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: + open64("filename", O_RDWR | O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: + openat(0, "filename", O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: + openat(0, "filename", O_RDWR | O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: +} + +class G { +public: + int open(const char *pathname, int flags, ...); + int open64(const char *pathname, int flags, ...); + int openat(int dirfd, const char *pathname, int flags, ...); + + void h() { +open("filename", O_RDWR); +// CHECK-MESSAGES-NOT: warning: +open64("filename", O_RDWR); +// CHECK-MESSAGES-NOT: warning: +openat(0, "filename", O_RDWR); +// CHECK-MESSAGES-NOT: warning: + } +}; Index: docs/clang-tidy/index.rst === --- docs/clang-tidy/index.rst +++ docs/clang-tidy/index.rst @@ -55,6 +55,7 @@ == = Name prefixDescription
[PATCH] D33304: [clang-tidy][Part1] Add a new module Android and three new checks.
hokein added a comment. In https://reviews.llvm.org/D33304#771493, @alexfh wrote: > IIUC, these checks enforce a certain - Android-specific - way of using POSIX > APIs. I'm not sure if the recommendations are universally useful. Or am I > mistaken? OK, that makes sense. I may miss some background context. Comment at: docs/clang-tidy/index.rst:58 == = +``android`` ``boost-`` Checks related to Boost library. Add some words explaining this module? https://reviews.llvm.org/D33304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33304: [clang-tidy][Part1] Add a new module Android and three new checks.
yawanng updated this revision to Diff 101279. yawanng added a comment. Format changes. https://reviews.llvm.org/D33304 Files: clang-tidy/CMakeLists.txt clang-tidy/android/AndroidTidyModule.cpp clang-tidy/android/CMakeLists.txt clang-tidy/android/FileOpenFlagCheck.cpp clang-tidy/android/FileOpenFlagCheck.h clang-tidy/plugin/CMakeLists.txt clang-tidy/tool/CMakeLists.txt clang-tidy/tool/ClangTidyMain.cpp docs/ReleaseNotes.rst docs/clang-tidy/checks/android-file-open-flag.rst docs/clang-tidy/checks/list.rst docs/clang-tidy/index.rst test/clang-tidy/android-file-open-flag.cpp unittests/clang-tidy/CMakeLists.txt Index: unittests/clang-tidy/CMakeLists.txt === --- unittests/clang-tidy/CMakeLists.txt +++ unittests/clang-tidy/CMakeLists.txt @@ -25,6 +25,7 @@ clangFrontend clangLex clangTidy + clangTidyAndroidModule clangTidyGoogleModule clangTidyLLVMModule clangTidyMiscModule Index: test/clang-tidy/android-file-open-flag.cpp === --- /dev/null +++ test/clang-tidy/android-file-open-flag.cpp @@ -0,0 +1,104 @@ +// RUN: %check_clang_tidy %s android-file-open-flag %t + +#define O_RDWR 1 +#define O_EXCL 2 +#define __O_CLOEXEC 3 +#define O_CLOEXEC __O_CLOEXEC + +extern "C" int open(const char *fn, int flags, ...); +extern "C" int open64(const char *fn, int flags, ...); +extern "C" int openat(int dirfd, const char *pathname, int flags, ...); + +void a() { + open("filename", O_RDWR); + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: open should use O_CLOEXEC where possible. [android-file-open-flag] + // CHECK-FIXES: O_RDWR | O_CLOEXEC + open("filename", O_RDWR | O_EXCL); + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: open should use O_CLOEXEC where + // CHECK-FIXES: O_RDWR | O_EXCL | O_CLOEXEC +} + +void b() { + open64("filename", O_RDWR); + // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: open64 should use O_CLOEXEC where possible. [android-file-open-flag] + // CHECK-FIXES: O_RDWR | O_CLOEXEC + open64("filename", O_RDWR | O_EXCL); + // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: open64 should use O_CLOEXEC where + // CHECK-FIXES: O_RDWR | O_EXCL | O_CLOEXEC +} + +void c() { + openat(0, "filename", O_RDWR); + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: openat should use O_CLOEXEC where possible. [android-file-open-flag] + // CHECK-FIXES: O_RDWR | O_CLOEXEC + openat(0, "filename", O_RDWR | O_EXCL); + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: openat should use O_CLOEXEC where + // CHECK-FIXES: O_RDWR | O_EXCL | O_CLOEXEC +} + +void f() { + open("filename", 3); + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: open should use O_CLOEXEC where possible. [android-file-open-flag] + // CHECK-FIXES: 3 | O_CLOEXEC + open64("filename", 3); + // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: open64 should use O_CLOEXEC where possible. [android-file-open-flag] + // CHECK-FIXES: 3 | O_CLOEXEC + openat(0, "filename", 3); + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: openat should use O_CLOEXEC where possible. [android-file-open-flag] + // CHECK-FIXES: 3 | O_CLOEXEC + + int flag = 3; + open("filename", flag); + // CHECK-MESSAGES-NOT: warning: + open64("filename", flag); + // CHECK-MESSAGES-NOT: warning: + openat(0, "filename", flag); + // CHECK-MESSAGES-NOT: warning: +} + +namespace i { +int open(const char *pathname, int flags, ...); +int open64(const char *pathname, int flags, ...); +int openat(int dirfd, const char *pathname, int flags, ...); + +void d() { + open("filename", O_RDWR); + // CHECK-MESSAGES-NOT: warning: + open64("filename", O_RDWR); + // CHECK-MESSAGES-NOT: warning: + openat(0, "filename", O_RDWR); + // CHECK-MESSAGES-NOT: warning: +} + +} // namespace i + +void e() { + open("filename", O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: + open("filename", O_RDWR | O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: + open64("filename", O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: + open64("filename", O_RDWR | O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: + openat(0, "filename", O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: + openat(0, "filename", O_RDWR | O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: +} + +class G { +public: + int open(const char *pathname, int flags, ...); + int open64(const char *pathname, int flags, ...); + int openat(int dirfd, const char *pathname, int flags, ...); + + void h() { +open("filename", O_RDWR); +// CHECK-MESSAGES-NOT: warning: +open64("filename", O_RDWR); +// CHECK-MESSAGES-NOT: warning: +openat(0, "filename", O_RDWR); +// CHECK-MESSAGES-NOT: warning: + } +}; Index: docs/clang-tidy/index.rst === --- docs/clang-tidy/index.rst +++ docs/clang-tidy/index.rst @@ -55,6 +55,7 @@ == = Name prefixDescription
[PATCH] D33304: [clang-tidy][Part1] Add a new module Android and three new checks.
yawanng updated this revision to Diff 101267. https://reviews.llvm.org/D33304 Files: clang-tidy/CMakeLists.txt clang-tidy/android/AndroidTidyModule.cpp clang-tidy/android/CMakeLists.txt clang-tidy/android/FileOpenFlagCheck.cpp clang-tidy/android/FileOpenFlagCheck.h clang-tidy/plugin/CMakeLists.txt clang-tidy/tool/CMakeLists.txt clang-tidy/tool/ClangTidyMain.cpp docs/ReleaseNotes.rst docs/clang-tidy/checks/android-file-open-flag.rst docs/clang-tidy/checks/list.rst docs/clang-tidy/index.rst test/clang-tidy/android-file-open-flag.cpp unittests/clang-tidy/CMakeLists.txt Index: unittests/clang-tidy/CMakeLists.txt === --- unittests/clang-tidy/CMakeLists.txt +++ unittests/clang-tidy/CMakeLists.txt @@ -25,6 +25,7 @@ clangFrontend clangLex clangTidy + clangTidyAndroidModule clangTidyGoogleModule clangTidyLLVMModule clangTidyMiscModule Index: test/clang-tidy/android-file-open-flag.cpp === --- /dev/null +++ test/clang-tidy/android-file-open-flag.cpp @@ -0,0 +1,104 @@ +// RUN: %check_clang_tidy %s android-file-open-flag %t + +#define O_RDWR 1 +#define O_EXCL 2 +#define __O_CLOEXEC 3 +#define O_CLOEXEC __O_CLOEXEC + +extern "C" int open(const char *fn, int flags, ...); +extern "C" int open64(const char *fn, int flags, ...); +extern "C" int openat(int dirfd, const char *pathname, int flags, ...); + +void a() { + open("filename", O_RDWR); + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: open should use O_CLOEXEC where possible. [android-file-open-flag] + // CHECK-FIXES: O_RDWR | O_CLOEXEC + open("filename", O_RDWR | O_EXCL); + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: open should use O_CLOEXEC where + // CHECK-FIXES: O_RDWR | O_EXCL | O_CLOEXEC +} + +void b() { + open64("filename", O_RDWR); + // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: open64 should use O_CLOEXEC where possible. [android-file-open-flag] + // CHECK-FIXES: O_RDWR | O_CLOEXEC + open64("filename", O_RDWR | O_EXCL); + // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: open64 should use O_CLOEXEC where + // CHECK-FIXES: O_RDWR | O_EXCL | O_CLOEXEC +} + +void c() { + openat(0, "filename", O_RDWR); + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: openat should use O_CLOEXEC where possible. [android-file-open-flag] + // CHECK-FIXES: O_RDWR | O_CLOEXEC + openat(0, "filename", O_RDWR | O_EXCL); + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: openat should use O_CLOEXEC where + // CHECK-FIXES: O_RDWR | O_EXCL | O_CLOEXEC +} + +void f() { + open("filename", 3); + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: open should use O_CLOEXEC where possible. [android-file-open-flag] + // CHECK-FIXES: 3 | O_CLOEXEC + open64("filename", 3); + // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: open64 should use O_CLOEXEC where possible. [android-file-open-flag] + // CHECK-FIXES: 3 | O_CLOEXEC + openat(0, "filename", 3); + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: openat should use O_CLOEXEC where possible. [android-file-open-flag] + // CHECK-FIXES: 3 | O_CLOEXEC + + int flag = 3; + open("filename", flag); + // CHECK-MESSAGES-NOT: warning: + open64("filename", flag); + // CHECK-MESSAGES-NOT: warning: + openat(0, "filename", flag); + // CHECK-MESSAGES-NOT: warning: +} + +namespace i { +int open(const char *pathname, int flags, ...); +int open64(const char *pathname, int flags, ...); +int openat(int dirfd, const char *pathname, int flags, ...); + +void d() { + open("filename", O_RDWR); + // CHECK-MESSAGES-NOT: warning: + open64("filename", O_RDWR); + // CHECK-MESSAGES-NOT: warning: + openat(0, "filename", O_RDWR); + // CHECK-MESSAGES-NOT: warning: +} + +} // namespace i + +void e() { + open("filename", O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: + open("filename", O_RDWR | O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: + open64("filename", O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: + open64("filename", O_RDWR | O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: + openat(0, "filename", O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: + openat(0, "filename", O_RDWR | O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: +} + +class G { +public: + int open(const char *pathname, int flags, ...); + int open64(const char *pathname, int flags, ...); + int openat(int dirfd, const char *pathname, int flags, ...); + + void h() { +open("filename", O_RDWR); +// CHECK-MESSAGES-NOT: warning: +open64("filename", O_RDWR); +// CHECK-MESSAGES-NOT: warning: +openat(0, "filename", O_RDWR); +// CHECK-MESSAGES-NOT: warning: + } +}; Index: docs/clang-tidy/index.rst === --- docs/clang-tidy/index.rst +++ docs/clang-tidy/index.rst @@ -55,6 +55,7 @@ == = Name prefixDescription ==
[PATCH] D33304: [clang-tidy][Part1] Add a new module Android and three new checks.
alexfh added a comment. In https://reviews.llvm.org/D33304#771215, @hokein wrote: > Thanks for the contributions. > > All your three checks are not android specific -- because they are checking > POSIX APIs (`open`, `creat`, `fopen`), which are more likely related to > POSIX. So personally, I'm +1 on a "posix" module, instead of "android", but > wait to see other reviewers' opinions before renaming it. IIUC, these checks enforce a certain - Android-specific - way of using POSIX APIs. I'm not sure if the recommendations are universally useful. Or am I mistaken? https://reviews.llvm.org/D33304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33304: [clang-tidy][Part1] Add a new module Android and three new checks.
hokein added a comment. Thanks for the contributions. All your three checks are not android specific -- because they are checking POSIX APIs (`open`, `creat`, `fopen`), which are more likely related to POSIX. So personally, I'm +1 on a "posix" module, instead of "android", but wait to see other reviewers' opinions before renaming it. https://reviews.llvm.org/D33304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33304: [clang-tidy][Part1] Add a new module Android and three new checks.
yawanng updated this revision to Diff 100934. yawanng edited the summary of this revision. https://reviews.llvm.org/D33304 Files: clang-tidy/CMakeLists.txt clang-tidy/android/AndroidTidyModule.cpp clang-tidy/android/CMakeLists.txt clang-tidy/android/FileOpenFlagCheck.cpp clang-tidy/android/FileOpenFlagCheck.h clang-tidy/plugin/CMakeLists.txt clang-tidy/tool/CMakeLists.txt clang-tidy/tool/ClangTidyMain.cpp docs/ReleaseNotes.rst docs/clang-tidy/checks/android-file-open-flag.rst docs/clang-tidy/checks/list.rst docs/clang-tidy/index.rst test/clang-tidy/android-file-open-flag.cpp unittests/clang-tidy/CMakeLists.txt Index: unittests/clang-tidy/CMakeLists.txt === --- unittests/clang-tidy/CMakeLists.txt +++ unittests/clang-tidy/CMakeLists.txt @@ -25,6 +25,7 @@ clangFrontend clangLex clangTidy + clangTidyAndroidModule clangTidyGoogleModule clangTidyLLVMModule clangTidyMiscModule Index: test/clang-tidy/android-file-open-flag.cpp === --- /dev/null +++ test/clang-tidy/android-file-open-flag.cpp @@ -0,0 +1,104 @@ +// RUN: %check_clang_tidy %s android-file-open-flag %t + +#define O_RDWR 1 +#define O_EXCL 2 +#define __O_CLOEXEC 3 +#define O_CLOEXEC __O_CLOEXEC + +extern "C" int open(const char *fn, int flags, ...); +extern "C" int open64(const char *fn, int flags, ...); +extern "C" int openat(int dirfd, const char *pathname, int flags, ...); + +void a() { + open("filename", O_RDWR); + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: open should use O_CLOEXEC where possible. [android-file-open-flag] + // CHECK-FIXES: O_RDWR | O_CLOEXEC + open("filename", O_RDWR | O_EXCL); + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: open should use O_CLOEXEC where possible. [android-file-open-flag] + // CHECK-FIXES: O_RDWR | O_EXCL | O_CLOEXEC +} + +void b() { + open64("filename", O_RDWR); + // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: open64 should use O_CLOEXEC where possible. [android-file-open-flag] + // CHECK-FIXES: O_RDWR | O_CLOEXEC + open64("filename", O_RDWR | O_EXCL); + // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: open64 should use O_CLOEXEC where possible. [android-file-open-flag] + // CHECK-FIXES: O_RDWR | O_EXCL | O_CLOEXEC +} + +void c() { + openat(0, "filename", O_RDWR); + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: openat should use O_CLOEXEC where possible. [android-file-open-flag] + // CHECK-FIXES: O_RDWR | O_CLOEXEC + openat(0, "filename", O_RDWR | O_EXCL); + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: openat should use O_CLOEXEC where possible. [android-file-open-flag] + // CHECK-FIXES: O_RDWR | O_EXCL | O_CLOEXEC +} + +void f() { + open("filename", 3); + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: open should use O_CLOEXEC where possible. [android-file-open-flag] + // CHECK-FIXES: 3 | O_CLOEXEC + open64("filename", 3); + // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: open64 should use O_CLOEXEC where possible. [android-file-open-flag] + // CHECK-FIXES: 3 | O_CLOEXEC + openat(0, "filename", 3); + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: openat should use O_CLOEXEC where possible. [android-file-open-flag] + // CHECK-FIXES: 3 | O_CLOEXEC + + int flag = 3; + open("filename", flag); + // CHECK-MESSAGES-NOT: warning: + open64("filename", flag); + // CHECK-MESSAGES-NOT: warning: + openat(0, "filename", flag); + // CHECK-MESSAGES-NOT: warning: +} + +namespace i { +int open(const char *pathname, int flags, ...); +int open64(const char *pathname, int flags, ...); +int openat(int dirfd, const char *pathname, int flags, ...); + +void d() { + open("filename", O_RDWR); + // CHECK-MESSAGES-NOT: warning: + open64("filename", O_RDWR); + // CHECK-MESSAGES-NOT: warning: + openat(0, "filename", O_RDWR); + // CHECK-MESSAGES-NOT: warning: +} + +} // namespace i + +void e() { + open("filename", O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: + open("filename", O_RDWR | O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: + open64("filename", O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: + open64("filename", O_RDWR | O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: + openat(0, "filename", O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: + openat(0, "filename", O_RDWR | O_CLOEXEC); + // CHECK-MESSAGES-NOT: warning: +} + +class G { +public: + int open(const char *pathname, int flags, ...); + int open64(const char *pathname, int flags, ...); + int openat(int dirfd, const char *pathname, int flags, ...); + + void h() { +open("filename", O_RDWR); +// CHECK-MESSAGES-NOT: warning: +open64("filename", O_RDWR); +// CHECK-MESSAGES-NOT: warning: +openat(0, "filename", O_RDWR); +// CHECK-MESSAGES-NOT: warning: + } +}; Index: docs/clang-tidy/index.rst === --- docs/clang-tidy/index.rst +++ docs/clang-tidy/index.rst @@ -55,6 +55,7 @@