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

Workaround "error: ‘(9.223372036854775807e+18 / 1.0e+9)’ is not a constant expression" #39044

Closed
noloader mannequin opened this issue Nov 17, 2018 · 44 comments
Closed

Workaround "error: ‘(9.223372036854775807e+18 / 1.0e+9)’ is not a constant expression" #39044

noloader mannequin opened this issue Nov 17, 2018 · 44 comments
Labels
bugzilla build-problem libc++

Comments

@noloader
Copy link
Mannequin

@noloader noloader mannequin commented Nov 17, 2018

Bugzilla Link 39696
Resolution FIXED
Resolved on Jun 10, 2020 10:36
Version 7.0
OS Linux
CC @ikitayama,@jdenny-ornl,@kencu,@mclow,@noloader

Extended Description

I'm trying to build LLVM and components on GCC112 from the compiler farm. GCC112 is ppc64-le, and ships with GCC 4.8.5:

$ gcc --version
gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-16)
Copyright (C) 2015 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

LLVM configuration and build goes well for a while. Then it encounters https://bugzilla.redhat.com/show_bug.cgi?id=1538817:

[ 35%] Building CXX object projects/libcxx/lib/CMakeFiles/cxx_objects.dir//src/condition_variable.cpp.o
cd /home/noloader/llvm_build/projects/libcxx/lib && /usr/bin/c++ -DNDEBUG -D_DEBUG -D_GNU_SOURCE -D_LIBCPP_BUILDING_LIBRARY -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/home/noloader/llvm_build/projects/libcxx/lib -I/home/noloader/llvm_source/llvm/projects/libcxx/lib -I/home/noloader/llvm_build/include -I/home/noloader/llvm_source/llvm/include -I/home/noloader/llvm_build/projects/libcxx/include/c++build -I/home/noloader/llvm_source/llvm/projects/libcxx/include -fPIC -fvisibility-inlines-hidden -std=c++11 -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -Wno-long-long -Wno-maybe-uninitialized -Wdelete-non-virtual-dtor -Wno-comment -g -DLIBCXX_BUILDING_LIBCXXABI -std=c++11 -nostdinc++ -fvisibility-inlines-hidden -Wall -Wextra -W -Wwrite-strings -Wno-unused-parameter -Wno-long-long -Werror=return-type -Wno-literal-suffix -Wno-c++14-compat -Wno-noexcept-type -Wno-error -fPIC -o CMakeFiles/cxx_objects.dir/
/src/condition_variable.cpp.o -c /home/noloader/llvm_source/llvm/projects/libcxx/src/condition_variable.cpp
In file included from /home/noloader/llvm_source/llvm/projects/libcxx/src/condition_variable.cpp:15:0:
/home/noloader/llvm_source/llvm/projects/libcxx/include/thread: In function ‘void std::__1::this_thread::sleep_for(const std::__1::chrono::duration<_Rep, _Period>&)’:
/home/noloader/llvm_source/llvm/projects/libcxx/include/thread:438:73: in constexpr expansion of ‘std::__1::chrono::duration(((const std::__1::chrono::duration<long long int, std::__1::ratio<1l, 1000000000l> >)(& std::__1::chrono::duration<_Rep, _Period>::max<long long int, std::__1::ratio<1l, 1000000000l> >())), 0u)’
/home/noloader/llvm_source/llvm/projects/libcxx/include/chrono:1055:68: in constexpr expansion of ‘std::__1::chrono::duration_cast<std::__1::chrono::duration, long long int, std::__1::ratio<1l, 1000000000l> >((* & __d))’
/home/noloader/llvm_source/llvm/projects/libcxx/include/chrono:907:72: in constexpr expansion of ‘std::__1::chrono::__duration_cast<std::__1::chrono::duration<long long int, std::__1::ratio<1l, 1000000000l> >, std::__1::chrono::duration, std::__1::ratio<1l, 1000000000l>, true, false>().std::__1::chrono::__duration_cast<_FromDuration, _ToDuration, _Period, true, false>::operator()<std::__1::chrono::duration<long long int, std::__1::ratio<1l, 1000000000l> >, std::__1::chrono::duration, std::__1::ratio<1l, 1000000000l> >((* & fd))’
/home/noloader/llvm_source/llvm/projects/libcxx/include/thread:438:73: error: ‘(9.223372036854775807e+18 / 1.0e+9)’ is not a constant expression
_LIBCPP_CONSTEXPR duration _Max = nanoseconds::max();
^
At global scope:
cc1plus: warning: unrecognized command line option "-Wno-noexcept-type" [enabled by default]
cc1plus: warning: unrecognized command line option "-Wno-c++14-compat" [enabledby default]
make[2]: *** [projects/libcxx/lib/CMakeFiles/cxx_objects.dir/
/src/condition_variable.cpp.o] Error 1
make[2]: Leaving directory /home/noloader/llvm_build' make[1]: *** [projects/libcxx/lib/CMakeFiles/cxx_objects.dir/all] Error 2 make[1]: Leaving directory /home/noloader/llvm_build'
make: *** [all] Error 2
Failed to make LLVM sources

