[analyzer] Limit UCharMax to min of max uchar or max int

[vabridgers]

Summary

This change is a follow up to commit 536456a7e93d73b9ff4e92f3e51d1aa1c72628fe
and limits UCharMax to the min of the maximum values for the
architecture's maximum unsigned char or maximum int values. This is
required for architectures where these values are different than the
most commonly supported architectures.

[Charusso]

Could you add a test please? We really need tests for every patch.

[martong]

Looks good to me! Thanks!

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

514

Perhaps it would be more precise to refer to the argument of these functions. So The C standard states that the arguments of functions like isalpha must be representable ...

517

Could you please spell out ISA? Maybe it is just me but I don't know what that stands for.

[vabridgers]

Address review comments

[vabridgers]

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

514

Will do.

517

Yes, will do.

[vabridgers]

@Charusso, I cannot find a way to create a test for this change, since this was found using an architecture that supports 16-bit chars. I understand the need to create test cases for changes. In this case, I believe a rationale for accepting this change is to conform to the C Standard, and accept that upstream clang does not have a way to test for architectures supporting 16-bit characters, or otherwise "peculiar" datatypes.

Please do indicate if you're willing to accept this patch or not. If not, I need to know so I solve this problem for my customer in a different way.

Thank you - Vince

[Charusso]

In D75529#1912849, @vabridgers wrote:

@Charusso, I cannot find a way to create a test for this change, since this was found using an architecture that supports 16-bit chars.

I really felt that we could write a test that using a target which behaves like that. I am not that awesome with triples, sorry. If I am right we are good to go without @NoQ's response (he is the maintainer of the Analyzer). Thanks!

[NoQ]

Aha, ok, i appreciate making life easier for downstream users.

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

519

Let's rename this constant then, so that we still had our UCharMax when we actually need the real UCHAR_MAX in the summary.

[Szelethus]

I have nothing else to add :)

[vabridgers]

Address Artem's comments.

[vabridgers]

fix pre merge checkswq

[vabridgers]

I believe all comments have been addressed. Please let me know if there's anything else required. Thanks

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

519

Done, thanks Artem.

[Charusso]

In D75529#1920796, @vabridgers wrote:

I believe all comments have been addressed. Please let me know if there's anything else required. Thanks

I think you have solved everything. Do you have commit access? May you would request it: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

[vabridgers]

@Charusso -- I do not have commit access, but I will request. Thank you.

[NoQ]

In D75529#1921959, @vabridgers wrote:

@Charusso -- I do not have commit access, but I will request. Thank you.

Whoops, i accidentally landed this patch because i misread your statement as if you're asking to land the patch for you :) Please request commit access anyway :)

clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

void StdLibraryFunctionsChecker::initFunctionSummaries(
   const QualType LongTy = ACtx.LongTy;
   const QualType LongLongTy = ACtx.LongLongTy;
   const QualType SizeTy = ACtx.getSizeType();
 
   const RangeInt IntMax = BVF.getMaxValue(IntTy).getLimitedValue();
   const RangeInt LongMax = BVF.getMaxValue(LongTy).getLimitedValue();
   const RangeInt LongLongMax = BVF.getMaxValue(LongLongTy).getLimitedValue();
 
-  const RangeInt UCharMax =
-      BVF.getMaxValue(ACtx.UnsignedCharTy).getLimitedValue();
+  // Set UCharRangeMax to min of int or uchar maximum value.
+  // The C standard states that the arguments of functions like isalpha must
+  // be representable as an unsigned char. Their type is 'int', so the max
+  // value of the argument should be min(UCharMax, IntMax). This just happen
+  // to be true for commonly used and well tested instruction set
+  // architectures, but not for others.
+  const RangeInt UCharRangeMax =
+      std::min(BVF.getMaxValue(ACtx.UnsignedCharTy).getLimitedValue(), IntMax);
 
   // The platform dependent value of EOF.
   // Try our best to parse this from the Preprocessor, otherwise fallback to -1.
   const auto EOFv = [&C]() -> RangeInt {
     if (const llvm::Optional<int> OptInt =
             tryExpandAsInteger("EOF", C.getPreprocessor()))
       return *OptInt;
     return -1;
void StdLibraryFunctionsChecker::initFunctionSummaries(
   };
   auto IsLessThan = [](ArgNo ArgN) { return IntRangeVector{{BO_LE, ArgN}}; };
 
   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, UCharMax}})});
