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 ↗

(On Diff #170443)

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 ↗

(On Diff #170443)

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 ↗

(On Diff #170443)

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 ↗

(On Diff #170443)

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.

llvm/trunk/test/tools/llvm-strings/negative-char.test

+# RUN: echo -e "z\0\x80\0a\0" | llvm-strings --bytes 1 - | FileCheck %s
+# CHECK: z{{$}}
+# CHECK-NEXT: {{^}} a

llvm/trunk/tools/llvm-strings/llvm-strings.cpp

case decimal:
       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) {
-    if (std::isgraph(*P) || std::isblank(*P)) {
+    if (isPrint(*P) || *P == '\t') {
       if (S == nullptr)
         S = P;
     } else if (S) {
       print(S - B, StringRef(S, P - S));
       S = nullptr;
     }
   }
   if (S)