Page MenuHomePhabricator

[clang-tidy] Refactor the code and add a close-on-exec check on memfd_create() in Android module.
ClosedPublic

Authored by yawanng on Jul 13 2017, 10:50 AM.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
yawanng edited the summary of this revision. (Show Details)Jul 13 2017, 10:53 AM
yawanng edited the summary of this revision. (Show Details)Jul 13 2017, 11:00 AM
Eugene.Zelenko edited reviewers, added: alexfh, aaron.ballman, hokein; removed: chh.Jul 13 2017, 11:22 AM
Eugene.Zelenko set the repository for this revision to rL LLVM.
Eugene.Zelenko added a subscriber: cfe-commits.
chh added a subscriber: chh.Jul 13 2017, 12:01 PM
Eugene.Zelenko added inline comments.
docs/ReleaseNotes.rst
60

Please sort checks in alphabetical order.

yawanng updated this revision to Diff 106515.Jul 13 2017, 1:41 PM
yawanng marked an inline comment as done.
alexfh edited edge metadata.Jul 15 2017, 1:15 AM

I have deja vu ;)

Should we make a single check for all CLOEXEC and friends with a single configurable list of (function name, flag name) pairs?

alexfh requested changes to this revision.Jul 15 2017, 1:18 AM
This revision now requires changes to proceed.Jul 15 2017, 1:18 AM

I have deja vu ;)

Should we make a single check for all CLOEXEC and friends with a single configurable list of (function name, flag name) pairs?

Okay, it may be a bit more complicated than just a list of function name -> flag name mappings, since we have to take in account the argument position as well. We also might want to check the signature to a certain degree to avoid matching wrong function. There are multiple approaches possible to rule out incorrect functions with the same name:

  1. just look at the number of arguments - this might well be enough, since for a certain codebase I wouldn't expect multiple memfd_create's etc. It would allow user configurability of the function -> flag mappings.
  2. encode the types of arguments as strings and have a small dictionary of matchers in the check (e.g. "const char*" -> pointerType(pointee(isAnyCharacter()))) - that will be more precise and still quite flexible and also allow user-configurable function -> flag mappings. But this mechanism may be an overkill, if we don't anticipate user-configurable functions. I don't know how complex the resulting code turns out to be.
  3. Add a matcher for each function statically. This would obviously allow for arbitrarily complex matchers, but won't be extensible via configuration options.
yawanng added a comment.EditedJul 17 2017, 10:51 AM

I have deja vu ;)

Should we make a single check for all CLOEXEC and friends with a single configurable list of (function name, flag name) pairs?

Okay, it may be a bit more complicated than just a list of function name -> flag name mappings, since we have to take in account the argument position as well. We also might want to check the signature to a certain degree to avoid matching wrong function. There are multiple approaches possible to rule out incorrect functions with the same name:

  1. just look at the number of arguments - this might well be enough, since for a certain codebase I wouldn't expect multiple memfd_create's etc. It would allow user configurability of the function -> flag mappings.
  2. encode the types of arguments as strings and have a small dictionary of matchers in the check (e.g. "const char*" -> pointerType(pointee(isAnyCharacter()))) - that will be more precise and still quite flexible and also allow user-configurable function -> flag mappings. But this mechanism may be an overkill, if we don't anticipate user-configurable functions. I don't know how complex the resulting code turns out to be.
  3. Add a matcher for each function statically. This would obviously allow for arbitrarily complex matchers, but won't be extensible via configuration options.

Great idea. But we may prefer separate checks, because each API can be controlled independently. To reduce the code duplication, what about a base class for all of them and try to share as much code as possible? The base class may be complex due to handling different numbers of arguments and types as well as the different fix suggestions and etc, the gain is that each separate checks is relatively simple :-)

chh added a comment.Jul 17 2017, 11:09 AM

If most code can be shared in a common base class like CloexecCheck,
maybe all 8 "Add a close-on-exec check" CLs can be combined into 1 or 2 CLs
to consolidate all review efforts.
I also prefer a separate check name for each function,
so users can enable/disable each check.

yawanng updated this revision to Diff 107557.EditedJul 20 2017, 10:59 AM
yawanng edited edge metadata.

Refactor the check, add a base class for it, which can facilitate all other similar checks. Basically, all checks in the same category will have only one or two lines code in both "check" and "registerMatcher" by inheriting the base class. If this looks good, I will modify all other similar ones. Thank you :-)

yawanng retitled this revision from [clang-tidy] Add a close-on-exec check on memfd_create() in Android module. to [clang-tidy] Refactor the code and add a close-on-exec check on memfd_create() in Android module..Jul 28 2017, 4:04 PM
yawanng edited the summary of this revision. (Show Details)
hokein edited edge metadata.Aug 3 2017, 2:02 AM

Sorry for the delay, and thanks for refining it.

In general, I'm fine with the current design, a few comments below.

clang-tidy/android/CloexecCheck.cpp
63

Use const auto*, obviously indicates this is a pointer, the same to other places.

95

I think there will be a use-after-free issue here.

The method returns a StringRef (which doesn't own the "std::string"), the temporary std::string will be destructed after the method finished.

clang-tidy/android/CloexecCheck.h
19

This base class will be used in a bunch of cloase-on-exec checks.
It deserves a better and more detailed documentation (e.g. the intended usage).

27

I think we can drop the do for most methods, how about

doRegisterMatchers => registerMatchers
doMacroFlagInsertion => insertMacroFlag
doFuncReplacement => replaceFunc
doStringFlagInsertion => insertStringFlag?

31

Instead of using the magic string funDecl anywhere, I would define a static const member for it. The same to func.

36

It is unclear to me what the "issue" means here? I assume there are three types of fixes?

37

Deserve a document on the behavior of the method, would be better to give an example. The same below.

Is Flag required to be a macro flag? looks like it could be any string.

54

Maybe getSpellingArg which sounds more natural?

55

s/n/N

76

should we put this method and below in the base class? They seem to be check-specific, I think?

yawanng updated this revision to Diff 109589.Aug 3 2017, 10:26 AM
yawanng marked 9 inline comments as done.
yawanng added inline comments.
clang-tidy/android/CloexecCheck.h
36

Yes. It means three types of fixes :-)

