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

Sat Jan 23 13:36:24 PST 2016

The discussion on the Standards reflector at the time that this text was ratified was about how the C names were introduced into the global namespace, otherwise all that needed to be said was:

<math.h> is simply equivalent to:

#include <cmath>

using namespace std;

I understand your perspective on what is now section “D.5 C standard library headers [depr.c.headers]” in the WP, but this also has to be interpreted in the context of ISO’s fundamental charter which is “to standardise existing practice”.  I do not think anyone could argue with what is meant by existing practice concerning the inclusion of C headers in the context of a C++ program.  It looks to me now like the wording here introduced an unintended ambiguity in the language of the C++ Standard.

I do try to educate people to do C++ the C++ way, and include ‘<cXXXX>’ and then ‘using namespace std;’ instead of ‘<XXXX.h>’, and it bugs me when they don’t; but in practice they ignore such advice.  In fact, the majority of programs that are ported from C to C++ start out life by simply being renamed from “.c” to “.cpp”.  I would say that a good 90% of programmers using C++ for embedded systems are actually C programmers trying to “move up” and to take advantage of the extraordinary compile-time optimisations possible when templates are used (I still think that this is awesome versus C).

‘fabs’ and ‘__fp16’ was simple the observable failure that drew my attention to the issues but another example is the simpler ‘abs’ from ‘<stdlib.h>’.  In this case (in C) there is just one function:

int abs(int);

so code such as:

#include <stdint.h>

int a;

float b;

double c;

__fp16 d;  // I’ve left FP16 in just to show that it is not really different

abs(a); // implicitly abs(a)

abs(b); // implicitly abs((int)b)

abs(c); // implicitly abs((int)c)

abs(d); // implicitly abs((int)d)

previously chose ONLY ‘int ::abs(int)’, but by introducing overloads to the C functions at global scope, the semantics and behaviour of ‘abs(b)’ and ‘abs(c)’ changes, and because ‘__fp16’ is not a recognised type, it becomes ambiguous.  At least I see from the error message that the ‘__fp16’ case is now broken, but the ‘float’ and ‘double’ variants silently do something very different by calling different functions whose semantic implementation of ‘abs’ I can only trust is true.  I mentioned ‘__fp16’ earlier because that was the unhandled data type that made an otherwise silent and hidden semantic change visible to me.  We also found problems with ‘putc’ by the way, where it silently went from being a MACRO to an externally linked function resulting in unresolved link errors.  This was slightly different, but the underlying pattern change was the same.

The same ambiguity will arise whenever a class with a UDC to one of the overloaded types is involved, it is not just the tacitly implemented ‘__fp16’ data type.

This change is not existing practice, and I am surprised it was not vetoed, I would have certainly would have advised vetoing this if I was still active in our national Standardisation panel.

It also breaks previously previously “compliant” C++, for example:

#include <stdlib.h>

struct Foo {

  operator double () const;

  operator int () const;

};

Foo aFoo;

int x = abs(aFoo);

I’m not advocating such code, but without your interpretation of this clause, the above was valid, compliant and unambiguous.  It chose ‘int ::abs(int)’ and used ‘Foo::operator int()const’ to satisfy it.  But with your interpretation it is now a compile-time error - ambiguous selection.

Even worse are existing compliant C++ programs such as:

#include <stdlib.h>

struct Bar {

  operator double () const;

};

Bar aBar;

int x = abs(aBar);

The effective code subtly changes silently from:

int x = abs((int)aBar.operator double());  // choosing ‘int ::abs(int)’

to:

int x = (int)abs(aBar.operator double());  // choosing ‘double ::abs(double)’

My interpretation of the clause in [depr.c.headers] is not that it is equivalent to:

using std::abs;

which brings in the whole overload set, but rather something like:

using std::abs(int); // sic! - but selects an element of the overload set

these have very different meanings and intent.  The inclusion of a Standard header does not mean that textual inclusion or syntactic equivalence is required, it only requires that the Standard names become available to the program with their intended meaning.  And I believe that this is why the Standard does not specify “how” the names get into the global namespace - textual inclusion is just a common way of doing implementing.

My original message was not criticise the new implementation, but to “critique” it and highlight that it has changed the expected semantics and hence the “existing practice” tenet of Standard C++.  If it wasn’t for the fact that we are looking at v3.8 code freeze very soon, I would not have brought it up as an urgent issue.

            MartinO

From: Eric Fiselier [mailto:eric at efcs.ca] 
Sent: 23 January 2016 20:13
To: Martin J. O'Riordan <Martin.ORiordan at movidius.com>
Cc: Richard Smith <richard at metafoo.co.uk>; Clang Dev <cfe-dev at lists.llvm.org>; Marshall Clow <mclow.lists at gmail.com>
Subject: Re: [cfe-dev] LibC++ v3.8 - Problems with ISO C wrapper headers

Hi Martin,

If you disagree you'll need to cite (or change) the standard. IMO the standard is explicitly clear on this topic. 

15.2 [library.c] says:

> The C ++ standard library also makes available the facilities of the C standard library, suitably adjusted to

> ensure static type safety.

D.3 [depr.c.headers] says:

>1. every C header, each of which has a name of the form name.h, behaves as if each name placed in the standard

> library namespace by the corresponding cname header is placed within the global namespace scope. It is

> unspecified whether these names are first declared or defined within namespace scope (3.3.6) of the namespace

> std and are then injected into the global namespace scope by explicit using-declarations (7.3.3).

/Eric

On Sat, Jan 23, 2016 at 12:50 PM, Martin J. O'Riordan via cfe-dev <cfe-dev at lists.llvm.org <mailto:cfe-dev at lists.llvm.org> > wrote:

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>  [mailto: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 <mailto:Martin.ORiordan at movidius.com> 
Cc: Clang Dev <cfe-dev at lists.llvm.org <mailto: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 <mailto: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

_______________________________________________
cfe-dev mailing list
cfe-dev at lists.llvm.org <mailto:cfe-dev at lists.llvm.org> 
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20160123/a015e860/attachment.html>