Page MenuHomePhabricator

[analyzer][StdLibraryFunctionsChecker] Add POSIX pthread handling functions
ClosedPublic

Authored by martong on Jul 23 2020, 7:59 AM.

Diff Detail

Event Timeline

martong created this revision.Jul 23 2020, 7:59 AM
martong added subscribers: vsavchenko, NoQ.

@NoQ, @vsavchenko My apologies, somehow I forgot to add you as reviewers.

Off-topic: I really think that we should have some sort of DSL for that kind of stuff.

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
2034

It feels like the readability of the code here can be drastically improved by introducing functions getPointerType, getRestrictType, and similar accepting Optional arguments.

martong updated this revision to Diff 285626.Aug 14 2020, 4:53 AM
martong marked an inline comment as done.
  • Use the shorter 'getPointerTy'

Off-topic: I really think that we should have some sort of DSL for that kind of stuff.

In a sense the API provided in the Checker itself is an (embedded) DSL. Or do you think about being able to provide the summaries by describing them in a JSON or a YAML file?

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
2034

Yeah, we already have getRestrictTy, but never though to have getPointerTy, which indeed simplifies the code, thanks!

Even some macro-like construct can improve readability. The current way of adding functions is source of copy-paste errors because more things are repeated, these should be written only once. And the ifs for the existence of types can be eliminated (automated) somehow. So the final way of specifying the functions is to only list (get) the types first, then add the functions with name, arguments and constraints in a way that nothing needs to be repeated and computable parts are done automatically.

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
2034

I think the improvement would be if these get functions accept Optional arguments and if the argument is empty return an empty value. This can make the ifs unnecessary.

martong marked an inline comment as done.Aug 14 2020, 7:11 AM

Even some macro-like construct can improve readability. The current way of adding functions is source of copy-paste errors because more things are repeated, these should be written only once. And the ifs for the existence of types can be eliminated (automated) somehow. So the final way of specifying the functions is to only list (get) the types first, then add the functions with name, arguments and constraints in a way that nothing needs to be repeated and computable parts are done automatically.

Yes, this totally makes sense, thank you guys for this suggestion!

Now I've started working on to reach this form, what do you think?

// Taking an optional as arg.
Optional<QualType> Pthread_cond_tPtrTy = getPointerTy(Pthread_cond_tTy);
addToFunctionSummaryMap(
    {"pthread_cond_signal", "pthread_cond_broadcast"},
    // Create a signature with Optionals, addToFunctionSummaryMap will skip
    // the summary if the optional is not set.
    Signature(ArgTypes{Pthread_cond_tPtrTy}, RetType{IntTy}),
    Summary(NoEvalCall)
        .ArgConstraint(NotNull(ArgNo(0))));

Side note, one day hopefully we could have the same API to fill a CallDescriptionMap.

martong updated this revision to Diff 285682.Aug 14 2020, 10:17 AM
  • Use Optionals through the Signature

Now I've update the patch to remove all the ifs. And the optional types are now implicitly handled, so no such mistakes can be done that I had done previously.
However, the patch is getting to affect the core of this Checker, so, perhaps I should split this into two patches? One NFC for the core changes and another that involves only the addition of the pthread functions?

It is better now, but the refactoring change should be in a separate revision (it affects the other already added functions too).

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
359

Not very bad but I do not like to call another constructor and assignment here, this can be many redundant operations. It is better to fill the internal arrays here and have a separate assert function that checks for validness (of the internal arrays) instead of the private constructor.

381

Check for isInvalid here (with assert)?

935

Another idea: Have a class that handles all the variants (simple, pointer, const pointer, const restrict pointer, restrict). It can have get functions that compute the type if not done yet, or get every variant at first time even if it is later not used (or has no sense).
For example create it like TypeVariants SizeTy{ACtx.getSizeType()}; and then call SizeTy.getType(), SizeTy.asPtr(), SizeTy.asPtrRestrict(), ... .

1052–1053

These ifs can be removed too (in another patch).

martong updated this revision to Diff 287409.Aug 24 2020, 8:31 AM
martong marked 4 inline comments as done.
  • Remove private ctor of Signature
  • Assert a valid signature in Signature::matces

Thanks @balazske for your comments, you always make an assiduous review!

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
359

Ok, makes sense, thanks!

381

We cannot do that, this is not a member function of the Signature, this operates on a FuncitonDecl. But it makes sense to add the assert to the mathces member function.

935

I'd rather keep the current form, because I think it makes it easier to compose types from different base types (and it is similar to the ASTMatchers in this sense).

1052–1053

Yep, I am going to create another base patch.

martong planned changes to this revision.Aug 24 2020, 9:13 AM

Planning to rebase this on a dependent patch that uses an API with Optionals.

balazske added inline comments.Aug 25 2020, 12:16 AM
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
935

If I got it correctly, the problem with my approach above is that it has separate functions for every special type variant. The current is acceptable but looks not fully uniform to me (the type has a withConst and for getting a pointer or restrict there is a function call). We could have something like a "TypeBuilder" that has methods withConst(), withRestrict(), withPointerTo() (these can return a TypeBuilder too) and finally a (omittable) getType() that returns Optional<QualType>.

const QualType ConstVoidPtrRestrictTy = TypeBuilder{ACtx, VoidTy}.withConst().withPointerTo().withRestrict().getType();
martong added inline comments.Aug 25 2020, 2:00 AM
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
935

Yeah, .withConst() is making it nonuniform currently, you are right.
What if we had getConstTy() that receives an optional? Composability would be just perfect in this case :)

Optional<QualType> ConstPthread_attr_tPtrTy = getPointerTy(getConstTy(Pthread_attr_tTy));

WDYT?

martong updated this revision to Diff 287640.Aug 25 2020, 5:18 AM
  • Rebase to parent patch
martong updated this revision to Diff 289970.Sep 4 2020, 8:49 AM
  • Rebase to master

Polite ping.

Szelethus accepted this revision.Sep 7 2020, 2:30 AM

@balazske seems to be very involved, he might have some closing words -- from my end, LGTM!

This revision is now accepted and ready to land.Sep 7 2020, 2:30 AM
vsavchenko accepted this revision.Sep 7 2020, 2:40 AM

LGTM!
Thanks!

balazske accepted this revision.Sep 7 2020, 6:23 AM