Bug 1472437 - glibc: Can't compile libvirt with clang: implicit conversion increases floating-point precision in isnan()
Summary: glibc: Can't compile libvirt with clang: implicit conversion increases floati...
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: clang
Version: 29
Hardware: x86_64
OS: Linux
unspecified
unspecified
Target Milestone: ---
Assignee: serge_sans_paille
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 1674026 1674028 1674031
TreeView+ depends on / blocked
 
Reported: 2017-07-18 17:46 UTC by Andrea Bolognani
Modified: 2019-07-03 12:53 UTC (History)
16 users (show)

Fixed In Version: clang-7.0.1-6.fc29
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
: 1674026 1674028 1674031 (view as bug list)
Environment:
Last Closed: 2019-07-03 12:53:20 UTC
Type: Bug


Attachments (Terms of Use)


Links
System ID Private Priority Status Summary Last Updated
LLVM 35268 0 P RESOLVED System header warning in /usr/include/math.h 2020-03-22 21:03:10 UTC

Description Andrea Bolognani 2017-07-18 17:46:48 UTC
Description of problem:

  libvirt can't be compiled using clang on Fedora rawhide,
  apparently due to the way the isnan() macro has been
  defined.

Version-Release number of selected component (if applicable):

  glibc-2.25.90-24.fc27.x86_64
  clang-4.0.1-1.fc27.x86_64

How reproducible:

  100%

Steps to Reproduce:

  1. git clone git://libvirt.org/libvirt.git
  2. cd libvirt
  3. sudo yum-builddep libvirt.spec.in
  4. CC=clang-4.0 sh autogen.sh --system
  5. make -j

