[PATCH] D129224: [InstCombine] Fold strtoul and strtoull and avoid PR #56293

[Martin Sebor via Phabricator via llvm-commits]

msebor added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:99
+    // Fail for empty subject sequences (POSIX allows but doesn't require
+    // strtol[l]/strtoul[l] to fail with EINVAL).
+    return nullptr;
----------------
efriedma wrote:
> I think the check for the empty string should come after the check for a +/- sign?
Sure.  I don't have access to Darwin but based on the dated Apple `strtol.c` source I found online it looks like it also sets `errno` to `EINVAL` for calls like `strtol("+", 0, 0)` and also `strtol("0x", 0, 0)` that Glibc succeeds for so I adjusted the code to fail for those sequences as well.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:117
+      if (Str[0] == '0') {
+        if (toUpper((unsigned char)Str[1]) == 'X') {
+          Base = 16;
----------------
efriedma wrote:
> The standard requires that a "0x" prefix be followed by a hexadecimal digit.  Similarly, the standard requires that a "0" prefix be followed by an octal digit.  If there isn't an appropriate digit after, the subject sequence (the part that's actually parsed as a number) is just a decimal "0".  And the rest is treated as "unrecognized characters".
> 
> The standard allows a "0x" prefix if the base is explicitly specified to be 16.
Whoops, yes, we need to consume the hex prefix even in base 16.  Thanks for pointing out this mistake!

I'm aware of what the C and POSIX standards require but I'm not sure I understand the first part of your comment, but just for clarity: 1) a lone zero is valid in any base (i.e., it doesn't need to be followed by other digits), and 2) the function fails if the whole subject sequence isn't consumed (i.e., has "unrecognized characters").


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129224/new/

https://reviews.llvm.org/D129224