It would be nice if LLVM could work around the issue.

@mclow
Copy link
Contributor

@mclow mclow commented Nov 20, 2018

The code is like it is because of #14093 ; so read that before attempting a fix.

@kencu
Copy link

@kencu kencu commented Dec 21, 2018

@mclow
Copy link
Contributor

@mclow mclow commented Feb 9, 2019

*** Bug llvm/llvm-bugzilla-archive#40671 has been marked as a duplicate of this bug. ***

@ikitayama
Copy link

@ikitayama ikitayama commented Feb 9, 2019

Is the proper fix being worked on?
I’d like a heads-up so I got pull from
llvm-project repo.

@mclow
Copy link
Contributor

@mclow mclow commented Feb 9, 2019

The "proper fix" is for GCC to fix their PPC floating point codegen, because '(9.223372036854775807e+18 / 1.0e+9)' certainly is a constant expression.

@ikitayama
Copy link

@ikitayama ikitayama commented Feb 9, 2019

So we either use the latest GCC version which has the fix or we apply the patch on
the GCC of preference? In my environment using GCC 7.2.0 seems to be required by the CUDA toolkit.

@llvmbot
Copy link
Collaborator

@llvmbot llvmbot commented Feb 10, 2019

Here's a minimal reproducer on godbolt: https://godbolt.org/z/xFkZTR

