[cfe-dev] LibC++ v3.8 - Problems with ISO C wrapper headers

[Martin J. O'Riordan via cfe-dev]

I disagree.  I'm not new to C++ either, I built my first C++ compiler in 1986 and have been involved in C++ standardisation for a very long time, and the recent changes are wrong.

Including "math.h" should NOT introduce overloaded versions of 'fabs' or other functions at global scope.  It may only introduce the overloaded functions within "namespace std".  The recent changes overload these functions at global scope.  That is a bug.

My tests are running now, and when they are complete I will propose the patch, but I can assure you that overloading global symbols from the C headers is not compliant with C++.

	MartinO

-----Original Message-----
From: metafoo at gmail.com [mailto:metafoo at gmail.com] On Behalf Of Richard Smith
Sent: 23 January 2016 16:58
To: Martin.ORiordan at movidius.com
Cc: Clang Dev <cfe-dev at lists.llvm.org>
Subject: Re: [cfe-dev] LibC++ v3.8 - Problems with ISO C wrapper headers

Hi Martin,

On 1/23/16, Martin J. O'Riordan via cfe-dev <cfe-dev at lists.llvm.org> wrote:
> While getting our implementation of the compiler ready for v3.8 we 
> came across a significant problem in the new implementation of 
> LibC++'s handling of the ISO C Standard headers that needs to be fixed 
> before the v3.8 release.

You're mistaken about a lot of the things you say below, but I agree that your example code ought to work, assuming that libc++ intends to support __fp16 (but I don't think it currently does). See the bottom of my mail for a suggestion of how to move forward with this.

> We discovered the problem when running some tests that were derived 
> from real-world code.  The initial example involved a C++ program 
> including '<math.h>' and using 'fabs' but the issue applies equally to 
> the other headers, for example:
>
> #include <math.h>
>
> ...
>
> __fp16 y;  // We use FP16 a lot
>
> fabs(y);   // really -> fabs((double)y);
>
> In C there is only one 'fabs' with the signature:
>
> double fabs(double)

That's largely irrelevant. In C++, <math.h> is a C++ standard library header, whose contents are specified by C++'s [c.math] and [depr.c.headers]. These say that <math.h> provides the same set of overloads in the global namespace that <cmath> provides in namespace std. libc++ has recently changed to implement this rule (that has
*always* been part of the C++ standard). Thus your suggested approach to fixing this is not appropriate.

> With the new implementation of the headers for LibC++, there is now a 
> file named 'c++/math.h' that includes the C version and then 
> supplements it for
> C++.  When I use this, the above use of 'fabs' above results in an 
> C++overload
> ambiguity error.
>
> It's easy to say "It's C++, use '<cmath>'",

That wouldn't help. In namespace std, libc++ has always provided overloads resulting in the ambiguity you're now seeing (and FWIW, this is the overload set required by the C++ standard).

[...]
> and has odd incongruities such as:
>
> #include <math.h>
>
> float       (*pf)(float)        = std::fabs; // Okay, we have this
> long double (*pld)(long double) = std::fabs; // Okay, we have this too
> double      (*pd)(double)       = std::fabs; // Oops, no such function

That seems a consequence of your (incorrect) patch. libc++'s current <math.h> doesn't provide any of these in namespace std, consistent with the requirements of the C++ standard. The intent is that no set of #includes gives you a partial overload set for any of these functions (except cases like 'abs', where C++ specifies that the overload set is split across multiple headers).


The real issue is that libc++ doesn't support __fp16 (you should be able to observe this with both past versions of libc++ and with trunk if you change your testcase to use <cmath> and std::fabs). The right way to fix this would be to add __fp16 overloads to <math.h> (in the cases where the underlying compiler and language mode provide such a
type) and ensure its __promote does the right thing for __fp16. It's up to the libc++ maintainers whether they consider this to be a regression and whether they'd accept a patch for it in the 3.8 release.

But it's not a bug that libc++'s <math.h> now follows the C++ standard when compiling in C++ mode.
--
Richard