[analyzer] StdLibraryFunctionsChecker: Add argument constraints

[merge_guards_bot]

Unit tests: pass. 62418 tests passed, 0 failed and 845 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[NoQ]

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

452

Yeah, a must-have for this check to be enabled by default would be to be able to provide a specific warning message for every function. I guess we could include them in the summaries as an extra argument of ArgConstraint.

455

Let's test our notes.

That'll be especially important when we get to non-concrete values, because the visitor might need to be expanded (or we might need a completely new visitor).

[xazax.hun]

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

442

Dealing with only concrete ints might be a good start but we might want to handle symbolic cases in the future like:

if (v > 255) 
  return isalpha(v);

I am ok with not addig this in the first version but adding TODOs and test cases upfront cannot hurt.

So basivally, I was wondering if we should query the solver for the result instead of matching the sval kind and just early return if we do not want to support a specific kind.

[Szelethus]

I wouldn't like to see reports emitted by a checker that resides in apiModeling. Could we create a new one? Some checkers, like the IteratorChecker, MallocChecker and CStringChecker implement a variety of user-facing checkers within the same class, that is also an option, if creating a new checker class is too much of a hassle.

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

704–706

This is true for the rest of the summaries as well, but shouldn't we retrieve the unsigned char size from ASTContext?

[martong]

In D73898#1863710, @Szelethus wrote:

I wouldn't like to see reports emitted by a checker that resides in apiModeling. Could we create a new one? Some checkers, like the IteratorChecker, MallocChecker and CStringChecker implement a variety of user-facing checkers within the same class, that is also an option, if creating a new checker class is too much of a hassle.

Yes, we could split the warning emitting part to a new checker. My concern with that is in that case we would have the argument constraining part in checkPostCall still in this checker, because that is part of the modelling. And actually it makes sense to apply the argument constraints only if we know for sure that they are not violated. The violation then would be checked in the new checker, this seems a bit awkward to me. Because checking the violation of the constraints and applying the constraints seems to be a cohesive action to me. I mean it would not even make sense to turn off the warning checker, because then we'd be applying the constraints blindly.

[martong]

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

704–706

Yes this is a good idea. I will do this.

What bothers me really much, however, is that we should handle EOF in a platform dependent way as well ... and I have absolutely no idea how to do that given that is defined by a macro in a platform specific header file. I am desperately in need for help and ideas about how could we get the value of EOF for the analysed platform.

[martong]

In D73898#1864066, @martong wrote:

In D73898#1863710, @Szelethus wrote:

I wouldn't like to see reports emitted by a checker that resides in apiModeling. Could we create a new one? Some checkers, like the IteratorChecker, MallocChecker and CStringChecker implement a variety of user-facing checkers within the same class, that is also an option, if creating a new checker class is too much of a hassle.

... And actually it makes sense to apply the argument constraints only if we know for sure that they are not violated. ...

What I mean by that is that we must do over-approximation if the argument is symbolic. I.e. we presume that the constraints do hold otherwise the program would be ill-formed and there is no point to continue the analysis on this path. It is very similar to what we do in case of the DivZero or the NullDeref Checkers: if there is no violation (no warning) and the variable is symbolic then we constrain the value by the condition. E.g. in DivZero::checkPreStmt we have:

// If we get here, then the denom should not be zero. We abandon the implicit
// zero denom case for now.
C.addTransition(stateNotZero);

Strictly speaking, these transitions should be part of the modeling then in this sense (and they should be in PostStmt?). Still they are not separated into a different checker.

[martong]

What I mean by that is that we must do over-approximation if the argument is symbolic. I.e. we presume that the constraints do hold otherwise the program would be ill-formed and there is no point to continue the analysis on this path.

Sorry, that's actually under-approximation because we elide paths.

[martong]

Based on our verbal discussion with @Szelethus and @steakhal and based on the mailing archives, I am going to do the following changes:

• Add a new checker that is implemented in the StdLibraryFunctionsChecker class.
• This new checker if switched on is responsible for emitting the warning. Even if this is turned off, the sink node is generated if the argument violates the given condition.
• This means, the new checker has the sole responsibility of emitting the warning, but nothing more.

[martong]

• Rebase to master

[balazske]

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

442

This check works now with concrete int values. We have a known value and a list of ranges with known limits, so testing for in any of the ranges does work the same way as testing for out of all ranges. And testing if the value is inside one of the ranges is more simple code.

But I think the symbolic evaluation with "eval" and "assume" functions would be more generic here (and more simple code). Then the way of cutting-of the bad ranges is usable (probably still there is other solution).

503

If evalCall is used it could be more simple to test and apply the constraints for arguments and return value in a "single step".

[balazske]

Probably a better solution can be:
For every "case" build a single SVal that contains all argument constraints for that case. It is possible using multiple evalBinOp calls (with <=, >=, logical or) to build such a condition (or repeated calls to other assume functions to cut off outer ranges). If the condition can be satisfied (by assume) add the new state, the condition for return value can be added here too. Repeat this for every different case. If no applicable case is found none of the conditions can be assumed, this means argument constraint error.

[martong]

• Add new Checker that does the report
• Refactor with negated RangeValues
• Add overload to findFunctionSummary
• Add tests for symbolic values
• Add test file for bug path

[martong]