37

Yes, the flag is expected to be a string of the required macro name.

76

Only the 'mode_t' one is currently only used by one check. The reason for putting them all together here is to be kind of uniform and maybe facilitate possible future checks that may contain this type.

hokein added inline comments.Aug 4 2017, 9:29 AM
clang-tidy/android/CloexecCheck.cpp
67

const auto*

clang-tidy/android/CloexecCheck.h
12

A blank line here.

24

Normally, the style should like:

class Foo {
public:
  ...
protected:
  ...
privated:
  ...
};

BTW, in C++11 these are declarations, you still need to define them in the .cc file like

const static constexpr char* CloexecCheck::FuncDeclBindingStr;
33

I think this method (and below) can be protected member.

52

I'd name it MacroFlag.

60

It is not obvious to see what these parameter mean. Msg is a message for warning, maybe WarningMsg?

I'd suggest using the formal documentation for this class (https://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments).

64

e is not a string, is a char.

65

s/fpen/fopen.

69

Is char for Mode sufficient? Seems to me that only fopen check is using it, or there will be more checks using this in the future?

73

n should be capital N. I think we can make it a const member method. And I can't see any usage of this method in this patch.

76

Looks like this method is used internally, do we need to expose it? or just define it as an anonymous function in the cc file?

80

The inline is redundant.

clang-tidy/android/CloexecMemfdCreateCheck.cpp
25

nit: add /*ArgPos=*/ before parameter 1 to make it more readable.

yawanng updated this revision to Diff 109778.Aug 4 2017, 10:50 AM
yawanng marked 13 inline comments as done.
yawanng added inline comments.
clang-tidy/android/CloexecCheck.h
73

It's not used in this one, but in others.

hokein accepted this revision.Aug 7 2017, 12:01 PM

Looks good to me, a few nits. Thanks for improving it continuously.

I'd hold it for a while to see whether @alexfh has further comments before submitting it.

clang-tidy/android/CloexecCheck.h
29

remove an extra blank before and.

46

Nit: you are missing the parameter part in the comment, the same below.

/// \param Result XXX
/// \param MacroFlag XXX
/// \param ArgPos
48

Is the ArgPos 0-based or 1-based? I know it is 0-based, but we'd better mention in the document part.

117

nit: I'd put this method as the first protected method.

yawanng updated this revision to Diff 110065.Aug 7 2017, 1:15 PM
yawanng marked 4 inline comments as done.

Looks good to me, a few nits. Thanks for improving it continuously.

I'd hold it for a while to see whether @alexfh has further comments before submitting it.

Thank you for the reviewing :-)

alexfh added a comment.Aug 9 2017, 9:39 AM

I'm not sure this approach is the best to deal with the boilerplate, but it seems like an improvement compared to repeating code. See a few comments inline.

clang-tidy/android/CloexecCheck.cpp
67
  1. Should this be a class method?
  2. Is this ever used? (if it is going to be used in a different check, let's postpone its addition to that moment)
clang-tidy/android/CloexecCheck.h
44

Can we make this method non-template and put its implementation to the .cpp file? It could accept a Matcher<FunctionDecl> or something like that and just apply it:

void registerForCall(MatchFinder *Finder, Matcher<FunctionDecl> Function) {

Finder->addMatcher(calExpr(callee(functionDecl(isExternC(), Function).bind(...))).bind(...), this);

}

Then you could make FuncDeclBindingStr and FuncBindingStr local to the file.

48

s/we has/we have/

103–138

These methods don't add much value: hasParameter(N, hasType(isInteger())) is not much longer or harder to read than hasIntegerParameter(N), etc. Also having these utilities as non-static methods doesn't make much sense. If you want a shorter way to describe function signatures, I would suggest a single utility function hasParameters() that would take a list of type matchers and apply them to the corresponding parameters. That one could be useful more broadly than in this specific check, so it could be placed in utils/ (and later in ASTMatchers.h, if we find enough potential users).

141

You're missing one more const: static const char * const FuncDeclBindingStr;

alexfh requested changes to this revision.Aug 9 2017, 9:40 AM
This revision now requires changes to proceed.Aug 9 2017, 9:40 AM
yawanng updated this revision to Diff 110443.Aug 9 2017, 11:29 AM
yawanng edited edge metadata.
yawanng marked 5 inline comments as done.
alexfh accepted this revision.Aug 10 2017, 6:06 AM

LG with a few nits.

clang-tidy/android/CloexecCheck.cpp
50

No need to qualify names in ast_matchers::, since there's a using directive above.

clang-tidy/android/CloexecCheck.h
39

nit: Please add an empty line before this comment.

55

Please remove top-level const from the last two arguments. It has no effect in declaration (and definition can still use top-level const, if needed, since it is not a part of the function signature). Same below.

This revision is now accepted and ready to land.Aug 10 2017, 6:06 AM
yawanng updated this revision to Diff 110606.Aug 10 2017, 10:13 AM
yawanng marked 3 inline comments as done.
yawanng closed this revision.Aug 10 2017, 10:18 AM

rL310669 is the latest and complete version. It fixes an issue in windows platform.