What confuses me is that the same expression that breaks with long double works with double and `float.

@llvmbot
Copy link
Collaborator

@llvmbot llvmbot commented Feb 10, 2019

@kencu
Copy link

@kencu kencu commented Feb 10, 2019

Here's my understanding:

It's a very old deficiency in gcc's handling of a unique IBM format for long long floating point values.

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=26374

A patch years ago was put it to indicate this type of folding could not be done:

https://gcc.gnu.org/ml/gcc-patches/2005-01/msg01966.html

And that code is more or less still in gcc8 in fold-const.c

  /* Don't constant fold this floating point operation if the
 result may dependent upon the run-time rounding mode and
 flag_rounding_math is set, or if GCC's software emulation
 is unable to accurately represent the result.  */
  if ((flag_rounding_math
   || (MODE_COMPOSITE_P (mode) && !flag_unsafe_math_optimizations))
  && (inexact || !real_identical (&result, &value)))
return NULL_TREE;

There is some work being done to change things for PPCLE but not for PPCBE:

https://fedoraproject.org/wiki/Changes/PPC64LE_Float128_Transition

There is nobody I know of who knows more about gcc and PowerPC than Iain Sandoe, and his workaround was in the patch I referenced above.

@noloader
Copy link
Mannequin Author

@noloader noloader mannequin commented Feb 10, 2019

For what it is worth I patch-out the constexpr part (https://github.com/noloader/build-llvm/blob/master/build-llvm.sh#L566):

THIS_FILE=include/thread
sed -i "s/_LIBCPP_CONSTEXPR duration _Max/const duration _Max/g" "$THIS_FILE" > "$THIS_FILE.patched"
mv "$THIS_FILE.patched" "$THIS_FILE"

I patch it out because my two choices are (1) broken compile an no compiler, or (2) lose constexpr on a function I probably won't use. The value still works, its just not constexpr.

Patching-out the constexpr is a no brainer for me because I need a working compiler.

@kencu
Copy link

@kencu kencu commented Feb 10, 2019

The way I understand Iain's patch, it changes the thread to sleep from thousands of years to just hundreds of years.

:>

@llvmbot
Copy link
Collaborator

@llvmbot llvmbot commented Feb 10, 2019

I'm wondering if replacing long double with double isn't the better fix here.
std::chrono::nanoseconds::max() should be representable as a double.

My understanding is that the GCC bug is only triggered when using the IBM long double ABI, and that other long double ABI's are available. Is these a GCC macro we can use to detect IBM's long double specifically?

1 similar comment
@llvmbot
Copy link
Collaborator

@llvmbot llvmbot commented Feb 10, 2019

I'm wondering if replacing long double with double isn't the better fix here.
std::chrono::nanoseconds::max() should be representable as a double.

My understanding is that the GCC bug is only triggered when using the IBM long double ABI, and that other long double ABI's are available. Is these a GCC macro we can use to detect IBM's long double specifically?

@kencu
Copy link

@kencu kencu commented Feb 10, 2019

As far as I can tell from https://github.com/llvm-mirror/libcxx/blob/master/src/condition_variable.cpp#L56 whatever superlong number we pass in here gets shortened down to "0x59682F000000E941" anyway.

So Iain's 592 years is long enough to saturate the system.

@ikitayama
Copy link

@ikitayama ikitayama commented Feb 13, 2019

Am I correct this doesn’t happen when building LLVM
with self bootstrapped LLVM?

@mclow
Copy link
Contributor

@mclow mclow commented Feb 13, 2019

Am I correct this doesn’t happen when building LLVM
with self bootstrapped LLVM?

That is my understanding. Building with LLVM is fine.
Also note that this is not "building LLVM", but building libc++.

@ikitayama
Copy link

@ikitayama ikitayama commented Feb 14, 2019

By fully bootstrapped LLVM, it means that the linker also needs to be
lld, not ld nor gold?

Am I correct this doesn’t happen when building LLVM
with self bootstrapped LLVM?

That is my understanding. Building with LLVM is fine.
Also note that this is not "building LLVM", but building libc++.

@mclow
Copy link
Contributor

@mclow mclow commented Feb 14, 2019

By fully bootstrapped LLVM, it means that the linker also needs to be
lld, not ld nor gold?

This is a compile-time error; it hasn't gotten to the linker yet.

@kencu
Copy link

@kencu kencu commented Feb 17, 2019

A similar issue recently came up in gcc’s bug tracker https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88204 on their test suite, and the resolution was to disable the test.

After 14 years, it would not seem very likely that gcc is going to fix this any time soon.

Although it is undesirable to have one system working differently than all the others, for bootstrapping libc++ with gcc on powerpc there would not appear to be a lot of other available options than a patch something like Iain’s.

Is there something about that patch (other than the systems test, which needs to be broadened out a bit) that I am missing that makes it unusable?

@mclow
Copy link
Contributor

@mclow mclow commented Apr 2, 2019

Fixed in r357478.

@ikitayama
Copy link

@ikitayama ikitayama commented Apr 2, 2019

Fixed in r357478.

I don't see the Fix is in the llvm-project repo on GitHub, should I try
the svn in the meantime?

@mclow
Copy link
Contributor

@mclow mclow commented Apr 3, 2019

And fixed correctly in https://llvm.org/r357540

@ikitayama
Copy link

@ikitayama ikitayama commented Apr 3, 2019

And fixed correctly in https://llvm.org/r357540

On ppc64le, the code base which has the Fix doen's get build:

$ cmake -G Ninja -DLLVM_ENABLE_PROJECTS="libcxx;libcxxabi" -DLLVM_TEMPORARILY_ALLOW_OLD_TOOLCHAIN=TRUE -DCMAKE_C_COMPILER=/gpfs/software/opt/gcc/5.4.0/bin/gcc -DCMAKE_CXX_COMPILER=/gpfs/software/opt/gcc/5.4.0/bin/g++ ../llvm

@ikitayama
Copy link

@ikitayama ikitayama commented Apr 3, 2019

BTW, it builds fine on x86.

@kencu
Copy link

@kencu kencu commented Jul 9, 2019

I must be cross-eyed.

To me the original commit looks correct, and the corrected fix now looks backwards.

When building with gcc, on powerpc, don't we want to use

_LIBCPP_CONSTEXPR duration _Max = duration(ULLONG_MAX/1000000000ULL) ;

Because if so, we have that backwards now, don't we?

@llvmbot
Copy link
Collaborator

@llvmbot llvmbot commented Aug 18, 2019

This is the correct patch for ppc64el. I've tested this on a Raptor System Talos II Power9 workstation.

diff --git a/libcxx/include/thread b/libcxx/include/thread
index 8c0115f87..e439f60b9 100644
--- a/libcxx/include/thread
+++ b/libcxx/include/thread
@@ -435,7 +435,12 @@ sleep_for(const chrono::duration<_Rep, _Period>& __d)
using namespace chrono;
if (__d > duration<_Rep, _Period>::zero())
{
+#if defined(_LIBCPP_COMPILER_GCC) && (powerpc || POWERPC)

  •    //  GCC's long double const folding is incomplete for IBM128 long doubles.
    
  •    _LIBCPP_CONSTEXPR duration<long double> _Max = duration<long double>(ULLONG_MAX/1000000000ULL);
    

+#else
_LIBCPP_CONSTEXPR duration _Max = nanoseconds::max();
+#endif
nanoseconds __ns;
if (__d < _Max)
{

@ikitayama
Copy link

@ikitayama ikitayama commented Aug 18, 2019

Has this been landed on Trunk yet?

@ikitayama
Copy link

@ikitayama ikitayama commented Aug 18, 2019

And note that C++14 now that is a requirement.

@ikitayama
Copy link

@ikitayama ikitayama commented Aug 18, 2019

I confirm that I am able to build one of the LLVM projects, libcxx and libcxxabi on PowerPC natively for the first time ever. Time for changing the doc to Supported on Linux and PowerPC combination?

@llvmbot
Copy link
Collaborator

@llvmbot llvmbot commented Aug 18, 2019

Build clang-8.0.1 on linux shell script.
Test on IBM Power9 Ubuntu-18.04 for ppc64le.

@llvmbot
Copy link
Collaborator

@llvmbot llvmbot commented Aug 18, 2019

@llvmbot
Copy link
Collaborator

@llvmbot llvmbot commented Aug 18, 2019

I've attached a shell script for building clang-8.0.1 on linux. It's been test on a Power9 ppc64le system.

@ikitayama
Copy link

@ikitayama ikitayama commented Aug 18, 2019

Created attachment 22388 [details]
Build clang-8.0.1 on linux shell script.

Test on IBM Power9 Ubuntu-18.04 for ppc64le.

Do you have build logs carried out on POWER9?

@llvmbot
Copy link
Collaborator

@llvmbot llvmbot commented Aug 19, 2019

I'll rebuild it again and attach the logs in an hour or so. What do you hope to see in the logs?

@ikitayama
Copy link

@ikitayama ikitayama commented Aug 19, 2019

I'd like to see the nightly build logs as I am on POWER8 and do build LLVM routinely by myself, when failture occurs I'd like to cross-check against it.

@llvmbot
Copy link
Collaborator

@llvmbot llvmbot commented Aug 19, 2019

201908190440 clang-8.0.1 build log on ppc64le - power9
I've attached the build log. I deleted the install messages, since I can only post attachments upto 1MB here.

@llvmbot
Copy link
Collaborator

@llvmbot llvmbot commented Aug 20, 2019

I can confirm that as of now (SHA cda334b) this fix is still reversed on master. Adding a NOT for the if condition (or switching the bodies for if and else) would fix it

@llvmbot
Copy link
Collaborator

@llvmbot llvmbot commented Dec 4, 2019

I can also confirm that this is an issue in current master / trunk (llvm 10)
Here is my patch which I have incorporated in an spack package.
diff --git a/libcxx/include/thread b/libcxx/include/thread
index 02da703..d1677a1 100644
--- a/projects/libcxx/include/thread
+++ b/projects/libcxx/include/thread
@@ -368,9 +368,9 @@ sleep_for(const chrono::duration<_Rep, _Period>& __d)
{
#if defined(_LIBCPP_COMPILER_GCC) && (powerpc || POWERPC)
// GCC's long double const folding is incomplete for IBM128 long doubles.

  •    _LIBCPP_CONSTEXPR duration<long double> _Max = nanoseconds::max();
    

-#else
_LIBCPP_CONSTEXPR duration _Max = duration(ULLONG_MAX/1000000000ULL) ;
+#else

  •    _LIBCPP_CONSTEXPR duration<long double> _Max = nanoseconds::max();
    

#endif
nanoseconds __ns;
if (__d < _Max)

@jdenny-ornl
Copy link
Collaborator

@jdenny-ornl jdenny-ornl commented Apr 10, 2020

We have recently encountered this bug and have found that the patch in #39044 #c38 fixes it. What stands in the way of this being pushed? Would it help if I created a phab review for it?

@jdenny-ornl
Copy link
Collaborator

@jdenny-ornl jdenny-ornl commented Apr 20, 2020

We have recently encountered this bug and have found that the patch in
#39044 #c38 fixes it. What stands in
the way of this being pushed? Would it help if I created a phab review for
it?

Ping.

@jdenny-ornl
Copy link
Collaborator

@jdenny-ornl jdenny-ornl commented Apr 28, 2020

Ping. What's the best way to move this forward?

@jdenny-ornl
Copy link
Collaborator

@jdenny-ornl jdenny-ornl commented May 4, 2020

Ping.I suppose I should just take the patch to phabricator.

@jdenny-ornl
Copy link
Collaborator

@jdenny-ornl jdenny-ornl commented Jun 10, 2020

Fixed: https://reviews.llvm.org/D81438

@mclow
Copy link
Contributor

@mclow mclow commented Nov 27, 2021

mentioned in issue llvm/llvm-bugzilla-archive#40671

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 10, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla build-problem libc++
Projects
None yet
Development

No branches or pull requests

5 participants