I've done a major refactor with the handling of argument constraints. I am now reusing ValueRange::apply in checkPreCall on "negated" value ranges to check if the constraint is violated.

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

452

What about warning messages with placeholders? E.g. "Argument constraint of {nth} argument of {fun} is not satisfied. The value is {v}, however it should be in {range}."
There will be a bunch of functions whose warning message template would be the same. On the other hand some others could have different warnings, and that justifies the need for specialized warnings.
Still, I think the warning message in the summary should be optional, because otherwise it would be really hard to automatically add summaries from other sources (like from cppcheck).

No matter how it turns out, this should be handled in a different patch.

455

Ok, I added a separate test file where the tests focus on the bug path.

[martong]

• Remove leftover call from test

[steakhal]

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

441

StringRef

[balazske]

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

461

Is this addTransition needed? It would be OK to call generateErrorNode with State. Even if not, adding the transition before should not be needed?

719–720

Why is this {128, UCharMax} here and at the next entry needed?

728

Is this ArgConstraint intentionally added only to isalnum?

[martong]

• Use StringRef for Msg
• Remove superfluous addTransition

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

461

Yes, you are right it is superfluous, I removed it.

[martong]

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

719–720

This is the local specific range , [128, 255]. There are characters like ä which we don't know if they are treated as an alphanumerical character or not. We can't really tell how a specific libc implementation classifies them. On the other hand, with English letters we can state the classes confidently.

728

Yes, I wanted to create first the infrastructure and then later to add all these constraints to the rest of the summaries with new tests.

[martong]

Rebase on top of https://reviews.llvm.org/D74973

[martong]

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

165

This default branch is not needed here (actually gives a compiler warning too).

[gamesh411]

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

704–706

If the EOF is not used in the TU analyzed, then there would be no way to find the specific #define.
Another approach would be to check if the value is defined by an expression that is the EOF define (maybe transitively?).

[balazske]

It may be useful to make a "macro value map" kind of object. Some macros can be added to it as a string, and it is possible to lookup for an Expr if one of the added macros is used there. This can be done by checking the concrete (numeric) value of the Expr and compare to the value of the macro, or by checking if the expression comes from a macro and take this macro name (use string comparison). Such an object can be useful because the functionality is needed at more checkers, for example the ones I am working on (StreamChecker and ErrorReturnChecker too).

[balazske]

something like this:

class MacroUsageDetector {
public:
  void addMacroName(StringRef MName);
  bool isMacroUsed(StringRef MName, Expr *E, ???);
  APSInt getMacroValue(StringRef MName);
};

Or one that handles a single macro?

[Szelethus]

The high level idea and the implementation of the checker seems great. In general, things that you want to address in later patches should be stated in the code with a TODO. I wrote a couple nits that I don't want to delete, but maybe it'd be better to address them after the dependency patch is agreed upon.

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

299

How about we add an example as well?

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

9–10

I suspect this comment is no longer relevant.

156

Maybe complement would be a better name? That sounds a lot more like a set operation. Also, this function highlights well that inheritance might not be the best solution here.

197

I think that is a rather poor example to help understand what list of list of ranges means :) -- Could you try to find something better?

437–445

While I find your usage of lambdas fascinating, this one seems a bit unnecessary :)

440

That is a TODO, rather :^)

clang/test/Analysis/std-c-library-functions-arg-constraints.c

2–8

Hmm, why do we have 2 different test files that essentially do the same? Shouldn't we only have a single one with analyzer-output=text?

clang/test/Analysis/std-c-library-functions.c

1–32

What a beautiful sight. Thanks.

[balazske]

Is it sure that the signedness in the ranges is handled correctly? The EOF is a negative value but the RangeInt is unsigned type. The tryExpandAsInteger returns int too that is put into an unsigned RangeInt later. Probably it is better to use APSInt for the ranges? (The problem exists already before this change.)

[martong]

In D73898#1894923, @balazske wrote:

It may be useful to make a "macro value map" kind of object. Some macros can be added to it as a string, and it is possible to lookup for an Expr if one of the added macros is used there. This can be done by checking the concrete (numeric) value of the Expr and compare to the value of the macro, or by checking if the expression comes from a macro and take this macro name (use string comparison). Such an object can be useful because the functionality is needed at more checkers, for example the ones I am working on (StreamChecker and ErrorReturnChecker too).

Please see my previous answer to @gamesh411

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

299

You mean like NonNull or other constraints?

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

9–10

Uh, yes.

156

Well, we check the argument constraint validity by trying to apply it's logical negation. In case of a range inclusion this is being out of that range. In case of non-null this is being null. And so on. The logic how we try to check an argument constraint is the same in all cases of the different constraints. And that is the point: in order to support a new kind of constraint we just have to figure out how to "apply" and "negate" one constraint. In my opinion this is a perfect case for polimorphism.

197

Yeah, that part definitely should be reworded.

704–706

I believe that the given standard C lib implementation (e.g. glibc) must provide a header for the prototypes of these functions where EOF is also defined transitively in any of the dependent system headers. Otherwise user code could misuse the value of EOF and thus the program would behave in an undefined manner.

C99 clearly states that you should #include <ctype.h> to use isalhpa.

clang/test/Analysis/std-c-library-functions-arg-constraints.c

2–8

