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

[Hal Finkel via cfe-dev]

----- Original Message -----

> From: "Martin J. O'Riordan via cfe-dev" <cfe-dev at lists.llvm.org>
> To: "Eric Fiselier" <eric at efcs.ca>
> Cc: "Marshall Clow" <mclow.lists at gmail.com>, "Richard Smith"
> <richard at metafoo.co.uk>, "Clang Dev" <cfe-dev at lists.llvm.org>
> Sent: Saturday, January 23, 2016 3:36:24 PM
> Subject: Re: [cfe-dev] LibC++ v3.8 - Problems with ISO C wrapper
> headers

> 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)’
Of all of your examples, this is the most important. The other changes result in a compilation failure, and are easy to fix. However, it is certainly my experience that, in almost all cases, the semantic change you highlight is actually a silent bug *fix*, as the author almost always intended the behavior we now have, not the behavior we provided previously. O bviously this will not be 100% true, but on balance, the change has seems positive. Either way, good compiler warnings are essential. As I recall, Clang does have a good warning for this. 

-Hal 

> 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 > 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 ] 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
> 

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

-- 

Hal Finkel 
Assistant Computational Scientist 
Leadership Computing Facility 
Argonne National Laboratory 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20160123/8abe7b8f/attachment.html>