This is an archive of the discontinued LLVM Phabricator instance.

[FileCheck] Fix FileCheck.cpp compilation on Solaris
ClosedPublic

Authored by ro on Mar 31 2019, 2:44 AM.

Details

Summary

Both LLVM 8.0.0 and current trunk fail to compile on Solaris with GCC 8.1.0:

/vol/llvm/src/llvm/dist/utils/FileCheck/FileCheck.cpp: In function ‘void DumpAnnotatedInput(llvm::raw_ostream&, const llvm::FileCheckRequest&, llvm::StringRef, std::vector<InputAnnotation>&, unsigned int)’:
/vol/llvm/src/llvm/dist/utils/FileCheck/FileCheck.cpp:408:41: error: call of overloaded ‘log10(unsigned int&)’ is ambiguous
   unsigned LineNoWidth = log10(LineCount) + 1;
                                         ^
In file included from /vol/gcc-8/lib/gcc/i386-pc-solaris2.11/8.1.0/include-fixed/math.h:24,
                 from /vol/gcc-8/include/c++/8.1.0/cmath:45,
                 from /vol/llvm/src/llvm/dist/include/llvm-c/DataTypes.h:28,
                 from /vol/llvm/src/llvm/dist/include/llvm/Support/DataTypes.h:16,
                 from /vol/llvm/src/llvm/dist/include/llvm/ADT/Hashing.h:47,
                 from /vol/llvm/src/llvm/dist/include/llvm/ADT/ArrayRef.h:12,
                 from /vol/llvm/src/llvm/dist/include/llvm/Support/CommandLine.h:22,
                 from /vol/llvm/src/llvm/dist/utils/FileCheck/FileCheck.cpp:18:
/vol/gcc-8/lib/gcc/i386-pc-solaris2.11/8.1.0/include-fixed/iso/math_iso.h:209:21: note: candidate: ‘long double std::log10(long double)’
  inline long double log10(long double __X) { return __log10l(__X); }
                     ^~~~~
/vol/gcc-8/lib/gcc/i386-pc-solaris2.11/8.1.0/include-fixed/iso/math_iso.h:170:15: note: candidate: ‘float std::log10(float)’
  inline float log10(float __X) { return __log10f(__X); }
               ^~~~~
/vol/gcc-8/lib/gcc/i386-pc-solaris2.11/8.1.0/include-fixed/iso/math_iso.h:70:15: note: candidate: ‘double std::log10(double)’
 extern double log10 __P((double));
               ^~~~~

Fixed by casting the log10 arg to double, which allowed the compilation on i386-pc-solaris2.11
and sparc-sun-solaris2.11 to continue.

Diff Detail

Repository
rL LLVM

Event Timeline

ro created this revision.Mar 31 2019, 2:44 AM
jdenny accepted this revision.Apr 1 2019, 8:16 AM

Thanks for the patch.

I'm not working on the affected platform, so I haven't confirmed the problem. Why doesn't it affect others?

Regardless, the solution should be harmless, so LGTM if it helps.

This revision is now accepted and ready to land.Apr 1 2019, 8:16 AM
ro added a comment.Apr 1 2019, 8:24 AM

I'm not working on the affected platform, so I haven't confirmed the problem. Why doesn't it affect others?

Unlike other platforms (e.g. glibc-based or the BSDs), Solaris has two specializations of std::log10 in its <math.h>, e.g.

	inline float log10(float __X) { return __log10f(__X); }
	inline long double log10(long double __X) { return __log10l(__X); }

This is allowed by ISO C++, but not required AFAIU.

Regardless, the solution should be harmless, so LGTM if it helps.

Thanks. FWIW, I'd also tried a build on x86_64-pc-linux-gnu without problems or regressions.

Sounds like this file is missing #include <math.h> (it probably only #include <cmath>). If it wants to use ::log10, it should #include <math.h>. If it wants to use <cmath>, it should use std::log10.

Ideally, this wouldn't cause this sort of error, but see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89855 which I just filed recently about this problem -- although I didn't know it affected llvm!

Why this is broken only in solaris -- in solaris libc, the LIBC math.h header itself defines the overloads for float, and long double in C++ (in addition to the usual C double). But, they do not define the C++11 "any-int-type" overload. Thus, you get the ambiguous overloads resulting from this GCC bug only on solaris. On other platforms, you just see only the double overload. Which in this instance is fine, but if you were passing a float or long double, may not be!

jdenny added a comment.Apr 1 2019, 8:49 AM

Sounds like this file is missing #include <math.h> (it probably only #include <cmath>). If it wants to use ::log10, it should #include <math.h>. If it wants to use <cmath>, it should use std::log10.

Ideally, this wouldn't cause this sort of error, but see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89855 which I just filed recently about this problem -- although I didn't know it affected llvm!

Why this is broken only in solaris -- in solaris libc, the LIBC math.h header itself defines the overloads for float, and long double in C++ (in addition to the usual C double). But, they do not define the C++11 "any-int-type" overload. Thus, you get the ambiguous overloads resulting from this GCC bug only on solaris. On other platforms, you just see only the double overload. Which in this instance is fine, but if you were passing a float or long double, may not be!

Would including <cmath> and instead calling std::log10 avoid the problem without the explicit cast?

ro updated this revision to Diff 193243.Apr 2 2019, 1:55 AM

Revised patch.

ro added a comment.Apr 2 2019, 1:58 AM

Would including <cmath> and instead calling std::log10 avoid the problem without the explicit cast?

This works just as well. The revised patch has been tested as before on i386-pc-solaris2.11 and sparc-sun-solaris2.11.

jdenny added inline comments.Apr 2 2019, 6:00 AM
include/llvm/Support/FileCheck.h
22

Does this need to be processed by all includers? Shouldn't it be sufficient in FileCheck.cpp?

ro marked an inline comment as done.Apr 2 2019, 6:04 AM
ro added inline comments.
include/llvm/Support/FileCheck.h
22

FileCheck.cpp didn't include any non-LLVM headers before,
therefore I placed the include in a common header that did.

jdenny added a comment.Apr 2 2019, 6:13 AM

FileCheck.h needs those other non-LLVM headers. This header is different.

ro updated this revision to Diff 193276.Apr 2 2019, 6:24 AM

Move <cmath> include to FileCheck.cpp.

ro added a comment.Apr 2 2019, 6:24 AM

FileCheck.h needs those other non-LLVM headers. This header is different.

Fine with me: the revised version works just as well.

jdenny added a comment.Apr 2 2019, 7:23 AM

LGTM again. Thanks everyone.

This revision was automatically updated to reflect the committed changes.