No, I wanted to have two different test files to test two different things: (1) We do have the constraints applied (here we don't care about the warnings and the path)
(2) Check that we have a warning with the proper tracking and notes.

clang/test/Analysis/std-c-library-functions.c

1–32

Anytime :D

[martong]

In D73898#1901142, @balazske wrote:

Is it sure that the signedness in the ranges is handled correctly? The EOF is a negative value but the RangeInt is unsigned type. The tryExpandAsInteger returns int too that is put into an unsigned RangeInt later. Probably it is better to use APSInt for the ranges? (The problem exists already before this change.)

That is not a problem, because finally in apply we use an APSInt that is constructed by considering the correct T type, e.g.:

const llvm::APSInt &Min = BVF.getValue(R[I].first, T);

We could consider RangeInt as a buffer that is big enough to hold the representation of the range values. The concrete interpretation of the bits (as T) is done by APSInt.

[Szelethus]

Just littering some more inlines, don't mind me :) Lets still wait on the dependency patch before updating.

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

299

Like

Check constraints of arguments of C standard library functions, such as whether the parameter of isalpha is in the range [0, 255] or is EOF.

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

91–92

How about ValueConstraintRef?

156

We agreed on inheritance in the previous patch, and regarding the name, sure, leave it as-is. :)

[NoQ]

Looks great as long as other reviewers are happy, thanks!

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

456–458

Maybe we should add an assertion that the same argument isn't specified multiple times.

[martong]

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

299

Ok, done.

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

91–92

Yeah, we have ProgramStateRef and SymbolRef. And both are actually just synonyms to smart pointers. I'd rather not call a pointer as a reference, because that can be confusing when reading the code. E.g. when I see that we return with a nullptr from a function that can return with a ...Ref I start to scratch my head.

197

I added an example with isalpha.

437–445

Ok I moved it to be a member function named ReportBug.

456–458

I think there could be cases when we want to have e.g. a not-null constraint on the 1st argument, but also we want to express that the 1st argument's size is described by the 2nd argument. I am planning to implement such a constraints in the future. In that case we would have two constraints on the 1st argument and the assert would fire.

[martong]

• Address review comments

[martong]

• tmp -> Tmp

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

165

This default branch is not needed here (actually gives a compiler warning too).

I am not sure why I thought that that's not needed, actually we need that. (Perhaps an intermediate version returned in each cases.)

[Szelethus]

LGTM, aside from some checker tagging nightmare. Its a bit easy to mess up, please allow me to have a final look before commiting! :)

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

298

Just noticed, this checker still lies in the apiModeling package. Could we find a more appropriate place?

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

91–92

Sure, I'm sold.

264

By passing this, the error message will be tied to the modeling checker, not to the one you just added. BugType has a constructor that accepts a string instead, pass CheckNames[CK_StdCLibraryFunctionArgsChecker] in there :) Also, how about BT_InvalidArgument or something?

[balazske]

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

112

Is it better done with = 0?

197

The "branches" are the structures that define relations between arguments and return values? This could be included in the description.

301

This should be called reportBug.

[martong]

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

298

Technically speaking this is still api modeling. In midterm we'd like to add support for more libc functions, gnu and posix functions, they are all library functions i.e. they provide some api.
Of course in long term, we'd like to experiment by getting some constraints from IR/Attributor, but we are still far from there.

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

112

Not all of the constraint classes must implement this. Right now, e.g. the ComparisonConstraint does not implement this, because there is no such summary (yet) that uses a ComparisonConstraint as an argument constraint.

197

Not exactly. A branch represents a path in the exploded graph of a function (which is a tree).
So, a branch is a series of assumptions. In other words, branches represent split states and additional assumptions on top of the splitting assumption. I added this explanation to the comments.

264

Thanks, good catch, I did not know about that. Please note that using CheckNames requires that we change the BT member to be lazily initialized. Because CheckNames is initialized only after the checker itself is created, thus we cannot initialize BT during the checkers construction, b/c that would be before CheckNames is set. So, I changed BT to be a unique_ptr and it is being lazily initialized in reportBug.

301

Yeah, can't get used to this strange naming convention that LLVM uses. Fixed it.

[martong]

• Add comments about what is a branch
• Do not use 'this' for BugType
• Lazily init BT and BT -> BT_InvalidArg
• ReportBug -> reportBug

[Szelethus]

Whoo! The patch looks great and well thought out, the tests look like they cover everything and we also talked about plans for future patches. Excellent!

I left a nit about merging the test files, but I'll leave it up to you to address or ignore it.

clang/test/Analysis/std-c-library-functions-arg-constraints.c

2–8

What if we had different -verifys? clang/test/Analysis/track-conditions.cpp is a great example.

[martong]

• Use prefixes for -verify to check different things in the same test file

[martong]

Thanks for the review guys!

clang/test/Analysis/std-c-library-functions-arg-constraints.c

2–8

Yeah, that's a very good approach, I just changed it like that. :)

[NoQ]

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

456–458

Wait, i misunderstood the code. It's even worse than that: you're adding transitions in a loop, so it'll cause state splits for every constraint. Because you do not intend to create multiple branches here, there needs to be exactly one addTransition performed every time checkPreCall is called. I.e., for now this code is breaking everything whenever there's more than one constraint, regardless of whether it's on the same argument.

[martong]

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

456–458

Yeah, that's a very good catch, thanks! I am going to prepare a patch to fix this soon. My idea is to store the SuccessSt and apply the next argument constraint on that. And once the loop is finished I'll have call the addTransition().

