Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[libc] Make strtointeger handle all bigint types #111926

Merged

Conversation

michaelrj-google
Copy link
Contributor

Needed for #110894

The strtointeger code was updated to support some bigint types in #85201
but not all. This patch finishes the cleanup so that it can work for a
wider range of types.

Needed for llvm#110894

The strtointeger code was updated to support some bigint types in llvm#85201
but not all. This patch finishes the cleanup so that it can work for a
wider range of types.
@llvmbot
Copy link
Member

llvmbot commented Oct 10, 2024

@llvm/pr-subscribers-libc

Author: Michael Jones (michaelrj-google)

Changes

Needed for #110894

The strtointeger code was updated to support some bigint types in #85201
but not all. This patch finishes the cleanup so that it can work for a
wider range of types.


Full diff: https://github.com/llvm/llvm-project/pull/111926.diff

1 Files Affected:

  • (modified) libc/src/__support/str_to_integer.h (+6-11)
diff --git a/libc/src/__support/str_to_integer.h b/libc/src/__support/str_to_integer.h
index 1f80229a9eff40..ea17178c6afb6a 100644
--- a/libc/src/__support/str_to_integer.h
+++ b/libc/src/__support/str_to_integer.h
@@ -11,6 +11,8 @@
 
 #include "src/__support/CPP/limits.h"
 #include "src/__support/CPP/type_traits.h"
+#include "src/__support/CPP/type_traits/make_unsigned.h"
+#include "src/__support/big_int.h"
 #include "src/__support/common.h"
 #include "src/__support/ctype_utils.h"
 #include "src/__support/macros/config.h"
@@ -77,9 +79,7 @@ template <class T>
 LIBC_INLINE StrToNumResult<T>
 strtointeger(const char *__restrict src, int base,
              const size_t src_len = cpp::numeric_limits<size_t>::max()) {
-  using ResultType = typename cpp::conditional_t<(cpp::is_same_v<T, UInt128> ||
-                                                  cpp::is_same_v<T, Int128>),
-                                                 UInt128, unsigned long long>;
+  using ResultType = make_integral_or_big_int_unsigned_t<T>;
 
   ResultType result = 0;
 
@@ -137,13 +137,13 @@ strtointeger(const char *__restrict src, int base,
       result = abs_max;
       error_val = ERANGE;
     } else {
-      result = result * base;
+      result = static_cast<ResultType>(result * base);
     }
     if (result > abs_max - cur_digit) {
       result = abs_max;
       error_val = ERANGE;
     } else {
-      result = result + cur_digit;
+      result = static_cast<ResultType>(result + cur_digit);
     }
   }
 
@@ -156,12 +156,7 @@ strtointeger(const char *__restrict src, int base,
       return {cpp::numeric_limits<T>::min(), str_len, error_val};
   }
 
-  return {
-      is_positive
-          ? static_cast<T>(result)
-          : static_cast<T>(
-                -static_cast<make_integral_or_big_int_unsigned_t<T>>(result)),
-      str_len, error_val};
+  return {static_cast<T>(is_positive ? result : -result), str_len, error_val};
 }
 
 } // namespace internal

@michaelrj-google michaelrj-google merged commit 6370546 into llvm:main Oct 11, 2024
9 checks passed
@michaelrj-google michaelrj-google deleted the libcStrToIntegerTypeFix branch October 11, 2024 23:15
@llvm-ci
Copy link
Collaborator

llvm-ci commented Oct 11, 2024

LLVM Buildbot has detected a new failure on builder openmp-offload-libc-amdgpu-runtime running on omp-vega20-1 while building libc at step 10 "Add check check-offload".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/73/builds/6959

Here is the relevant piece of the build log for the reference
Step 10 (Add check check-offload) failure: 1200 seconds without output running [b'ninja', b'-j 32', b'check-offload'], attempting to kill
...
PASS: libomptarget :: x86_64-unknown-linux-gnu-LTO :: offloading/bug50022.cpp (866 of 879)
PASS: libomptarget :: x86_64-unknown-linux-gnu-LTO :: offloading/bug53727.cpp (867 of 879)
PASS: libomptarget :: x86_64-unknown-linux-gnu-LTO :: offloading/bug47654.cpp (868 of 879)
PASS: libomptarget :: x86_64-unknown-linux-gnu-LTO :: offloading/test_libc.cpp (869 of 879)
PASS: libomptarget :: x86_64-unknown-linux-gnu-LTO :: offloading/wtime.c (870 of 879)
PASS: libomptarget :: x86_64-unknown-linux-gnu :: offloading/bug49021.cpp (871 of 879)
PASS: libomptarget :: x86_64-unknown-linux-gnu :: offloading/std_complex_arithmetic.cpp (872 of 879)
PASS: libomptarget :: x86_64-unknown-linux-gnu-LTO :: offloading/complex_reduction.cpp (873 of 879)
PASS: libomptarget :: x86_64-unknown-linux-gnu-LTO :: offloading/bug49021.cpp (874 of 879)
PASS: libomptarget :: x86_64-unknown-linux-gnu-LTO :: offloading/std_complex_arithmetic.cpp (875 of 879)
command timed out: 1200 seconds without output running [b'ninja', b'-j 32', b'check-offload'], attempting to kill
process killed by signal 9
program finished with exit code -1
elapsedTime=1237.276704

@nickdesaulniers
Copy link
Member

Ah, thanks! make_integral_or_big_int_unsigned_t in the using statement looks nicer than what I had locally (nested ternaries 🤮 )!

DanielCChen pushed a commit to DanielCChen/llvm-project that referenced this pull request Oct 16, 2024
Needed for llvm#110894

The strtointeger code was updated to support some bigint types in llvm#85201
but not all. This patch finishes the cleanup so that it can work for a
wider range of types.
bricknerb pushed a commit to bricknerb/llvm-project that referenced this pull request Oct 17, 2024
Needed for llvm#110894

The strtointeger code was updated to support some bigint types in llvm#85201
but not all. This patch finishes the cleanup so that it can work for a
wider range of types.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants