[analyzer] Add path note tags to standard library function summaries.

[NoQ]

Summary

This is a solution to the issue with getenv() (https://github.com/llvm/llvm-project/issues/53276) but I covered a few more functions just because I could.

The patch is straightforward except the tiny fix in BugReporterVisitors.cpp that suppresses a default note for "Assuming pointer value is null" when a note tag from the checker is present. This is probably the right thing to do but also definitely not a complete solution to the problem of different sources of path notes being unaware of each other, which is a large and annoying issue that we have to deal with. Note tags really help there because they're nicely introspectable. The problem is demonstrated by the newly added getenv() test; I did not investigate why doesn't the original buggy report have the same note but I agree that this might be interesting to figure out.

The notes are currently optional but I think we should eventually implement all of them and then make them mandatory.

The notes are prunable, i.e. they won't bring-in entire stack frames worth of notes just because they're there, but they will be always visible regardless of whether the value is of interest to the bug report. I think this is debatable, the arguably better solution is to make them non-prunable but conditional to the value being tracked back to the call, which would probably need a better tracking infrastructure.

[NoQ]

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

41

This comment has been out of date for years and I don't think it makes sense to have it in the first place.

1215–1217

I'm not adding notes to these ranges because these days I think they were a mistake. A three-way (sometimes four-way) state split is pretty hard to justify. It's much safer to drop the case and stick to the two ASCII cases.

[steakhal]

The notes are prunable, i.e. they won't bring-in entire stack frames worth of notes just because they're there, but they will be always visible regardless of whether the value is of interest to the bug report. I think this is debatable, the arguably better solution is to make them non-prunable but conditional to the value being tracked back to the call, which would probably need a better tracking infrastructure.

I was thinking of passing a lambda and doing the rest there. We could have lambda factories to make it less cumbersome to define - and also reuse code.

IMO this is a nice increment.

[Szelethus]

LGTM on my end, this is awesome!

In D122285#3401754, @steakhal wrote:

The notes are prunable, i.e. they won't bring-in entire stack frames worth of notes just because they're there, but they will be always visible regardless of whether the value is of interest to the bug report. I think this is debatable, the arguably better solution is to make them non-prunable but conditional to the value being tracked back to the call, which would probably need a better tracking infrastructure.

I was thinking of passing a lambda and doing the rest there. We could have lambda factories to make it less cumbersome to define - and also reuse code.

Could you think of a scenario where a lambda would be required? It sure is more general, but I don't immediately see the gain.

[NoQ]

Ok there's actually a huge bug in this patch, namely we can't say "Assuming..." if there's no state split (i.e., when we know from the start which branch is taken so we don't have to assume). I'll fix.

In D122285#3401754, @steakhal wrote:

The notes are prunable, i.e. they won't bring-in entire stack frames worth of notes just because they're there, but they will be always visible regardless of whether the value is of interest to the bug report. I think this is debatable, the arguably better solution is to make them non-prunable but conditional to the value being tracked back to the call, which would probably need a better tracking infrastructure.

I was thinking of passing a lambda and doing the rest there. We could have lambda factories to make it less cumbersome to define - and also reuse code.

Yes sure, that's exactly how note tags are designed to work, but that lambda still needs to know whether the value is tracked. I guess I'll take a look at how well interestingness performs in these examples, maybe it's worth having from the start.

[NoQ]

• Eliminate notes on known paths.
• Add some documentation for the new class.
  • Mostly moved from existing documentation for the entire summary class.
  • Fix incorrect assessment that Exploded Graph is a tree. It's not even acyclic!
• Fix clang-format warnings.

[NoQ]

I think interestingness definitely requires more work. In particular, in null dereference from getenv(), the note should be unprunable. But we can't control prunability dynamically yet, so it requires a bit more work. So I think this patch is ok to go and I'll hopefully follow up with more interestingness logic.

[steakhal]

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

872–884

I thought we mostly have one or two cases. If we have two cases, then the constraint should be the opposite of the other one.

If this is the case, when does NewState && NewState != State not imply Node->succ_size() > 1 given that Summary.getCases().size() >= 2?

1326–1330

Why don't we have notes for these cases?

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

4

This is the default value. What's the benefit of passing this?

[balazske]

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

400

It would be good to test how this looks in doxygen output. If it is for generated documentation formatting tags can be used, otherwise this can be a // comment.

879

Can other checkers not add successor nodes (that make this check not always correct)?

