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

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

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.

 

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)

 

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 overload
ambiguity error.

 

It's easy to say "It's C++, use '<cmath>'", but in the real-world C++
programmers very commonly use the C headers whether they should or not.  In
any case, '<math.h>' does not belong to C++ and the C++ implementation
should not interfere with the interface expressed by a foreign header when
that header is included directly.

 

After looking further into this I realised that there is an inadvertent bug
in the new implementation rather than an intent to interfere with the C
interface.  It also breaks C++'s '<cmath>' interface requirements.  The
supplementary elements used to be in '<cmath>' itself, but since they were
extracted they are in the global namespace and not in 'namespace std' where
they are intended to be, and they are consequently introducing overloads
into the C usage.  I will describe this for 'fabs' but the pattern appears
to be in all of the new ISO C wrapper headers.  In 'c++/math.h' we have
(abbreviated for illustration):

 

#ifndef _LIBCPP_MATH_H

#define _LIBCPP_MATH_H

 

#include <__config>

#include_next <math.h>

 

#ifdef __cplusplus

 

// Missing: _LIBCPP_BEGIN_NAMESPACE_STD

 

#if !(defined(_LIBCPP_MSVCRT) || defined(_AIX))

inline _LIBCPP_INLINE_VISIBILITY float       fabs(float __lcpp_x) _NOEXCEPT
{return fabsf(__lcpp_x);}

inline _LIBCPP_INLINE_VISIBILITY long double fabs(long double __lcpp_x)
_NOEXCEPT {return fabsl(__lcpp_x);}

#endif

 

template <class _A1>

inline _LIBCPP_INLINE_VISIBILITY

typename std::enable_if<std::is_integral<_A1>::value, double>::type

fabs(_A1 __lcpp_x) _NOEXCEPT {return fabs((double)__lcpp_x);}

 

// Missing: _LIBCPP_END_NAMESPACE_STD

 

#endif // __cplusplus

 

#endif // _LIBCPP_MATH_H

 

but because this is no longer between '_LIBCPP_BEGIN_NAMESPACE_STD' and
'_LIBCPP_END_NAMESPACE_STD', the new declarations introduce overloads at the
global namespace and not in the 'std' namespace.

 

Now when '<math.h>' is included explicitly you get 'fabs' as follows:

 

double         ::fabs(double)      // From 'include/math.h'

float          ::fabs(float)       // From 'include/c++/math.h'

long double    ::fabs(long double) // Also from 'include/c++/math.h'

 

With the namespace correction explicitly including '<math.h>' would produce:

 

double         ::fabs(double)      // From 'include/math.h'

float       std::fabs(float)       // From 'include/c++/math.h'

long double std::fabs(long double) // Also from 'include/c++/math.h'

 

Which will not break the C compatible use case.

 

However, I think that using the ISO C math header explicitly like this
should not be supplemented with partial ISO C++math header requirements,
it's neither C's '<math.h>' nor C++'s '<cmath>' 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

 

This is not as serious as the namespace bug, but it is strangely
inconsistent and shouldn't really be there.  What I propose for this is an
equally simple change.  In '<cmath>' replace:

 

#include <math.h>

with:

#define _LIBCPP_INCLUDING_STDC_HEADER

#include <math.h>

#undef _LIBCPP_INCLUDING_STDC_HEADER

 

and then in 'c++/math.h', replace:

 

#ifdef __cplusplus

with:

#ifdef _LIBCPP_INCLUDING_STDC_HEADER

 

I've only referred to 'fabs' and '<math.h>', but of course it is all the
overloaded "from C" functions in the 'std::' namespace that are affected,
and the same problem is present in the other ISO C wrapper headers.

 

I will work on this over the weekend and hope to be able to submit a tested
patch file on Monday or Tuesday that will correct this.  I'm at SVN revision
#258500 for the v3.8 branch at the moment, but I will update and merge to
the most recent version before I submit a patch file for your consideration.

 

Thanks,

 

            MartinO

 

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