+        .Case({ReturnValueCondition(WithinRange,
+                                    {{EOFv, EOFv}, {0, UCharRangeMax}})});
   };
   auto Read = [&](RetType R, RangeInt Max) {
     return Summary(ArgTypes{Irrelevant, Irrelevant, SizeTy}, RetType{R},
                    NoEvalCall)
         .Case({ReturnValueCondition(ComparesToArgument, IsLessThan(2)),
                ReturnValueCondition(WithinRange, Range(-1, Max))});
   };
   auto Fread = [&]() {
FunctionSummaryMap = {
                   // 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, UCharMax}})})
+                  .Case({ArgumentCondition(0U, WithinRange,
+                                           {{128, UCharRangeMax}})})
                   .Case({ArgumentCondition(0U, OutOfRange,
                                            {{'0', '9'},
                                             {'A', 'Z'},
                                             {'a', 'z'},
-                                            {128, UCharMax}}),
+                                            {128, UCharRangeMax}}),
                          ReturnValueCondition(WithinRange, SingleValue(0))})},
       },
       {
           "isalpha",
           Summaries{
               Summary(ArgTypes{IntTy}, RetType{IntTy}, EvalCallAsPure)
                   .Case({ArgumentCondition(0U, WithinRange,
                                            {{'A', 'Z'}, {'a', 'z'}}),
                          ReturnValueCondition(OutOfRange, SingleValue(0))})
                   // The locale-specific range.
-                  .Case({ArgumentCondition(0U, WithinRange, {{128, UCharMax}})})
+                  .Case({ArgumentCondition(0U, WithinRange,
+                                           {{128, UCharRangeMax}})})
                   .Case({ArgumentCondition(
                              0U, OutOfRange,
-                             {{'A', 'Z'}, {'a', 'z'}, {128, UCharMax}}),
+                             {{'A', 'Z'}, {'a', 'z'}, {128, UCharRangeMax}}),
                          ReturnValueCondition(WithinRange, SingleValue(0))})},
       },
       {
           "isascii",
           Summaries{
               Summary(ArgTypes{IntTy}, RetType{IntTy}, EvalCallAsPure)
                   .Case({ArgumentCondition(0U, WithinRange, Range(0, 127)),
                          ReturnValueCondition(OutOfRange, SingleValue(0))})
FunctionSummaryMap = {
                   // Is certainly lowercase.
                   .Case({ArgumentCondition(0U, WithinRange, Range('a', 'z')),
                          ReturnValueCondition(OutOfRange, SingleValue(0))})
                   // Is ascii but not lowercase.
                   .Case({ArgumentCondition(0U, WithinRange, Range(0, 127)),
                          ArgumentCondition(0U, OutOfRange, Range('a', 'z')),
                          ReturnValueCondition(WithinRange, SingleValue(0))})
                   // The locale-specific range.
-                  .Case({ArgumentCondition(0U, WithinRange, {{128, UCharMax}})})
+                  .Case({ArgumentCondition(0U, WithinRange,
+                                           {{128, UCharRangeMax}})})
                   // Is not an unsigned char.
-                  .Case({ArgumentCondition(0U, OutOfRange, Range(0, UCharMax)),
+                  .Case({ArgumentCondition(0U, OutOfRange,
+                                           Range(0, UCharRangeMax)),
                          ReturnValueCondition(WithinRange, SingleValue(0))})},
       },
       {
           "isprint",
           Summaries{
               Summary(ArgTypes{IntTy}, RetType{IntTy}, EvalCallAsPure)
                   .Case({ArgumentCondition(0U, WithinRange, Range(32, 126)),
                          ReturnValueCondition(OutOfRange, SingleValue(0))})
FunctionSummaryMap = {
           "isspace",
           Summaries{
               Summary(ArgTypes{IntTy}, RetType{IntTy}, EvalCallAsPure)
                   // Space, '\f', '\n', '\r', '\t', '\v'.
                   .Case({ArgumentCondition(0U, WithinRange,
                                            {{9, 13}, {' ', ' '}}),
                          ReturnValueCondition(OutOfRange, SingleValue(0))})
                   // The locale-specific range.
-                  .Case({ArgumentCondition(0U, WithinRange, {{128, UCharMax}})})
+                  .Case({ArgumentCondition(0U, WithinRange,
+                                           {{128, UCharRangeMax}})})
                   .Case({ArgumentCondition(
                              0U, OutOfRange,
-                             {{9, 13}, {' ', ' '}, {128, UCharMax}}),
+                             {{9, 13}, {' ', ' '}, {128, UCharRangeMax}}),
                          ReturnValueCondition(WithinRange, SingleValue(0))})},
       },
       {
           "isupper",
           Summaries{
               Summary(ArgTypes{IntTy}, RetType{IntTy}, EvalCallAsPure)
                   // Is certainly uppercase.
                   .Case({ArgumentCondition(0U, WithinRange, Range('A', 'Z')),
                          ReturnValueCondition(OutOfRange, SingleValue(0))})
                   // The locale-specific range.
-                  .Case({ArgumentCondition(0U, WithinRange, {{128, UCharMax}})})
+                  .Case({ArgumentCondition(0U, WithinRange,
+                                           {{128, UCharRangeMax}})})
                   // Other.
                   .Case({ArgumentCondition(0U, OutOfRange,
-                                           {{'A', 'Z'}, {128, UCharMax}}),
+                                           {{'A', 'Z'}, {128, UCharRangeMax}}),
                          ReturnValueCondition(WithinRange, SingleValue(0))})},
       },
       {
           "isxdigit",
           Summaries{
               Summary(ArgTypes{IntTy}, RetType{IntTy}, EvalCallAsPure)
                   .Case(
                       {ArgumentCondition(0U, WithinRange,
                                          {{'0', '9'}, {'A', 'F'}, {'a', 'f'}}),
                        ReturnValueCondition(OutOfRange, SingleValue(0))})
                   .Case(
                       {ArgumentCondition(0U, OutOfRange,
                                          {{'0', '9'}, {'A', 'F'}, {'a', 'f'}}),
                        ReturnValueCondition(WithinRange, SingleValue(0))})},
       },
 
       // The getc() family of functions that returns either a char or an EOF.
       {"getc", Summaries{Getc()}},
       {"fgetc", Summaries{Getc()}},
       {"getchar",
        Summaries{Summary(ArgTypes{}, RetType{IntTy}, NoEvalCall)
                      .Case({ReturnValueCondition(
-                         WithinRange, {{EOFv, EOFv}, {0, UCharMax}})})}},
+                         WithinRange, {{EOFv, EOFv}, {0, UCharRangeMax}})})}},
 
       // read()-like functions that never return more than buffer size.
       // We are not sure how ssize_t is defined on every platform, so we
       // provide three variants that should cover common cases.
       {"read", Summaries{Read(IntTy, IntMax), Read(LongTy, LongMax),
                          Read(LongLongTy, LongLongMax)}},
       {"write", Summaries{Read(IntTy, IntMax), Read(LongTy, LongMax),
                           Read(LongLongTy, LongLongMax)}},