879

Is there a reason against add the note without the word "Assuming" instead of no note?

[NoQ]

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

872–884

Typically this is going to be the case but I wouldn't rely on that. The constraint solver may get confused (inb4 "system is over constrained"), there may be other updates to the state in the branch definition which would cause the state to change.

879

Can other checkers not add successor nodes (that make this check not always correct)?

They'll be chained to the newly created node, not to the same predecessor. Two checkers adding transitions on the same post-call won't (and shouldn't) suddenly cause a state split.

Is there a reason against add the note without the word "Assuming" instead of no note?

We generally never have event notes in such cases.

We sometimes have control flow notes such as "aking true branch" (arrow pointing to the branch in plist).

Then, separately from that, we have tracking notes such as "an interesting value appeared in this variable from this assignment, which also makes the assigned value interesting" (eg. "null pointer value assigned to 'p'") - we could have a similar note "an interesting value was returned from that function because that other value had a certain range, which also makes that other value interesting" - which we should definitely consider.

But simply saying "this function call was completely predictable and we don't even care what the value is in the first place" doesn't sound useful.

879

*Taking

1326–1330

Like I mentioned in D122285#inline-1169194, I'm not sure these cases should exist at all. In particular, explaining them to the user is fairly problematic because justifying them in the first place is problematic.

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

4

I'm not sure what the default value should be. I guess I can remove this and/or move non-getenv tests into another file.

[NoQ]

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

400

It doesn't! Classes in .cpp files don't seem to show up in doxygen at all. So it doesn't matter which style do we use. I think I'll keep it doxygenized just in case we decide to move these classes out to a public header at some point.

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

 // side effets), for which their `Summary' is a precise model. This avoids
 // unnecessary invalidation passes. Conflicts with other checkers are unlikely
 // because if the function has no other effects, other checkers would probably
 // never want to improve upon the modeling done by this checker.
 //
 // Non-pure functions, for which only partial improvement over the default
 // behavior is expected, are modeled via check::PostCall, non-intrusively.
 //
-// The following standard C functions are currently supported:
-//
-//   fgetc      getline   isdigit   isupper     toascii
-//   fread      isalnum   isgraph   isxdigit
-//   fwrite     isalpha   islower   read
-//   getc       isascii   isprint   write
-//   getchar    isblank   ispunct   toupper
-//   getdelim   iscntrl   isspace   tolower
-//
 //===----------------------------------------------------------------------===//
 
 #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"
bool checkSpecificValidity(const FunctionDecl *FD) const override {
       const bool ValidArg = getArgType(FD, ArgN)->isPointerType();
       assert(ValidArg &&
              "This constraint should be applied only on a pointer type");
       return ValidArg;
     }
   };
 
   /// The complete list of constraints that defines a single branch.
-  typedef std::vector<ValueConstraintPtr> ConstraintSet;
+  using ConstraintSet = std::vector<ValueConstraintPtr>;
+
+  /// A single branch of a function summary.
+  ///
+  /// A branch is defined by a series of constraints - "assumptions" -
+  /// that together form a single possible outcome of invoking the function.
+  /// When static analyzer considers a branch, it tries to introduce
+  /// a child node in the Exploded Graph. The child node has to include
+  /// constraints that define the branch. If the constraints contradict
+  /// existing constraints in the state, the node is not created and the branch
+  /// is dropped; otherwise it's queued for future exploration.
+  /// The branch is accompanied by a note text that may be displayed
+  /// to the user when a bug is found on a path that takes this branch.
+  ///
+  /// 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])
+  ///     and the note may say "Assuming the character is alphabetical"
+  ///   Branch 2)
+  ///     x is out-of-range ['A', 'Z'] and out-of-range ['a', 'z']
+  ///     then the return value is 0
+  ///     and the note may say "Assuming the character is non-alphabetical".
+  class SummaryCase {
+    ConstraintSet Constraints;
+    StringRef Note;
+
+  public:
+    SummaryCase(ConstraintSet &&Constraints, StringRef Note)
+        : Constraints(std::move(Constraints)), Note(Note) {}
+
+    SummaryCase(const ConstraintSet &Constraints, StringRef Note)
+        : Constraints(Constraints), Note(Note) {}
+
+    const ConstraintSet &getConstraints() const { return Constraints; }
+    StringRef getNote() const { return Note; }
+  };
 
   using ArgTypes = std::vector<Optional<QualType>>;
   using RetType = Optional<QualType>;
 
   // A placeholder type, we use it whenever we do not care about the concrete
   // type in a Signature.
   const QualType Irrelevant{};
   bool static isIrrelevant(QualType T) { return T.isNull(); }