Actual results:

  Compilation fails with a bunch of errors along the lines of:

  util/virxml.c:453:30: error: implicit conversion increases floating-point
  precision: 'double' to 'long double' [-Werror,-Wdouble-promotion]
                 (!(isnan(obj->floatval)))) {
                    ~~~~~~~~~~~^~~~~~~~~
  /usr/include/math.h:440:46: note: expanded from macro 'isnan'
  #  define isnan(x) __MATH_TG ((x), __isnan, (x))
                     ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~
  /usr/include/math.h:373:16: note: expanded from macro '__MATH_TG'
     : FUNC ## l ARGS)
       ~~~~~~~~~ ^~~~

Expected results:

  Compilation doesn't fail :)

Additional info:

  Tweaking /usr/include/math.h to replace the definition of
  isnan() with the one lifted from the very same file of a
  Fedora 24 box (glibc-2.23.1-12.fc24.x86_64) results in a
  successful compilation.

Comment 1 Daniel Berrangé 2017-08-11 16:28:55 UTC
FYI, this is is easily reproducable without libvirt

$ cat isnan.c 

#include <math.h>

int main(void)
{
  double foo = 1.0;

  if (isnan(foo))
    return 1;
  return 0;
}


$ clang -Wdouble-promotion -Wall -o isnan isnan.c
isnan.c:8:13: warning: implicit conversion increases floating-point precision:
      'double' to 'long double' [-Wdouble-promotion]
  if (isnan(foo))
      ~~~~~~^~~~
/usr/include/math.h:385:46: note: expanded from macro 'isnan'
#  define isnan(x) __MATH_TG ((x), __isnan, (x))
                   ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~
/usr/include/math.h:320:16: note: expanded from macro '__MATH_TG'
   : FUNC ## l ARGS)
     ~~~~~~~~~ ^~~~
1 warning generated.


I do wonder if this is actually a clang bug though.  The new glib math.h is calling  _builtin_isnan(), so presumably it is clang's implementation of _builtin_isnan() that is causing the double promotion. Glibc just happens to trigger it ?

Comment 2 Jan Kurik 2017-08-15 07:20:25 UTC
This bug appears to have been reported against 'rawhide' during the Fedora 27 development cycle.
Changing version to '27'.

Comment 3 Dridi Boukelmoune 2017-11-03 13:52:33 UTC
Same problem on f26, and if I neuter -Wdouble-promotion then -Wconversion triggers, and I'd rather really keep this one around :)

Comment 4 Carlos O'Donell 2017-11-06 18:23:42 UTC
The macro in question is this:

math/math.h:

398 # define __MATH_TG(TG_ARG, FUNC, ARGS)          \
399   (sizeof (TG_ARG) == sizeof (float)            \
400    ? FUNC ## f ARGS                             \
401    : sizeof (TG_ARG) == sizeof (double)         \
402    ? FUNC ARGS                                  \
403    : FUNC ## l ARGS)

The problem is that clang warns about 'FUNC ## l ARGS' even though this is dead code, the parameter is a double, and will never be passed to 'FUNC ## l ARGS'.

The is is a false positive warning from clang.

I'm assigning to clang for further triage.

Comment 5 Tom Stellard 2017-11-07 22:04:21 UTC
__builtin_isnan() is not actually called in this example program, because clang does:

Builder.defineMacro("__GNUC_MINOR__", "2");
Builder.defineMacro("__GNUC_PATCHLEVEL__", "1");
Builder.defineMacro("__GNUC__", "4");

And the call to __builtin_isnan() is guarded by:
if __GNUC_PREREQ (4,4) && !defined __SUPPORT_SNAN__

Comment 6 Ben Cotton 2018-11-27 13:52:59 UTC
This message is a reminder that Fedora 27 is nearing its end of life.
On 2018-Nov-30  Fedora will stop maintaining and issuing updates for
Fedora 27. It is Fedora's policy to close all bug reports from releases
that are no longer maintained. At that time this bug will be closed as
EOL if it remains open with a Fedora  'version' of '27'.

Package Maintainer: If you wish for this bug to remain open because you
plan to fix it in a currently maintained version, simply change the 'version' 
to a later Fedora version.

Thank you for reporting this issue and we are sorry that we were not 
able to fix it before Fedora 27 is end of life. If you would still like 
to see this bug fixed and are able to reproduce it against a later version 
of Fedora, you are encouraged  change the 'version' to a later Fedora 
version prior this bug is closed as described in the policy above.

Although we aim to fix as many bugs as possible during every release's 
lifetime, sometimes those efforts are overtaken by events. Often a 
more recent Fedora release includes newer upstream software that fixes 
bugs or makes them obsolete.

Comment 7 Andrea Bolognani 2018-11-27 14:05:15 UTC
The reproducer provided by Dan in Comment 1 still triggers the
issue on a fully up-to-date Fedora 29 system:

  $ cat isnan.c
  #include <math.h>

  int main(void)
  {
    double foo = 1.0;

    if (isnan(foo))
      return 1;
    return 0;
  }

  $ clang -Wdouble-promotion -Wall -o isnan isnan.c
  isnan.c:7:13: warning: implicit conversion increases floating-point precision: 'double' to 'long double' [-Wdouble-promotion]
    if (isnan(foo))
        ~~~~~~^~~~
  /usr/include/math.h:928:46: note: expanded from macro 'isnan'
  #  define isnan(x) __MATH_TG ((x), __isnan, (x))
                     ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~
  /usr/include/math.h:846:16: note: expanded from macro '__MATH_TG'
     : FUNC ## l ARGS)
       ~~~~~~~~~ ^~~~
  1 warning generated.

  $ clang --version
  clang version 7.0.0 (Fedora 7.0.0-2.fc29)
  Target: x86_64-unknown-linux-gnu
  Thread model: posix
  InstalledDir: /usr/bin

Updating the 'version' field accordingly.

Comment 8 serge_sans_paille 2018-12-10 16:22:21 UTC
The prepprocessed code  (relevant part) look like this:


  if ((sizeof ((foo)) == sizeof (float)
                          ? __isnanf (foo) :
                          sizeof ((foo)) == sizeof (double)
                          ? __isnan (foo)
                          : __isnanl (foo)))

So the warning actually warns on a dead branch, which lets me wonder what's the best fix:

1. Fix clang to evaluate the conditions at compile time and don't warn on dead branches
2. Force a cast on each branch to the relevant type, which should be a no-op.

Comment 9 serge_sans_paille 2018-12-10 17:36:40 UTC
Libc-independant reproducer:

extern int popl(long double __value); 
extern int pop (double __value); 
int main(void) 
{ 
  float foo = 1.0; 
 
  if (sizeof ((foo)) == sizeof (double)) 
        pop(foo); 
  if (sizeof ((foo)) == sizeof (long double)) 
        popl(foo); 
  return 0; 
}

Comment 10 serge_sans_paille 2018-12-10 19:25:32 UTC
So the underlying question is: should a compiler warn about code that never gets executed. Clang tends to not report warning in dead code, as showcased in https://godbolt.org/z/3PD-eF

int case0(int num) {
    if(0)
        return num / 0; << no warning
    else
        return 1;
}
int case1(int num) {
    if(num)
        return num / 0; << warning
    else
        return 1;
}

This behavior is not consistent for -Wdouble-conversion, as showcased in https://godbolt.org/z/58Q5oA


double foo(double);

int case0(float num) {
    if(0)
        return foo(num); << warning
    else
        return 1;
}
int case1(float num) {
    if(num)
        return foo(num); << warning
    else
        return 1;
}


Bug reported in https://bugs.llvm.org/show_bug.cgi?id=39938

Comment 11 serge_sans_paille 2018-12-17 20:24:23 UTC
Patch + bug details in https://reviews.llvm.org/D55782

Comment 12 serge_sans_paille 2019-02-01 06:33:54 UTC
Fixed upstream in https://reviews.llvm.org/rL352838

Comment 13 Fedora Update System 2019-03-06 08:55:25 UTC
clang-7.0.1-5.fc29 has been submitted as an update to Fedora 29. https://bodhi.fedoraproject.org/updates/FEDORA-2019-3521ab1e66

Comment 14 Fedora Update System 2019-03-06 15:57:24 UTC
clang-7.0.1-5.fc29 has been pushed to the Fedora 29 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2019-3521ab1e66

Comment 15 Dridi Boukelmoune 2019-03-09 08:12:30 UTC
I gave negative karma to this update, it didn't solve the warning on my code, not even for the isnan.c reproducer from this ticket :(

Comment 16 Fedora Update System 2019-03-11 08:30:13 UTC
clang-7.0.1-6.fc29 has been submitted as an update to Fedora 29. https://bodhi.fedoraproject.org/updates/FEDORA-2019-33b0baf601

Comment 17 Fedora Update System 2019-03-11 20:48:54 UTC
clang-7.0.1-6.fc29 has been pushed to the Fedora 29 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2019-33b0baf601

Comment 18 Dridi Boukelmoune 2019-03-12 17:19:15 UTC
Still a problem for me:

$ cat isnan.c 
#include <assert.h>
#include <math.h>

int main(void)
{
  double foo = 1.0;

  assert(!isnan(foo));
  return 0;
}

$ clang -Wdouble-promotion -Wall -o isnan isnan.c
isnan.c:8:17: warning: implicit conversion increases floating-point precision: 'double' to 'long double' [-Wdouble-promotion]
  assert(!isnan(foo));
          ~~~~~~^~~~
/usr/include/math.h:928:46: note: expanded from macro 'isnan'
#  define isnan(x) __MATH_TG ((x), __isnan, (x))
                   ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~
/usr/include/math.h:846:16: note: expanded from macro '__MATH_TG'
   : FUNC ## l ARGS)
     ~~~~~~~~~ ^~~~
/usr/include/assert.h:106:11: note: expanded from macro 'assert'
      if (expr)                                                         \
          ^~~~
1 warning generated.

Comment 19 serge_sans_paille 2019-03-14 10:18:55 UTC
I can reproduce on master, the following test case also fails (not using a system macro)

#include <math.h>
#define cassert(v) v

int main() {
        double d = 1.;
        cassert(isnan(d));
        return 0;
}

Comment 20 Florian Weimer 2019-03-14 10:34:06 UTC
Does Clang support __builtin_tgmath or __builtin_isnan?

The macro expansion is problematic for nested expressions with type-generic math anyway:

  <https://sourceware.org/bugzilla/show_bug.cgi?id=21660>

This is a serious impediment to usability of this feature, and it was fixed in GCC 8.

(isnan is required to be type-generic even though it is not defined in <tgmath.h>, but <math.h>.)

Comment 21 serge_sans_paille 2019-03-15 17:50:25 UTC
Patch upstream: https://reviews.llvm.org/D59413

Comment 22 serge_sans_paille 2019-03-18 16:36:53 UTC
@fweimer: yes clang does support ``__builtin_isnan``, but not ``__builtin_tgmath``.
I double-checked and confirm clang's memory usage doesn't peek on the test case associated to https://sourceware.org/bugzilla/show_bug.cgi?id=21660

Comment 23 Fedora Update System 2019-03-20 22:12:47 UTC
clang-7.0.1-6.fc29 has been pushed to the Fedora 29 stable repository. If problems still persist, please make note of it in this bug report.

Comment 24 Fedora Update System 2019-03-21 14:40:27 UTC
clang-7.0.1-6.fc29 has been pushed to the Fedora 29 stable repository. If problems still persist, please make note of it in this bug report.

Comment 25 Dridi Boukelmoune 2019-03-24 10:02:29 UTC
I'm reopening this ticket because I'm looking for an update with the second patch too.

Comment 26 serge_sans_paille 2019-05-16 15:15:45 UTC
Didri, this scratch build should do the trick can you confirm? https://koji.fedoraproject.org/koji/taskinfo?taskID=34882820

Comment 27 serge_sans_paille 2019-06-04 16:15:57 UTC
Didri : can you confirm clang-8.0.0-3.fc31 fixes your issue?

Comment 28 Dridi Boukelmoune 2019-07-01 10:02:11 UTC
Apologies for the late reply, but I've been unable to login with either my FAS account or my email/password for a long while now (not that I checked everyday).

I just gave a try to this update and I still get spurious -Wdouble-promotion warnings.

Comment 29 Dridi Boukelmoune 2019-07-01 10:09:09 UTC
Actually, it looks like it fixes more of the false positives, and I was running into more of them:

    $ cat test.c 
    #include <math.h>

    int main() {
            double d = NAN;
            return 0;
    }

    $ clang -Wdouble-promotion test.c 
    test.c:4:20: warning: implicit conversion increases floating-point precision: 'float' to 'double' [-Wdouble-promotion]
            double d = NAN;
               ~   ^~~
    /usr/include/math.h:98:16: note: expanded from macro 'NAN'
    #  define NAN (__builtin_nanf (""))
               ^~~~~~~~~~~~~~~~~~~
    1 warning generated.

HTH

Comment 30 serge_sans_paille 2019-07-01 13:44:33 UTC
Glad you made your way through the authentication!

According to https://www.gnu.org/software/libc/manual/html_node/Infinity-and-NaN.html you shoould use NAN for float and SNAN for double. Not clang's fault this time ;-)

Comment 31 Florian Weimer 2019-07-01 13:54:17 UTC
(In reply to serge_sans_paille from comment #30)
> Glad you made your way through the authentication!
> 
> According to
> https://www.gnu.org/software/libc/manual/html_node/Infinity-and-NaN.html you
> shoould use NAN for float and SNAN for double. Not clang's fault this time
> ;-)

The SNAN* constants are for signaling NANs.  I don't think the headers define type-specific non-signalling NAN constants.  NAN is expected to be type-generic, so I think the clang warning is off here.

Comment 32 Dridi Boukelmoune 2019-07-01 14:37:59 UTC
I wasn't aware of those extensions of the ISO C standard, and I'm almost positive this is not in POSIX.

Alternatively I could use the nan(), nanf() or nanl() functions but I find the NAN macro more tidy, and I tend to use the functions in error handling code instead.

I trust Florian's expertise here, but will add that POSIX explicitly mentions the float type if my memory serves well. My expectation as a Joe Random C developer would be no warning though, but my expectations can be wrong too :)

Comment 33 serge_sans_paille 2019-07-01 15:20:12 UTC
My bad, thanks Florian for pointing out the signaling NAN issue.

Didri, according to https://godbolt.org/z/iZ8g8Z clang does not handle NAN and nan("") the same way, but an explicit cast on NAN does silent out the warning.

Comment 34 Dridi Boukelmoune 2019-07-01 16:32:52 UTC
Segre, I know an explicit "I know what I'm doing" cast will silence the warning, but it's a bit cumbersome to have to cast everywhere in a code base that almost always uses double.

I'm fine with the outcome, at least isnan() no longer triggers the warning and -Wdouble-promotion is much more usable with clang (GCC doesn't complain for NAN). Feel free to bring this upstream or close this ticket again.

Comment 35 serge_sans_paille 2019-07-03 12:53:20 UTC
@Didri, ok I'm closing this thread almost two years after it was opened then!


Note You need to log in before you can comment on or make changes to this bug.