Fix llvm-strings crash for negative char values

[jhenderson]

Summary

On Windows at least, llvm-strings was crashing if it encountered bytes that mapped to negative chars, as it was passing these into std::isgraph and std::isblank functions, resulting in undefined behaviour. On debug builds using MSVC, these functions verfiy that the value passed in is representable as an unsigned char. Since the char is promoted to an int, a value greater than 127 would turn into a negative integer value, and fail the check. Using the llvm::isPrint function is sufficient to solve the issue.

[ruiu]

include/llvm/ADT/StringRef.h

93–97

Not sure if this is a good thing to do. If you do, you probably should do the same for StringRef::StringRef(const char *Str) and other functions? But isn't that a bit too much?

[dexonsmith]

include/llvm/ADT/StringRef.h

93–97

If you do, you probably should do the same for StringRef::StringRef(const char *Str) and other functions?

IMO StringRef::StringRef(const char *Str) should only be used for string literals. Same with StringRef::withNullAsEmpty().

[efriedma]

tools/llvm-strings/llvm-strings.cpp

84

Could we just use something like if (isPrint(*P) || *P == '\t') to avoid the issue? std::isgraph has other issues (see r338034).

[jhenderson]

tools/llvm-strings/llvm-strings.cpp

84

Yes, I think we can, and that then solves the need for the new StringRef constructor. I'll update.

[jhenderson]

Remove unsigned char StringRef constructor, use llvm::isPrint, and improve test to show that the negative character is not printed.

[mstorsjo]

LGTM for what that's worth, but I'll leave it for those who commented earlier to do the final approval.

[ruiu]

LGTM

[jhenderson]

Add quotes to echo string in test. This fixes the test on Linux.

include/llvm/ADT/StringRef.h

public:
     /*implicit*/ StringRef(const char *Str)
         : Data(Str), Length(Str ? ::strlen(Str) : 0) {}
 
     /// Construct a string ref from a pointer and length.
     LLVM_ATTRIBUTE_ALWAYS_INLINE
     /*implicit*/ constexpr StringRef(const char *data, size_t length)
         : Data(data), Length(length) {}
 
+    /// Construct a string ref from a pointer to bytes and length.
+    LLVM_ATTRIBUTE_ALWAYS_INLINE
+    /*implicit*/ constexpr StringRef(const unsigned char *data, size_t length)
+        : Data(reinterpret_cast<const char *>(data)), Length(length) {}
+
     /// Construct a string ref from an std::string.
     LLVM_ATTRIBUTE_ALWAYS_INLINE
     /*implicit*/ StringRef(const std::string &Str)
       : Data(Str.data()), Length(Str.length()) {}
 
     static StringRef withNullAsEmpty(const char *data) {
       return StringRef(data ? data : "");
     }

test/tools/llvm-strings/negative.test

+# RUN: echo -e \x80 | llvm-strings -

tools/llvm-strings/llvm-strings.cpp

case hexadecimal:
       break;
     case decimal:
       OS << format("%8u", Offset);
       break;
     }
     OS << " " << L << '\n';
   };
 
-  const char *B = Contents.begin();
-  const char *P = nullptr, *E = nullptr, *S = nullptr;
-  for (P = Contents.begin(), E = Contents.end(); P < E; ++P) {
+  const unsigned char *B = Contents.bytes_begin();
+  const unsigned char *E = Contents.bytes_end();
+  const unsigned char *S = nullptr;
+  for (const unsigned char *P = B; P < E; ++P) {
     if (std::isgraph(*P) || std::isblank(*P)) {
       if (S == nullptr)
         S = P;
     } else if (S) {
       print(S - B, StringRef(S, P - S));
       S = nullptr;
     }
   }
   if (S)