[NoQ]

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

456–458

Yup, that's the common thing to do in such cases.

[NoQ]

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

456–458

While we're at it, could you try to come up with a runtime assertion that'll help us prevent these mistakes?

Like, dunno, make CheckerContext crash whenever there's more than one branch being added, and then add a method to opt out when it's actually necessary to add more transitions (i.e., the user would say C.setMaxTransitions(2) at the beginning of their checker callback whenever they need to make a state split, defaulting to 1). It's a bit tricky because i still want to allow multiple transitions when they allow one branch (i.e., transitions chained together) but i think it'll take a lot of review anxiety from me because it's a very dangerous mistake to make and for now code review is the only way to catch it. So, yay, faster code reviews.

[martong]

I just created a quick fix for the issue: https://reviews.llvm.org/D76790

[martong]

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

456–458

Hmm I see your point and I agree this would be a valuable sanity check. But if you don't mind I'd like to address this in a different and stand-alone patch (independently from the quick-fix https://reviews.llvm.org/D76790) because it does not seem to be trivial for me.

My first concern is this: if we have 1 as the default value for maxTranisitions then we should add an extra C.setMaxTransitions(N) in every checker callback that does a state split, is that right?

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

 //===----------------------------------------------------------------------===//
 
 let ParentPackage = APIModeling in {
 
 def StdCLibraryFunctionsChecker : Checker<"StdCLibraryFunctions">,
   HelpText<"Improve modeling of the C standard library functions">,
   Documentation<NotDocumented>;
 
+def StdCLibraryFunctionArgsChecker : Checker<"StdCLibraryFunctionArgs">,
+  HelpText<"Check constraints of arguments of C standard library functions, "
+           "such as whether the parameter of isalpha is in the range [0, 255] "
+           "or is EOF.">,
+  Dependencies<[StdCLibraryFunctionsChecker]>,
+  Documentation<NotDocumented>;
+
 def TrustNonnullChecker : Checker<"TrustNonnull">,
   HelpText<"Trust that returns from framework methods annotated with _Nonnull "
            "are not null">,
   Documentation<NotDocumented>;
 
 } // end "apiModeling"
 
 //===----------------------------------------------------------------------===//

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

 //=== StdLibraryFunctionsChecker.cpp - Model standard functions -*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This checker improves modeling of a few simple library functions.
-// It does not generate warnings.
 //
 // This checker provides a specification format - `Summary' - and
 // contains descriptions of some library functions in this format. Each
 // specification contains a list of branches for splitting the program state
 // upon call, and range constraints on argument and return-value symbols that
 // are satisfied on each branch. This spec can be expanded to include more
 // items, like external effects of the function.
 //
 // The main difference between this approach and the body farms technique is

 //   fwrite     isalpha   islower   read
 //   getc       isascii   isprint   write
 //   getchar    isblank   ispunct
 //   getdelim   iscntrl   isspace
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
 
 using namespace clang;
 using namespace clang::ento;
 
 namespace {
-class StdLibraryFunctionsChecker : public Checker<check::PostCall, eval::Call> {
+class StdLibraryFunctionsChecker
+    : public Checker<check::PreCall, check::PostCall, eval::Call> {
   /// Below is a series of typedefs necessary to define function specs.
   /// We avoid nesting types here because each additional qualifier
   /// would need to be repeated in every function spec.
   struct Summary;
 
   /// Specify how much the analyzer engine should entrust modeling this function
   /// to us. If he doesn't, he performs additional invalidations.
   enum InvalidationKind { NoEvalCall, EvalCallAsPure };
class StdLibraryFunctionsChecker
   typedef std::vector<std::pair<RangeInt, RangeInt>> IntRangeVector;
 
   /// A reference to an argument or return value by its number.
   /// ArgNo in CallExpr and CallEvent is defined as Unsigned, but
   /// obviously uint32_t should be enough for all practical purposes.
   typedef uint32_t ArgNo;
   static const ArgNo Ret;
 
+  class ValueConstraint;
+
+  // Pointer to the ValueConstraint. We need a copyable, polymorphic and
+  // default initialize able type (vector needs that). A raw pointer was good,
+  // however, we cannot default initialize that. unique_ptr makes the Summary
+  // class non-copyable, therefore not an option. Releasing the copyability
+  // requirement would render the initialization of the Summary map infeasible.
+  using ValueConstraintPtr = std::shared_ptr<ValueConstraint>;
+
   /// Polymorphic base class that represents a constraint on a given argument
   /// (or return value) of a function. Derived classes implement different kind
   /// of constraints, e.g range constraints or correlation between two
   /// arguments.
   class ValueConstraint {
   public:
     ValueConstraint(ArgNo ArgN) : ArgN(ArgN) {}
     virtual ~ValueConstraint() {}
     /// Apply the effects of the constraint on the given program state. If null
     /// is returned then the constraint is not feasible.
     virtual ProgramStateRef apply(ProgramStateRef State, const CallEvent &Call,
                                   const Summary &Summary) const = 0;
+    virtual ValueConstraintPtr negate() const {
+      llvm_unreachable("Not implemented");
+    };
     ArgNo getArgNo() const { return ArgN; }
 
   protected:
     ArgNo ArgN; // Argument to which we apply the constraint.
   };
 
   /// Given a range, should the argument stay inside or outside this range?
   enum RangeKind { OutOfRange, WithinRange };
ProgramStateRef apply(ProgramStateRef State, const CallEvent &Call,
       switch (Kind) {
       case OutOfRange:
         return applyAsOutOfRange(State, Call, Summary);
       case WithinRange:
         return applyAsWithinRange(State, Call, Summary);
       }
       llvm_unreachable("Unknown range kind!");
     }
+
+    ValueConstraintPtr negate() const override {
+      RangeConstraint Tmp(*this);
+      switch (Kind) {
+      case OutOfRange:
+        Tmp.Kind = WithinRange;
+        break;
+      case WithinRange:
+        Tmp.Kind = OutOfRange;
+        break;
+      default:
+        llvm_unreachable("Unknown RangeConstraint kind!");
+      }
+      return std::make_shared<RangeConstraint>(Tmp);
+    }
   };
 
   class ComparisonConstraint : public ValueConstraint {
     BinaryOperator::Opcode Opcode;
     ArgNo OtherArgN;
 
   public:
     ComparisonConstraint(ArgNo ArgN, BinaryOperator::Opcode Opcode,
                          ArgNo OtherArgN)
         : ValueConstraint(ArgN), Opcode(Opcode), OtherArgN(OtherArgN) {}
     ArgNo getOtherArgNo() const { return OtherArgN; }
     BinaryOperator::Opcode getOpcode() const { return Opcode; }
     ProgramStateRef apply(ProgramStateRef State, const CallEvent &Call,
                           const Summary &Summary) const override;
   };
 
-  // Pointer to the ValueConstraint. We need a copyable, polymorphic and
-  // default initialize able type (vector needs that). A raw pointer was good,
-  // however, we cannot default initialize that. unique_ptr makes the Summary
-  // class non-copyable, therefore not an option. Releasing the copyability
-  // requirement would render the initialization of the Summary map infeasible.
-  using ValueConstraintPtr = std::shared_ptr<ValueConstraint>;
   /// The complete list of constraints that defines a single branch.
   typedef std::vector<ValueConstraintPtr> ConstraintSet;
 
   using ArgTypes = std::vector<QualType>;
   using Cases = std::vector<ConstraintSet>;
 
-  /// Includes information about function prototype (which is necessary to
-  /// ensure we're modeling the right function and casting values properly),
-  /// approach to invalidation, and a list of branches - essentially, a list
-  /// of list of ranges - essentially, a list of lists of lists of segments.
+  /// Includes information about
+  ///   * function prototype (which is necessary to
+  ///     ensure we're modeling the right function and casting values properly),
+  ///   * approach to invalidation,
+  ///   * a list of branches - a list of list of ranges -
+  ///     A branch represents a path in the exploded graph of a function (which
+  ///     is a tree). So, a branch is a series of assumptions. In other words,
+  ///     branches represent split states and additional assumptions on top of
+  ///     the splitting assumption.
+  ///     For example, consider the branches in `isalpha(x)`
+  ///       Branch 1)
+  ///         x is in range ['A', 'Z'] or in ['a', 'z']
+  ///         then the return value is not 0. (I.e. out-of-range [0, 0])
+  ///       Branch 2)
+  ///         x is out-of-range ['A', 'Z'] and out-of-range ['a', 'z']
+  ///         then the return value is 0.
+  ///   * a list of argument constraints, that must be true on every branch.
+  ///     If these constraints are not satisfied that means a fatal error
+  ///     usually resulting in undefined behaviour.
   struct Summary {
     const ArgTypes ArgTys;
     const QualType RetTy;
     const InvalidationKind InvalidationKd;
     Cases CaseConstraints;
     ConstraintSet ArgConstraints;
 
     Summary(ArgTypes ArgTys, QualType RetTy, InvalidationKind InvalidationKd)
         : ArgTys(ArgTys), RetTy(RetTy), InvalidationKd(InvalidationKd) {}
 
     Summary &Case(ConstraintSet&& CS) {
       CaseConstraints.push_back(std::move(CS));
       return *this;
     }
+    Summary &ArgConstraint(ValueConstraintPtr VC) {
+      ArgConstraints.push_back(VC);
+      return *this;
+    }
 
   private:
     static void assertTypeSuitableForSummary(QualType T) {
       assert(!T->isVoidType() &&
              "We should have had no significant void types in the spec");
       assert(T.isCanonical() &&
              "We should only have canonical types in the spec");
       // FIXME: lift this assert (but not the ones above!)
class StdLibraryFunctionsChecker
   // C++ function overloads, and also it can be used when the same function
   // may have different definitions on different platforms.
   typedef std::vector<Summary> Summaries;
 
   // The map of all functions supported by the checker. It is initialized
   // lazily, and it doesn't change after initialization.
   mutable llvm::StringMap<Summaries> FunctionSummaryMap;
 
+  mutable std::unique_ptr<BugType> BT_InvalidArg;
+
   // Auxiliary functions to support ArgNo within all structures
   // in a unified manner.
   static QualType getArgType(const Summary &Summary, ArgNo ArgN) {
     return Summary.getArgType(ArgN);
   }
   static QualType getArgType(const CallEvent &Call, ArgNo ArgN) {
     return ArgN == Ret ? Call.getResultType().getCanonicalType()
                        : Call.getArgExpr(ArgN)->getType().getCanonicalType();
   }
   static QualType getArgType(const CallExpr *CE, ArgNo ArgN) {
     return ArgN == Ret ? CE->getType().getCanonicalType()
                        : CE->getArg(ArgN)->getType().getCanonicalType();
   }
   static SVal getArgSVal(const CallEvent &Call, ArgNo ArgN) {
     return ArgN == Ret ? Call.getReturnValue() : Call.getArgSVal(ArgN);
   }
 
 public:
+  void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
   void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
   bool evalCall(const CallEvent &Call, CheckerContext &C) const;
 
+  enum CheckKind { CK_StdCLibraryFunctionArgsChecker, CK_NumCheckKinds };
+  DefaultBool ChecksEnabled[CK_NumCheckKinds];
+  CheckerNameRef CheckNames[CK_NumCheckKinds];
+
 private:
   Optional<Summary> findFunctionSummary(const FunctionDecl *FD,
                                         const CallExpr *CE,
                                         CheckerContext &C) const;
+  Optional<Summary> findFunctionSummary(const CallEvent &Call,
+                                        CheckerContext &C) const;
 
   void initFunctionSummaries(CheckerContext &C) const;
+
+  void reportBug(const CallEvent &Call, ExplodedNode *N,
+                 CheckerContext &C) const {
+    if (!ChecksEnabled[CK_StdCLibraryFunctionArgsChecker])
+      return;
+    // TODO Add detailed diagnostic.
+    StringRef Msg = "Function argument constraint is not satisfied";
+    if (!BT_InvalidArg)
+      BT_InvalidArg = std::make_unique<BugType>(
+          CheckNames[CK_StdCLibraryFunctionArgsChecker],
+          "Unsatisfied argument constraints", categories::LogicError);
+    auto R = std::make_unique<PathSensitiveBugReport>(*BT_InvalidArg, Msg, N);
+    bugreporter::trackExpressionValue(N, Call.getArgExpr(0), *R);
+    C.emitReport(std::move(R));
+  }
 };
 
 const StdLibraryFunctionsChecker::ArgNo StdLibraryFunctionsChecker::Ret =
     std::numeric_limits<ArgNo>::max();
 
 } // end of anonymous namespace
 
 ProgramStateRef StdLibraryFunctionsChecker::RangeConstraint::applyAsOutOfRange(
ProgramStateRef StdLibraryFunctionsChecker::ComparisonConstraint::apply(
   // Note: we avoid integral promotion for comparison.
   OtherV = SVB.evalCast(OtherV, T, OtherT);
   if (auto CompV = SVB.evalBinOp(State, Op, V, OtherV, CondT)
                        .getAs<DefinedOrUnknownSVal>())
     State = State->assume(*CompV, true);
   return State;
 }
 
-void StdLibraryFunctionsChecker::checkPostCall(const CallEvent &Call,
-                                               CheckerContext &C) const {
-  const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(Call.getDecl());
-  if (!FD)
+void StdLibraryFunctionsChecker::checkPreCall(const CallEvent &Call,
+  Optional<Summary> FoundSummary = findFunctionSummary(Call, C);
+  if (!FoundSummary)
     return;
 
-  const CallExpr *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
-  if (!CE)
-    return;
+  const Summary &Summary = *FoundSummary;
+  ProgramStateRef State = C.getState();
 
-  Optional<Summary> FoundSummary = findFunctionSummary(FD, CE, C);
+  for (const ValueConstraintPtr& VC : Summary.ArgConstraints) {
+    ProgramStateRef SuccessSt = VC->apply(State, Call, Summary);
+    ProgramStateRef FailureSt = VC->negate()->apply(State, Call, Summary);
+    // The argument constraint is not satisfied.
+    if (FailureSt && !SuccessSt) {
+      if (ExplodedNode *N = C.generateErrorNode(State))
+        reportBug(Call, N, C);
+      break;
+    } else {
+      // Apply the constraint even if we cannot reason about the argument. This
+      // means both SuccessSt and FailureSt can be true. If we weren't applying
+      // the constraint that would mean that symbolic execution continues on a
+      // code whose behaviour is undefined.
+      assert(SuccessSt);
+      C.addTransition(SuccessSt);
+    }
+  }
+}
+
+void StdLibraryFunctionsChecker::checkPostCall(const CallEvent &Call,
+                                               CheckerContext &C) const {
+  Optional<Summary> FoundSummary = findFunctionSummary(Call, C);
   if (!FoundSummary)
     return;
 
   // Now apply the constraints.
   const Summary &Summary = *FoundSummary;
   ProgramStateRef State = C.getState();
 
   // Apply case/branch specifications.
   for (const auto &VRS : Summary.CaseConstraints) {
     ProgramStateRef NewState = State;
     for (const auto &VR: VRS) {
       NewState = VR->apply(NewState, Call, Summary);
       if (!NewState)
         break;
     }
 
     if (NewState && NewState != State)
       C.addTransition(NewState);
   }
 }
 
 bool StdLibraryFunctionsChecker::evalCall(const CallEvent &Call,
                                           CheckerContext &C) const {
-  const auto *FD = dyn_cast_or_null<FunctionDecl>(Call.getDecl());
-  if (!FD)
-    return false;
-
-  const auto *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
-  if (!CE)
-    return false;
-
-  Optional<Summary> FoundSummary = findFunctionSummary(FD, CE, C);
+  Optional<Summary> FoundSummary = findFunctionSummary(Call, C);
   if (!FoundSummary)
     return false;
 
   const Summary &Summary = *FoundSummary;
   switch (Summary.InvalidationKd) {
   case EvalCallAsPure: {
     ProgramStateRef State = C.getState();
     const LocationContext *LC = C.getLocationContext();
+    const auto *CE = cast_or_null<CallExpr>(Call.getOriginExpr());
     SVal V = C.getSValBuilder().conjureSymbolVal(
         CE, LC, CE->getType().getCanonicalType(), C.blockCount());
     State = State->BindExpr(CE, LC, V);
     C.addTransition(State);
     return true;
   }
   case NoEvalCall:
     // Summary tells us to avoid performing eval::Call. The function is possibly
     // evaluated by another checker, or evaluated conservatively.
     return false;
   }
   llvm_unreachable("Unknown invalidation kind!");
 }
 
 bool StdLibraryFunctionsChecker::Summary::matchesCall(
     const CallExpr *CE) const {
   // Check number of arguments:
   if (CE->getNumArgs() != ArgTys.size())
     return false;
 
StdLibraryFunctionsChecker::findFunctionSummary(const FunctionDecl *FD,
   const Summaries &SpecVariants = FSMI->second;
   for (const Summary &Spec : SpecVariants)
     if (Spec.matchesCall(CE))
       return Spec;
 
   return None;
 }
 
+Optional<StdLibraryFunctionsChecker::Summary>
+StdLibraryFunctionsChecker::findFunctionSummary(const CallEvent &Call,
+                                                CheckerContext &C) const {
+  const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(Call.getDecl());
+  if (!FD)
+    return None;
+  const CallExpr *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
+  if (!CE)
+    return None;
+  return findFunctionSummary(FD, CE, C);
+}
+
 void StdLibraryFunctionsChecker::initFunctionSummaries(
     CheckerContext &C) const {
   if (!FunctionSummaryMap.empty())
     return;
 
   SValBuilder &SVB = C.getSValBuilder();
   BasicValueFactory &BVF = SVB.getBasicValueFactory();
   const ASTContext &ACtx = BVF.getContext();
auto Range = [](RangeInt b, RangeInt e) {
     return IntRangeVector{std::pair<RangeInt, RangeInt>{b, e}};
   };
   auto SingleValue = [](RangeInt v) {
     return IntRangeVector{std::pair<RangeInt, RangeInt>{v, v}};
   };
   auto LessThanOrEq = BO_LE;
 
   using RetType = QualType;
-
   // Templates for summaries that are reused by many functions.
   auto Getc = [&]() {
     return Summary(ArgTypes{Irrelevant}, RetType{IntTy}, NoEvalCall)
         .Case({ReturnValueCondition(WithinRange,
                                     {{EOFv, EOFv}, {0, UCharRangeMax}})});
   };
   auto Read = [&](RetType R, RangeInt Max) {
     return Summary(ArgTypes{Irrelevant, Irrelevant, SizeTy}, RetType{R},
void StdLibraryFunctionsChecker::initFunctionSummaries(
   auto Getline = [&](RetType R, RangeInt Max) {
     return Summary(ArgTypes{Irrelevant, Irrelevant, Irrelevant}, RetType{R},
                    NoEvalCall)
         .Case({ReturnValueCondition(WithinRange, {{-1, -1}, {1, Max}})});
   };
 
   FunctionSummaryMap = {
       // The isascii() family of functions.
+      // The behavior is undefined if the value of the argument is not
+      // representable as unsigned char or is not equal to EOF. See e.g. C99
+      // 7.4.1.2 The isalpha function (p: 181-182).
       {
           "isalnum",
           Summaries{
               Summary(ArgTypes{IntTy}, RetType{IntTy}, EvalCallAsPure)
                   // Boils down to isupper() or islower() or isdigit().
                   .Case(
                       {ArgumentCondition(0U, WithinRange,
                                          {{'0', '9'}, {'A', 'Z'}, {'a', 'z'}}),
                        ReturnValueCondition(OutOfRange, SingleValue(0))})
                   // The locale-specific range.
                   // No post-condition. We are completely unaware of
                   // locale-specific return values.
                   .Case({ArgumentCondition(0U, WithinRange,
                                            {{128, UCharRangeMax}})})
                   .Case({ArgumentCondition(0U, OutOfRange,
                                            {{'0', '9'},
                                             {'A', 'Z'},
                                             {'a', 'z'},
                                             {128, UCharRangeMax}}),
-                         ReturnValueCondition(WithinRange, SingleValue(0))})},
+                         ReturnValueCondition(WithinRange, SingleValue(0))})
+                  .ArgConstraint(ArgumentCondition(
+                      0U, WithinRange, {{EOFv, EOFv}, {0, UCharRangeMax}}))},
       },
       {
           "isalpha",
           Summaries{
               Summary(ArgTypes{IntTy}, RetType{IntTy}, EvalCallAsPure)
                   .Case({ArgumentCondition(0U, WithinRange,
                                            {{'A', 'Z'}, {'a', 'z'}}),
                          ReturnValueCondition(OutOfRange, SingleValue(0))})
FunctionSummaryMap = {
       {"getline", Summaries{Getline(IntTy, IntMax), Getline(LongTy, LongMax),
                             Getline(LongLongTy, LongLongMax)}},
       {"getdelim", Summaries{Getline(IntTy, IntMax), Getline(LongTy, LongMax),
                              Getline(LongLongTy, LongLongMax)}},
   };
 }
 
 void ento::registerStdCLibraryFunctionsChecker(CheckerManager &mgr) {
-  // If this checker grows large enough to support C++, Objective-C, or other
-  // standard libraries, we could use multiple register...Checker() functions,
-  // which would register various checkers with the help of the same Checker
-  // class, turning on different function summaries.
   mgr.registerChecker<StdLibraryFunctionsChecker>();
 }
 
 bool ento::shouldRegisterStdCLibraryFunctionsChecker(const LangOptions &LO) {
   return true;
 }
+
+#define REGISTER_CHECKER(name)                                                 \
+  void ento::register##name(CheckerManager &mgr) {                             \
+    StdLibraryFunctionsChecker *checker =                                      \
+        mgr.getChecker<StdLibraryFunctionsChecker>();                          \
+    checker->ChecksEnabled[StdLibraryFunctionsChecker::CK_##name] = true;      \
+    checker->CheckNames[StdLibraryFunctionsChecker::CK_##name] =               \
+        mgr.getCurrentCheckerName();                                           \
+  }                                                                            \
+                                                                               \
+  bool ento::shouldRegister##name(const LangOptions &LO) { return true; }
+
+REGISTER_CHECKER(StdCLibraryFunctionArgsChecker)

clang/test/Analysis/analyzer-enabled-checkers.c

 // RUN: %clang --analyze %s --target=x86_64-pc-linux-gnu \
 // RUN:   -Xclang -analyzer-list-enabled-checkers \
 // RUN:   -Xclang -analyzer-display-progress \
 // RUN:   2>&1 | FileCheck %s --implicit-check-not=ANALYZE \
 // RUN:                       --implicit-check-not=\.
 
 // CHECK:      OVERVIEW: Clang Static Analyzer Enabled Checkers List
 // CHECK-EMPTY:
 // CHECK-NEXT: apiModeling.StdCLibraryFunctions
+// CHECK-NEXT: apiModeling.StdCLibraryFunctionArgs
 // CHECK-NEXT: apiModeling.TrustNonnull
 // CHECK-NEXT: apiModeling.llvm.CastValue
 // CHECK-NEXT: apiModeling.llvm.ReturnValue
 // CHECK-NEXT: core.CallAndMessage
 // CHECK-NEXT: core.DivideZero
 // CHECK-NEXT: core.DynamicTypePropagation
 // CHECK-NEXT: core.NonNullParamChecker
 // CHECK-NEXT: core.NonnilStringConstants

clang/test/Analysis/std-c-library-functions-arg-constraints.c

+// Check the basic reporting/warning and the application of constraints.
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctionArgs \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -triple x86_64-unknown-linux-gnu \
+// RUN:   -verify=report
+
+// Check the bugpath related to the reports.
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctionArgs \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -triple x86_64-unknown-linux-gnu \
+// RUN:   -analyzer-output=text \
+// RUN:   -verify=bugpath
+
+void clang_analyzer_eval(int);
+
+int glob;
+
+#define EOF -1
+
+int isalnum(int);
+
+void test_alnum_concrete(int v) {
+  int ret = isalnum(256); // \
+  // report-warning{{Function argument constraint is not satisfied}} \
+  // bugpath-warning{{Function argument constraint is not satisfied}} \
+  // bugpath-note{{Function argument constraint is not satisfied}}
+  (void)ret;
+}
+
+void test_alnum_symbolic(int x) {
+  int ret = isalnum(x);
+  (void)ret;
+
+  clang_analyzer_eval(EOF <= x && x <= 255); // \
+  // report-warning{{TRUE}} \
+  // bugpath-warning{{TRUE}} \
+  // bugpath-note{{TRUE}} \
+  // bugpath-note{{Left side of '&&' is true}} \
+  // bugpath-note{{'x' is <= 255}}
+
+}
+
+void test_alnum_symbolic2(int x) {
+  if (x > 255) { // \
+    // bugpath-note{{Assuming 'x' is > 255}} \
+    // bugpath-note{{Taking true branch}}
+
+    int ret = isalnum(x); // \
+    // report-warning{{Function argument constraint is not satisfied}} \
+    // bugpath-warning{{Function argument constraint is not satisfied}} \
+    // bugpath-note{{Function argument constraint is not satisfied}}
+
+    (void)ret;
+  }
+}

clang/test/Analysis/std-c-library-functions.c

-// RUN: %clang_analyze_cc1 -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple i686-unknown-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple x86_64-unknown-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple armv7-a15-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple thumbv7-a15-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple i686-unknown-linux \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple x86_64-unknown-linux \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple armv7-a15-linux \
+// RUN:   -verify
+
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=apiModeling.StdCLibraryFunctions \
+// RUN:   -analyzer-checker=debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false \
+// RUN:   -triple thumbv7-a15-linux \
+// RUN:   -verify
 
 void clang_analyzer_eval(int);
 
 int glob;
 
 typedef struct FILE FILE;
 #define EOF -1
 
 int getc(FILE *);