[llvm][json] Fix UINT64 json parsing

[wallace]

Summary

https://reviews.llvm.org/D109347 added support for UINT64 json numeric
types. However, it seems that it didn't properly test uint64_t numbers
larger than the int64_t because the number parsing logic doesn't
have any special handling for these large numbers.

This diffs adds a handler for large numbers, and besides that, fixes the
parsing of signed types by checking for errno ERANGE, which is the
recommended way to check if parsing fails because of out of bounds
errors. Before this diff, strtoll was always returning a number within
the bounds of an int64_t and the bounds check it was doing was completely
superfluous.

As an interesting fact about the old implementation, when calling strtoll
with "18446744073709551615", the largest uint64_t, End was S.end(), even
though it didn't use all digits. Which means that this check can only be
used to identify if the numeric string is malformed or not.

This patch also adds additional tests for extreme cases.

[wallace]

add additional test

[djtodoro]

sounds reasonable to me

llvm/lib/Support/JSON.cpp

917

what is this?

llvm/unittests/Support/JSONTest.cpp

388

please add a full-stop

398

please add a full-stop

416

please add a full-stop

428

please add a full-stop

[wallace]

address comments

[clayborg]

LGTM, but I don't own the LLVM JSON parser, so someone else should give the final ok!

[dblaikie]

Sure, sounds OK.

llvm/lib/Support/JSON.cpp

bool Parser::parseNumber(char First, Value &Out) {
   // Read the number into a string. (Must be null-terminated for strto*).
   SmallString<24> S;
   S.push_back(First);
   while (isNumber(peek()))
     S.push_back(next());
   char *End;
   // Try first to parse as integer, and if so preserve full 64 bits.
   // strtoll returns long long >= 64 bits, so check it's in range too.
-  auto I = std::strtoll(S.c_str(), &End, 10);
-  if (End == S.end() && I >= std::numeric_limits<int64_t>::min() &&
-      I <= std::numeric_limits<int64_t>::max()) {
+  errno = 0;
+  int64_t I = std::strtoll(S.c_str(), &End, 10);
+  if (End == S.end() && errno != ERANGE) {
     Out = int64_t(I);
     return true;
   }
+  // strtroull has a special handling for negative numbers, but in this
+  // case we don't want to do that because negative numbers were already
+  // handled in the previous block.
+  if (First != '-') {
+    errno = 0;
+    uint64_t UI = std::strtoull(S.c_str(), &End, 10);
+    if (End == S.end() && errno != ERANGE) {
+      Out = UI;
+      return true;
+    }
+  }
   // If it's not an integer
   Out = std::strtod(S.c_str(), &End);
   return End == S.end() || parseError("Invalid JSON value (number?)");
 }
 
 bool Parser::parseString(std::string &Out) {
   // leading quote was already consumed.
   for (char C = next(); C != '"'; C = next()) {

 
 void llvm::format_provider<llvm::json::Value>::format(
     const llvm::json::Value &E, raw_ostream &OS, StringRef Options) {
   unsigned IndentAmount = 0;
   if (!Options.empty() && Options.getAsInteger(/*Radix=*/10, IndentAmount))
     llvm_unreachable("json::Value format options should be an integer");
   json::OStream(OS, IndentAmount).value(E);
 }
-

llvm/unittests/Support/JSONTest.cpp

 }
 
 // Verify uint64_t type.
 TEST(JSONTest, U64Integers) {
   Value Val = uint64_t{3100100100};
   uint64_t Var = 3100100100;
   EXPECT_EQ(Val, Var);
 
+  Val = uint64_t{std::numeric_limits<uint64_t>::max()};
+  Var = std::numeric_limits<uint64_t>::max();
+  EXPECT_EQ(Val, Var);
+
   // Test the parse() part.
+  {
     const char *Str = "4611686018427387905";
     llvm::Expected<Value> Doc = parse(Str);
 
     EXPECT_TRUE(!!Doc);
     EXPECT_EQ(Doc->getAsInteger(), int64_t{4611686018427387905});
     EXPECT_EQ(Doc->getAsUINT64(), uint64_t{4611686018427387905});
+  }
 
-  const char *Str2 = "-78278238238328222";
-  llvm::Expected<Value> Doc2 = parse(Str2);
-  EXPECT_TRUE(!!Doc2);
-  EXPECT_EQ(Doc2->getAsInteger(), int64_t{-78278238238328222});
-  EXPECT_EQ(Doc2->getAsUINT64(), llvm::None);
+  {
+    const char *Str = "-78278238238328222";
+    llvm::Expected<Value> Doc = parse(Str);
+
+    EXPECT_TRUE(!!Doc);
+    EXPECT_EQ(Doc->getAsInteger(), int64_t{-78278238238328222});
+    EXPECT_EQ(Doc->getAsUINT64(), llvm::None);
+  }
+
+  // Test with the largest 64 signed int
+  {
+    const char *Str = "9223372036854775807";
+    llvm::Expected<Value> Doc = parse(Str);
+
+    EXPECT_TRUE(!!Doc);
+    EXPECT_EQ(Doc->getAsInteger(), int64_t{9223372036854775807});
+    EXPECT_EQ(Doc->getAsUINT64(), uint64_t{9223372036854775807});
+  }
+
+  // Test with the largest 64 unsigned int
+  {
+    const char *Str = "18446744073709551615";
+    llvm::Expected<Value> Doc = parse(Str);
+
+    EXPECT_TRUE(!!Doc);
+    EXPECT_EQ(Doc->getAsInteger(), None);
+    EXPECT_EQ(Doc->getAsUINT64(), uint64_t{18446744073709551615u});
+  }
+
+  // Test with a number that is too big for 64 bits.
+  {
+    const char *Str = "184467440737095516150";
+    llvm::Expected<Value> Doc = parse(Str);
+
+    EXPECT_TRUE(!!Doc);
+    EXPECT_EQ(Doc->getAsInteger(), None);
+    EXPECT_EQ(Doc->getAsUINT64(), None);
+    // The number was parsed as a double
+    EXPECT_TRUE(!!Doc->getAsNumber());
+  }
+
+  // Test with a negative number that is too small for 64 bits.
+  {
+    const char *Str = "-18446744073709551615";
+    llvm::Expected<Value> Doc = parse(Str);
+
+    EXPECT_TRUE(!!Doc);
+    EXPECT_EQ(Doc->getAsInteger(), None);
+    EXPECT_EQ(Doc->getAsUINT64(), None);
+    // The number was parsed as a double
+    EXPECT_TRUE(!!Doc->getAsNumber());
+  }
 }
 
 // Sample struct with typical JSON-mapping rules.
 struct CustomStruct {
   CustomStruct() : B(false) {}
   CustomStruct(std::string S, llvm::Optional<int> I, bool B)
       : S(S), I(I), B(B) {}
   std::string S;