Skip to content

Conversation

saturn691
Copy link
Contributor

Remove strtoll functions for testing i128/u128.

Under AArch64, long long is defined as i64. When running the test integral.pass.cpp, it calls tries to call strtoll, which returns an i64, not an i128, therefore only keeping the lower 64 bits.

Remove `strtoll` functions for testing i128/u128.
Under AArch64, long long is defined as i64. When running the test
integral.pass.cpp, it calls tries to call `strtoll`, which returns
an i64, not an i128, therefore only keeping the lower 64 bits.
@saturn691 saturn691 requested a review from a team as a code owner April 11, 2025 12:26
Copy link

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Apr 11, 2025
@llvmbot
Copy link
Member

llvmbot commented Apr 11, 2025

@llvm/pr-subscribers-libcxx

Author: William (saturn691)

Changes

Remove strtoll functions for testing i128/u128.

Under AArch64, long long is defined as i64. When running the test integral.pass.cpp, it calls tries to call strtoll, which returns an i64, not an i128, therefore only keeping the lower 64 bits.


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

1 Files Affected:

  • (modified) libcxx/test/support/charconv_test_helpers.h (-19)
diff --git a/libcxx/test/support/charconv_test_helpers.h b/libcxx/test/support/charconv_test_helpers.h
index fcae09478457b..89927df02e56d 100644
--- a/libcxx/test/support/charconv_test_helpers.h
+++ b/libcxx/test/support/charconv_test_helpers.h
@@ -19,7 +19,6 @@
 #include <string.h>
 #include <stdlib.h>
 #include <type_traits>
-
 #include "test_macros.h"
 
 #if TEST_STD_VER < 11
@@ -163,15 +162,6 @@ struct to_chars_test_base
 #ifndef TEST_HAS_NO_INT128
     static TEST_CONSTEXPR_CXX23 __int128_t fromchars128_impl(char const* p, char const* ep, int base, true_type)
     {
-        if (!TEST_IS_CONSTANT_EVALUATED) {
-            char* last;
-            __int128_t r = strtoll(p, &last, base);
-            if(errno != ERANGE) {
-                assert(last == ep);
-                return r;
-            }
-        }
-
         // When the value doesn't fit in a long long use from_chars. This is
         // not ideal since it does a round-trip test instead if using an
         // external source.
@@ -185,15 +175,6 @@ struct to_chars_test_base
 
     static TEST_CONSTEXPR_CXX23 __uint128_t fromchars128_impl(char const* p, char const* ep, int base, false_type)
     {
-        if (!TEST_IS_CONSTANT_EVALUATED) {
-            char* last;
-            __uint128_t r = strtoull(p, &last, base);
-            if(errno != ERANGE) {
-                assert(last == ep);
-                return r;
-            }
-        }
-
         __uint128_t r;
         std::from_chars_result s = std::from_chars(p, ep, r, base);
         assert(s.ec == std::errc{});

@cpplearner
Copy link
Contributor

When running the test integral.pass.cpp, it calls tries to call strtoll, which returns an i64, not an i128, therefore only keeping the lower 64 bits.

If the correct result would be larger than LLONG_MAX, strtoll should set errno to ERANGE, and fromchars128_impl would then proceed to use from_chars instead.

Could you please elaborate on the problem that you want to fix?

@philnik777 philnik777 requested a review from mordante April 11, 2025 13:18
@saturn691
Copy link
Contributor Author

This is not the case when tested with picolibc under AArch64.
I'm trying to fix the failed test case integral.pass.cpp which relies on the function fromchars128_impl. Specifically, when tested with std::numeric_limits<__int128_t>::lowest(), it fails.

@philnik777
Copy link
Contributor

@saturn691 That sounds more like a picolibc bug than a libc++ one.

@saturn691
Copy link
Contributor Author

saturn691 commented Apr 11, 2025

@philnik777 I've double checked right now, errno = ERANGE = 34 under picolibc. It may be another issue

@saturn691
Copy link
Contributor Author

Closing, as this is a bug in picolibc after all.

@saturn691 saturn691 closed this Apr 11, 2025
@saturn691 saturn691 deleted the libcxx-fix-integral-pass branch April 12, 2025 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants