[FileCheck] Fix FileCheck.cpp compilation on Solaris

[ro]

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.

[jdenny]

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.

[ro]

In D60043#1449881, @jdenny wrote:

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.

[jyknight]

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]

In D60043#1449898, @jyknight wrote:

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]

Revised patch.

[ro]

In D60043#1449930, @jdenny wrote:

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]

include/llvm/Support/FileCheck.h

22 ↗

(On Diff #193243)

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

[ro]

include/llvm/Support/FileCheck.h

22 ↗

(On Diff #193243)

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

[jdenny]

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

[ro]

Move <cmath> include to FileCheck.cpp.

[ro]

In D60043#1451326, @jdenny wrote:

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

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

[jdenny]

LGTM again. Thanks everyone.

llvm/trunk/utils/FileCheck/FileCheck.cpp

 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/WithColor.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Support/FileCheck.h"
+#include <cmath>
 using namespace llvm;
 
 static cl::opt<std::string>
     CheckFilename(cl::Positional, cl::desc("<check-file>"), cl::Optional);
 
 static cl::opt<std::string>
     InputFilename("input-file", cl::desc("File to check (defaults to stdin)"),
                   cl::init("-"), cl::value_desc("filename"));
std::sort(Annotations.begin(), Annotations.end(),
             });
 
   // Compute the width of the label column.
   const unsigned char *InputFilePtr = InputFileText.bytes_begin(),
                       *InputFileEnd = InputFileText.bytes_end();
   unsigned LineCount = InputFileText.count('\n');
   if (InputFileEnd[-1] != '\n')
     ++LineCount;
-  unsigned LineNoWidth = log10(LineCount) + 1;
+  unsigned LineNoWidth = std::log10(LineCount) + 1;
   // +3 below adds spaces (1) to the left of the (right-aligned) line numbers
   // on input lines and (2) to the right of the (left-aligned) labels on
   // annotation lines so that input lines and annotation lines are more
   // visually distinct.  For example, the spaces on the annotation lines ensure
   // that input line numbers and check directive line numbers never align
   // horizontally.  Those line numbers might not even be for the same file.
   // One space would be enough to achieve that, but more makes it even easier
   // to see.