class StdLibraryFunctionsChecker
   static QualType getArgType(const FunctionDecl *FD, ArgNo ArgN) {
     assert(FD && "Function must be set");
     QualType T = (ArgN == Ret)
                      ? FD->getReturnType().getCanonicalType()
                      : FD->getParamDecl(ArgN)->getType().getCanonicalType();
     return T;
   }
 
-  using Cases = std::vector<ConstraintSet>;
+  using SummaryCases = std::vector<SummaryCase>;
 
   /// A summary includes information about
   ///   * function prototype (signature)
   ///   * 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 branches - so, a list of list of ranges,
   ///   * 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.
   ///
   /// Application of a summary:
   ///   The signature and argument constraints together contain information
   ///   about which functions are handled by the summary. The signature can use
   ///   "wildcards", i.e. Irrelevant types. Irrelevant type of a parameter in
   ///   a signature means that type is not compared to the type of the parameter
   ///   in the found FunctionDecl. Argument constraints may specify additional
   ///   rules for the given parameter's type, those rules are checked once the
   ///   signature is matched.
   class Summary {
     const InvalidationKind InvalidationKd;
-    Cases CaseConstraints;
+    SummaryCases Cases;
     ConstraintSet ArgConstraints;
 
     // The function to which the summary applies. This is set after lookup and
     // match to the signature.
     const FunctionDecl *FD = nullptr;
 
   public:
     Summary(InvalidationKind InvalidationKd) : InvalidationKd(InvalidationKd) {}
 
-    Summary &Case(ConstraintSet &&CS) {
-      CaseConstraints.push_back(std::move(CS));
+    Summary &Case(ConstraintSet &&CS, StringRef Note = "") {
+      Cases.push_back(SummaryCase(std::move(CS), Note));
       return *this;
     }
-    Summary &Case(const ConstraintSet &CS) {
-      CaseConstraints.push_back(CS);
+    Summary &Case(const ConstraintSet &CS, StringRef Note = "") {
+      Cases.push_back(SummaryCase(CS, Note));
       return *this;
     }
     Summary &ArgConstraint(ValueConstraintPtr VC) {
       assert(VC->getArgNo() != Ret &&
              "Arg constraint should not refer to the return value");
       ArgConstraints.push_back(VC);
       return *this;
     }
 
     InvalidationKind getInvalidationKd() const { return InvalidationKd; }
-    const Cases &getCaseConstraints() const { return CaseConstraints; }
+    const SummaryCases &getCases() const { return Cases; }
     const ConstraintSet &getArgConstraints() const { return ArgConstraints; }
 
     QualType getArgType(ArgNo ArgN) const {
       return StdLibraryFunctionsChecker::getArgType(FD, ArgN);
     }
 
     // Returns true if the summary should be applied to the given function.
     // And if yes then store the function declaration.
     bool matchesAndSet(const Signature &Sign, const FunctionDecl *FD) {
       bool Result = Sign.matches(FD) && validateByConstraints(FD);
       if (Result) {
         assert(!this->FD && "FD must not be set more than once");
         this->FD = FD;
       }
       return Result;
     }
 
   private:
     // Once we know the exact type of the function then do validation check on
     // all the given constraints.
     bool validateByConstraints(const FunctionDecl *FD) const {
-      for (const ConstraintSet &Case : CaseConstraints)
-        for (const ValueConstraintPtr &Constraint : Case)
+      for (const SummaryCase &Case : Cases)
+        for (const ValueConstraintPtr &Constraint : Case.getConstraints())
           if (!Constraint->checkValidity(FD))
             return false;
       for (const ValueConstraintPtr &Constraint : ArgConstraints)
         if (!Constraint->checkValidity(FD))
           return false;
       return true;
     }
   };
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();
+  const ExplodedNode *Node = C.getPredecessor();
 
   // Apply case/branch specifications.
-  for (const ConstraintSet &Case : Summary.getCaseConstraints()) {
+  for (const SummaryCase &Case : Summary.getCases()) {
     ProgramStateRef NewState = State;
-    for (const ValueConstraintPtr &Constraint : Case) {
+    for (const ValueConstraintPtr &Constraint : Case.getConstraints()) {
       NewState = Constraint->apply(NewState, Call, Summary, C);
       if (!NewState)
         break;
     }
 
-    if (NewState && NewState != State)
-      C.addTransition(NewState);
+    if (NewState && NewState != State) {
+      StringRef Note = Case.getNote();
+      const NoteTag *Tag = C.getNoteTag(
+          // Sorry couldn't help myself.
+          [Node, Note]() {
+            // Don't emit "Assuming..." note when we ended up
+            // knowing in advance which branch is taken.
+            return (Node->succ_size() > 1) ? Note.str() : "";
+          },
+          /*IsPrunable=*/true);
+      C.addTransition(NewState, Tag);
+    }
   }
 }
 
 bool StdLibraryFunctionsChecker::evalCall(const CallEvent &Call,
                                           CheckerContext &C) const {
   Optional<Summary> FoundSummary = findFunctionSummary(Call, C);
   if (!FoundSummary)
     return false;
 
void StdLibraryFunctionsChecker::initFunctionSummaries(
   // representable as unsigned char or is not equal to EOF. See e.g. C99
   // 7.4.1.2 The isalpha function (p: 181-182).
   addToFunctionSummaryMap(
       "isalnum", Signature(ArgTypes{IntTy}, RetType{IntTy}),
       Summary(EvalCallAsPure)
           // Boils down to isupper() or islower() or isdigit().
           .Case({ArgumentCondition(0U, WithinRange,
                                    {{'0', '9'}, {'A', 'Z'}, {'a', 'z'}}),
-                 ReturnValueCondition(OutOfRange, SingleValue(0))})
+                 ReturnValueCondition(OutOfRange, SingleValue(0))},
+                "Assuming the character is alphanumeric")
           // 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))},
+              "Assuming the character is non-alphanumeric")
           .ArgConstraint(ArgumentCondition(
               0U, WithinRange, {{EOFv, EOFv}, {0, UCharRangeMax}})));
   addToFunctionSummaryMap(
       "isalpha", Signature(ArgTypes{IntTy}, RetType{IntTy}),
       Summary(EvalCallAsPure)
           .Case({ArgumentCondition(0U, WithinRange, {{'A', 'Z'}, {'a', 'z'}}),
-                 ReturnValueCondition(OutOfRange, SingleValue(0))})
+                 ReturnValueCondition(OutOfRange, SingleValue(0))},
+                "Assuming the character is alphabetical")
           // The locale-specific range.
           .Case({ArgumentCondition(0U, WithinRange, {{128, UCharRangeMax}})})
           .Case({ArgumentCondition(
                      0U, OutOfRange,
                      {{'A', 'Z'}, {'a', 'z'}, {128, UCharRangeMax}}),
-                 ReturnValueCondition(WithinRange, SingleValue(0))}));
+                 ReturnValueCondition(WithinRange, SingleValue(0))},
+                "Assuming the character is non-alphabetical"));
   addToFunctionSummaryMap(
       "isascii", Signature(ArgTypes{IntTy}, RetType{IntTy}),
       Summary(EvalCallAsPure)
           .Case({ArgumentCondition(0U, WithinRange, Range(0, 127)),
-                 ReturnValueCondition(OutOfRange, SingleValue(0))})
+                 ReturnValueCondition(OutOfRange, SingleValue(0))},
+                "Assuming the character is an ASCII character")
           .Case({ArgumentCondition(0U, OutOfRange, Range(0, 127)),
-                 ReturnValueCondition(WithinRange, SingleValue(0))}));
+                 ReturnValueCondition(WithinRange, SingleValue(0))},
+                "Assuming the character is not an ASCII character"));
   addToFunctionSummaryMap(
       "isblank", Signature(ArgTypes{IntTy}, RetType{IntTy}),
       Summary(EvalCallAsPure)
           .Case({ArgumentCondition(0U, WithinRange, {{'\t', '\t'}, {' ', ' '}}),
-                 ReturnValueCondition(OutOfRange, SingleValue(0))})
+                 ReturnValueCondition(OutOfRange, SingleValue(0))},
+                "Assuming the character is a blank character")
           .Case({ArgumentCondition(0U, OutOfRange, {{'\t', '\t'}, {' ', ' '}}),
-                 ReturnValueCondition(WithinRange, SingleValue(0))}));
+                 ReturnValueCondition(WithinRange, SingleValue(0))},
+                "Assuming the character is not a blank character"));
   addToFunctionSummaryMap(
       "iscntrl", Signature(ArgTypes{IntTy}, RetType{IntTy}),
       Summary(EvalCallAsPure)
           .Case({ArgumentCondition(0U, WithinRange, {{0, 32}, {127, 127}}),
-                 ReturnValueCondition(OutOfRange, SingleValue(0))})
+                 ReturnValueCondition(OutOfRange, SingleValue(0))},
+                "Assuming the character is a control character")
           .Case({ArgumentCondition(0U, OutOfRange, {{0, 32}, {127, 127}}),
-                 ReturnValueCondition(WithinRange, SingleValue(0))}));
+                 ReturnValueCondition(WithinRange, SingleValue(0))},
+                "Assuming the character is not a control character"));
   addToFunctionSummaryMap(
       "isdigit", Signature(ArgTypes{IntTy}, RetType{IntTy}),
       Summary(EvalCallAsPure)
           .Case({ArgumentCondition(0U, WithinRange, Range('0', '9')),
-                 ReturnValueCondition(OutOfRange, SingleValue(0))})
+                 ReturnValueCondition(OutOfRange, SingleValue(0))},
+                "Assuming the character is a digit")
           .Case({ArgumentCondition(0U, OutOfRange, Range('0', '9')),
-                 ReturnValueCondition(WithinRange, SingleValue(0))}));
+                 ReturnValueCondition(WithinRange, SingleValue(0))},
+                "Assuming the character is not a digit"));
   addToFunctionSummaryMap(
       "isgraph", Signature(ArgTypes{IntTy}, RetType{IntTy}),
       Summary(EvalCallAsPure)
           .Case({ArgumentCondition(0U, WithinRange, Range(33, 126)),
-                 ReturnValueCondition(OutOfRange, SingleValue(0))})
-          .Case({ArgumentCondition(0U, OutOfRange, Range(33, 126)),
-                 ReturnValueCondition(WithinRange, SingleValue(0))}));
+                 ReturnValueCondition(OutOfRange, SingleValue(0))},
+                "Assuming the character has graphical representation")
+          .Case(
+              {ArgumentCondition(0U, OutOfRange, Range(33, 126)),
+               ReturnValueCondition(WithinRange, SingleValue(0))},
+              "Assuming the character does not have graphical representation"));
   addToFunctionSummaryMap(
       "islower", Signature(ArgTypes{IntTy}, RetType{IntTy}),
       Summary(EvalCallAsPure)
           // Is certainly lowercase.
           .Case({ArgumentCondition(0U, WithinRange, Range('a', 'z')),
-                 ReturnValueCondition(OutOfRange, SingleValue(0))})
+                 ReturnValueCondition(OutOfRange, SingleValue(0))},
+                "Assuming the character is a lowercase letter")
           // Is ascii but not lowercase.
           .Case({ArgumentCondition(0U, WithinRange, Range(0, 127)),
                  ArgumentCondition(0U, OutOfRange, Range('a', 'z')),
-                 ReturnValueCondition(WithinRange, SingleValue(0))})
+                 ReturnValueCondition(WithinRange, SingleValue(0))},
+                "Assuming the character is not a lowercase letter")
           // The locale-specific range.
           .Case({ArgumentCondition(0U, WithinRange, {{128, UCharRangeMax}})})
           // Is not an unsigned char.
           .Case({ArgumentCondition(0U, OutOfRange, Range(0, UCharRangeMax)),
                  ReturnValueCondition(WithinRange, SingleValue(0))}));
   addToFunctionSummaryMap(
       "isprint", Signature(ArgTypes{IntTy}, RetType{IntTy}),
       Summary(EvalCallAsPure)
           .Case({ArgumentCondition(0U, WithinRange, Range(32, 126)),
-                 ReturnValueCondition(OutOfRange, SingleValue(0))})
+                 ReturnValueCondition(OutOfRange, SingleValue(0))},
+                "Assuming the character is printable")
           .Case({ArgumentCondition(0U, OutOfRange, Range(32, 126)),
-                 ReturnValueCondition(WithinRange, SingleValue(0))}));
+                 ReturnValueCondition(WithinRange, SingleValue(0))},
+                "Assuming the character is non-printable"));
   addToFunctionSummaryMap(
       "ispunct", Signature(ArgTypes{IntTy}, RetType{IntTy}),
       Summary(EvalCallAsPure)
           .Case({ArgumentCondition(
                      0U, WithinRange,
                      {{'!', '/'}, {':', '@'}, {'[', '`'}, {'{', '~'}}),
-                 ReturnValueCondition(OutOfRange, SingleValue(0))})
+                 ReturnValueCondition(OutOfRange, SingleValue(0))},
+                "Assuming the character is a punctuation mark")
           .Case({ArgumentCondition(
                      0U, OutOfRange,
                      {{'!', '/'}, {':', '@'}, {'[', '`'}, {'{', '~'}}),
-                 ReturnValueCondition(WithinRange, SingleValue(0))}));
+                 ReturnValueCondition(WithinRange, SingleValue(0))},
+                "Assuming the character is not a punctuation mark"));
   addToFunctionSummaryMap(
       "isspace", Signature(ArgTypes{IntTy}, RetType{IntTy}),
       Summary(EvalCallAsPure)
           // Space, '\f', '\n', '\r', '\t', '\v'.
           .Case({ArgumentCondition(0U, WithinRange, {{9, 13}, {' ', ' '}}),
-                 ReturnValueCondition(OutOfRange, SingleValue(0))})
+                 ReturnValueCondition(OutOfRange, SingleValue(0))},
+                "Assuming the character is a whitespace character")
           // The locale-specific range.
           .Case({ArgumentCondition(0U, WithinRange, {{128, UCharRangeMax}})})
           .Case({ArgumentCondition(0U, OutOfRange,
                                    {{9, 13}, {' ', ' '}, {128, UCharRangeMax}}),
-                 ReturnValueCondition(WithinRange, SingleValue(0))}));
+                 ReturnValueCondition(WithinRange, SingleValue(0))},
+                "Assuming the character is not a whitespace character"));
   addToFunctionSummaryMap(
       "isupper", Signature(ArgTypes{IntTy}, RetType{IntTy}),
       Summary(EvalCallAsPure)
           // Is certainly uppercase.
           .Case({ArgumentCondition(0U, WithinRange, Range('A', 'Z')),
-                 ReturnValueCondition(OutOfRange, SingleValue(0))})
+                 ReturnValueCondition(OutOfRange, SingleValue(0))},
+                "Assuming the character is an uppercase letter")
           // The locale-specific range.
           .Case({ArgumentCondition(0U, WithinRange, {{128, UCharRangeMax}})})
           // Other.
           .Case({ArgumentCondition(0U, OutOfRange,
                                    {{'A', 'Z'}, {128, UCharRangeMax}}),
-                 ReturnValueCondition(WithinRange, SingleValue(0))}));
+                 ReturnValueCondition(WithinRange, SingleValue(0))},
+                "Assuming the character is not an uppercase letter"));
   addToFunctionSummaryMap(
       "isxdigit", Signature(ArgTypes{IntTy}, RetType{IntTy}),
       Summary(EvalCallAsPure)
           .Case({ArgumentCondition(0U, WithinRange,
                                    {{'0', '9'}, {'A', 'F'}, {'a', 'f'}}),
-                 ReturnValueCondition(OutOfRange, SingleValue(0))})
+                 ReturnValueCondition(OutOfRange, SingleValue(0))},
+                "Assuming the character is a hexadecimal digit")
           .Case({ArgumentCondition(0U, OutOfRange,
                                    {{'0', '9'}, {'A', 'F'}, {'a', 'f'}}),
-                 ReturnValueCondition(WithinRange, SingleValue(0))}));
+                 ReturnValueCondition(WithinRange, SingleValue(0))},
+                "Assuming the character is not a hexadecimal digit"));
   addToFunctionSummaryMap(
       "toupper", Signature(ArgTypes{IntTy}, RetType{IntTy}),
       Summary(EvalCallAsPure)
           .ArgConstraint(ArgumentCondition(
               0U, WithinRange, {{EOFv, EOFv}, {0, UCharRangeMax}})));
   addToFunctionSummaryMap(
       "tolower", Signature(ArgTypes{IntTy}, RetType{IntTy}),
       Summary(EvalCallAsPure)
void StdLibraryFunctionsChecker::initFunctionSummaries(
   addToFunctionSummaryMap(
       "getdelim",
       Signature(ArgTypes{CharPtrPtrRestrictTy, SizePtrRestrictTy, IntTy,
                          FilePtrRestrictTy},
                 RetType{Ssize_tTy}),
       GetLineSummary);
 
   {
-    Summary GetenvSummary = Summary(NoEvalCall)
-                                .ArgConstraint(NotNull(ArgNo(0)))
-                                .Case({NotNull(Ret)});
+    Summary GetenvSummary =
+        Summary(NoEvalCall)
+            .Case({NotNull(Ret)}, "Assuming the environment variable exists");
     // In untrusted environments the envvar might not exist.
     if (!ShouldAssumeControlledEnvironment)
-      GetenvSummary.Case({NotNull(Ret)->negate()});
+      GetenvSummary.Case({NotNull(Ret)->negate()},
+                         "Assuming the environment variable does not exist");
 
     // char *getenv(const char *name);
     addToFunctionSummaryMap(
         "getenv", Signature(ArgTypes{ConstCharPtrTy}, RetType{CharPtrTy}),
         std::move(GetenvSummary));
   }
 
   if (ModelPOSIX) {

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

if (Constraint.getAs<Loc>()) {
       os << (Assumption ? "non-null" : "null");
     }
 
     if (os.str().empty())
       return nullptr;
 
     // Construct a new PathDiagnosticPiece.
     ProgramPoint P = N->getLocation();
+
+    // If this node already have a specialized note, it's probably better
+    // than our generic note.
+    // FIXME: This only looks for note tags, not for other ways to add a note.
+    if (isa_and_nonnull<NoteTag>(P.getTag()))
+      return nullptr;
+
     PathDiagnosticLocation L =
       PathDiagnosticLocation::create(P, BRC.getSourceManager());
     if (!L.isValid())
       return nullptr;
 
     auto X = std::make_shared<PathDiagnosticEventPiece>(L, os.str());
     X->setTag(getTag());
     return std::move(X);

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

void test_alnum_concrete(int v) {
   // report-note{{}} \
   // bugpath-warning{{Function argument constraint is not satisfied}} \
   // bugpath-note{{}} \
   // bugpath-note{{Function argument constraint is not satisfied}}
   (void)ret;
 }
 
 void test_alnum_symbolic(int x) {
-  int ret = isalnum(x);
+  int ret = isalnum(x); // \
+  // bugpath-note{{Assuming the character is non-alphanumeric}}
   (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}}

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

+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:     -analyzer-checker=core,apiModeling \
+// RUN:     -analyzer-output=text
+
+#define NULL ((void *)0)
+
+char *getenv(const char *);
+int isalpha(int);
+int isdigit(int);
+int islower(int);
+
+char test_getenv() {
+  char *env = getenv("VAR"); // \
+  // expected-note{{Assuming the environment variable does not exist}} \
+  // expected-note{{'env' initialized here}}
+
+  return env[0]; // \
+  // expected-warning{{Array access (from variable 'env') results in a null pointer dereference}} \
+  // expected-note   {{Array access (from variable 'env') results in a null pointer dereference}}
+}
+
+int test_isalpha(int *x, char c) {
+  if (isalpha(c)) {// \
+    // expected-note{{Assuming the character is alphabetical}} \
+    // expected-note{{Taking true branch}}
+    x = NULL; // \
+    // expected-note{{Null pointer value stored to 'x'}}
+  }
+
+  return *x; // \
+  // expected-warning{{Dereference of null pointer (loaded from variable 'x')}} \
+  // expected-note   {{Dereference of null pointer (loaded from variable 'x')}}
+}
+
+int test_isdigit(int *x, char c) {
+  if (!isdigit(c)) {// \
+    // expected-note{{Assuming the character is not a digit}} \
+    // expected-note{{Taking true branch}}
+    x = NULL; // \
+    // expected-note{{Null pointer value stored to 'x'}}
+  }
+
+  return *x; // \
+  // expected-warning{{Dereference of null pointer (loaded from variable 'x')}} \
+  // expected-note   {{Dereference of null pointer (loaded from variable 'x')}}
+}
+
+int test_islower(int *x) {
+  char c = 'c';
+  // No "Assuming..." note. We aren't assuming anything. We *know*.
+  if (islower(c)) { // \
+    // expected-note{{Taking true branch}}
+    x = NULL; // \
+    // expected-note{{Null pointer value stored to 'x'}}
+  }
+
+  return *x; // \
+  // expected-warning{{Dereference of null pointer (loaded from variable 'x')}} \
+  // expected-note   {{Dereference of null pointer (loaded from variable 'x')}}
+}