NetBSD Problem Report #53618

From www@NetBSD.org  Wed Sep 19 03:37:16 2018
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 59D447A1B9
	for <gnats-bugs@gnats.NetBSD.org>; Wed, 19 Sep 2018 03:37:16 +0000 (UTC)
Message-Id: <20180919033715.3B0397A233@mollari.NetBSD.org>
Date: Wed, 19 Sep 2018 03:37:15 +0000 (UTC)
From: murray+net@ip-64-139-1-69.sjc.megapath.net
Reply-To: murray+net@ip-64-139-1-69.sjc.megapath.net
To: gnats-bugs@NetBSD.org
Subject: bogus warnings from printf %u and ntohs with -O1
X-Send-Pr-Version: www-1.0

>Number:         53618
>Category:       toolchain
>Synopsis:       bogus warnings from printf %u and ntohs with -O1
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    toolchain-manager
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Wed Sep 19 03:40:00 +0000 2018
>Last-Modified:  Fri Sep 21 23:40:01 +0000 2018
>Originator:     Hal Murray
>Release:        8.0
>Organization:
>Environment:
NetBSD bob3.example.com 8.0 NetBSD 8.0 (GENERIC) #0: Tue Jul 17 14:59:51 UTC 2018  mkrepro@mkrepro.NetBSD.org:/usr/src/sys/arch/amd64/compile/GENERIC amd64

>Description:
The test case below gets warnings like the following:

foo.c:17:10: warning: format '%u' expects argument of type 'unsigned int', but argument 2 has type 'int' [-Wformat=]
   printf("Foo %u\n", ntohs(x));

ntohs is unsigned, so I don't expect any problems.

It needs -Wformat -Wformat-signedness to correctly generate the warnings.  Adding -O1 generates the bogus warnings.

>How-To-Repeat:
/*
  save this as foo.c
  Gets bogus warnings from ntohs/htons when built with
    gcc -Wformat -Wformat-signedness -O1 foo.c
  Works OK with
    gcc -Wformat -Wformat-signedness foo.c
*/

#include <stdio.h>
#include <arpa/inet.h>

int main (int argc, char *argv[])
{
  uint16_t x = 13;
  int y = 13;
  printf("Yyy %u\n", y);        /* I expect this to get an error */
  printf("Y13 %u\n", 13);
  printf("Foo %u\n", ntohs(x));
  printf("Bar %u\n", htons(x));
}

>Fix:

>Audit-Trail:
From: Martin Husemann <martin@duskware.de>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: toolchain/53618: bogus warnings from printf %u and ntohs with -O1
Date: Wed, 19 Sep 2018 09:47:10 +0200

 This is how it traditionally worked. It does not even depend on
 optimization level, on big endian CPUs all ntoh* are no-ops.

 Our documentation gives a small hint:

      On machines which have a byte order which is the same as the network
      order, these routines are defined as macros that expand to the value of
      their argument.

 So you better store the value in an uint16_t if you need to rely on
 exact type.

 Martin

From: coypu@sdf.org
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: toolchain/53618: bogus warnings from printf %u and ntohs with -O1
Date: Wed, 19 Sep 2018 07:39:42 +0000

 this is because the definition of ntohs is
 #define ntohs(x) (x)
 in sys/endian.h.

 I feel like it's risky to change it because we would be making things
 truncate, no? are other people truncating?
 there's also a libc non-macro version.

From: Hal Murray <murray+net@ip-64-139-1-69.sjc.megapath.net>
To: gnats-bugs@NetBSD.org
Cc: toolchain-manager@netbsd.org, gnats-admin@netbsd.org,
    netbsd-bugs@netbsd.org, murray+net@ip-64-139-1-69.sjc.megapath.net
Subject: Re: toolchain/53618: bogus warnings from printf %u and ntohs with -O1
Date: Wed, 19 Sep 2018 01:28:10 -0700

 martin@duskware.de said:
 >  This is how it traditionally worked. It does not even depend on
 >  optimization level, on big endian CPUs all ntoh* are no-ops. 

 Thanks, but I don't understand what you are trying to tell me.

 I'm running on X86 so the no-op case shouldn't be happening.  Even if it did 
 happen, the argument to ntohs is an unsigned so it shouldn't complain.

 There are 4 printfs.  The first is there to make sure that I've got the 
 command line switches set to generate this type of warning.  The second is 
 there because I was curious.

 The last 2 are the interesting ones.  There are no complaints without -O1, but 
 warnings with -O1.

 If I actually run the code, both produce the same output and it is what I 
 expect.
 Yyy 13
 Y13 13
 Foo 3328
 Bar 3328

 3328 == 0xD00


 coypu@sdf.org said:
 >  this is because the definition of ntohs is
 >  #define ntohs(x) (x)
 >  in sys/endian.h. 

 That's the BYTE_ORDER == BIG_ENDIAN case
 I'm running on X86 which is LITTLE_ENDIAN

 #define ntohs(x)        bswap16(__CAST(uint16_t, (x)))


 -- 
 These are my opinions.  I hate spam.



From: Martin Husemann <martin@duskware.de>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: toolchain/53618: bogus warnings from printf %u and ntohs with -O1
Date: Wed, 19 Sep 2018 11:51:08 +0200

 On Wed, Sep 19, 2018 at 01:28:10AM -0700, Hal Murray wrote:
 > Thanks, but I don't understand what you are trying to tell me.

 The warning is not bogus, it just happens to depend on various things,
 starting with optimization level, but also the CPU you compile for.

 The man page says:

      On machines which have a byte order which is the same as the network
      order, these routines are defined as macros that expand to the value of
      their argument.

 so the behaviour is kinda expected.

 Gcc warnings vary a lot depending on what optimizer passes run - hunting
 that difference is like tilting windmills.

 Martin

From: Joerg Sonnenberger <joerg@bec.de>
To: gnats-bugs@NetBSD.org
Cc: toolchain-manager@netbsd.org, gnats-admin@netbsd.org,
	netbsd-bugs@netbsd.org
Subject: Re: toolchain/53618: bogus warnings from printf %u and ntohs with -O1
Date: Fri, 21 Sep 2018 09:36:59 +0200

 On Wed, Sep 19, 2018 at 03:40:00AM +0000, murray+net@ip-64-139-1-69.sjc.megapath.net wrote:
 > The test case below gets warnings like the following:
 > 
 > foo.c:17:10: warning: format '%u' expects argument of type 'unsigned int', but argument 2 has type 'int' [-Wformat=]
 >    printf("Foo %u\n", ntohs(x));

 ntohs(x) has two possible expansions:
    (x)
    (unsigned short expression)

 In the second case, it will be implicitly promoted to int. That's what
 the warning is about.

 Joerg

From: Valery Ushakov <uwe@stderr.spb.ru>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: toolchain/53618: bogus warnings from printf %u and ntohs with -O1
Date: Fri, 21 Sep 2018 14:16:01 +0300

 On Fri, Sep 21, 2018 at 07:40:01 +0000, Joerg Sonnenberger wrote:

 > From: Joerg Sonnenberger <joerg@bec.de>
 > To: gnats-bugs@NetBSD.org
 > Cc: toolchain-manager@netbsd.org, gnats-admin@netbsd.org,
 > 	netbsd-bugs@netbsd.org
 > Subject: Re: toolchain/53618: bogus warnings from printf %u and ntohs with -O1
 > Date: Fri, 21 Sep 2018 09:36:59 +0200
 > 
 >  On Wed, Sep 19, 2018 at 03:40:00AM +0000, murray+net@ip-64-139-1-69.sjc.megapath.net wrote:
 >  > The test case below gets warnings like the following:
 >  > 
 >  > foo.c:17:10: warning: format '%u' expects argument of type 'unsigned int', but argument 2 has type 'int' [-Wformat=]
 >  >    printf("Foo %u\n", ntohs(x));
 >  
 >  ntohs(x) has two possible expansions:
 >     (x)
 >     (unsigned short expression)
 >  
 >  In the second case, it will be implicitly promoted to int. That's what
 >  the warning is about.

 Isn't that what the 'h' length modifier (i.e. "%hu") is for?

 -uwe

From: Robert Elz <kre@munnari.OZ.AU>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: toolchain/53618: bogus warnings from printf %u and ntohs with -O1
Date: Sat, 22 Sep 2018 03:33:35 +0700

     Date:        Fri, 21 Sep 2018 11:20:01 +0000 (UTC)
     From:        Valery Ushakov <uwe@stderr.spb.ru>
     Message-ID:  <20180921112001.6B4C07A1D2@mollari.NetBSD.org>

   |  Isn't that what the 'h' length modifier (i.e. "%hu") is for?

 No, 'h' truncates the int value provided to 16 bits, it doesn't prevent the
 value being promoted from short to int in the first place.   And yes, all of 
 this is bizarre, and gcc could be lots smarter about (almost all of) these
 kinds of warnings ....

 kre

From: Valery Ushakov <uwe@stderr.spb.ru>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: toolchain/53618: bogus warnings from printf %u and ntohs with -O1
Date: Sat, 22 Sep 2018 01:12:47 +0300

 On Fri, Sep 21, 2018 at 20:35:01 +0000, Robert Elz wrote:

 > From: Robert Elz <kre@munnari.OZ.AU>
 > To: gnats-bugs@NetBSD.org
 > Cc: 
 > Subject: Re: toolchain/53618: bogus warnings from printf %u and ntohs with -O1
 > Date: Sat, 22 Sep 2018 03:33:35 +0700
 > 
 >      Date:        Fri, 21 Sep 2018 11:20:01 +0000 (UTC)
 >      From:        Valery Ushakov <uwe@stderr.spb.ru>
 >      Message-ID:  <20180921112001.6B4C07A1D2@mollari.NetBSD.org>
 >  
 >    |  Isn't that what the 'h' length modifier (i.e. "%hu") is for?
 >  
 >  No, 'h' truncates the int value provided to 16 bits, it doesn't
 >  prevent the value being promoted from short to int in the first
 >  place.

 Right, and the standard even says that explicitly:

   h            Specifies that a following d, i, o, u, x, or  X
                conversion  specifier applies to a short int or
                unsigned short int argument (the argument  will
                have  been  promoted  according  to the integer
                promotions, but its value shall be converted to
                short   int   or   unsigned  short  int  before
                printing);

 >  And yes, all of this is bizarre, and gcc could be lots
 >  smarter about (almost all of) these kinds of warnings ....

 Yeah.  I'm a bit too tired so I can't think of a situation where using
 'h' with unsigned short would affect the result.  But my point was
 that %hu seems technically the right thing to do and it does shut up
 gcc.

 -uwe

From: Robert Elz <kre@munnari.OZ.AU>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: toolchain/53618: bogus warnings from printf %u and ntohs with -O1
Date: Sat, 22 Sep 2018 06:37:33 +0700

     Date:        Fri, 21 Sep 2018 22:15:01 +0000 (UTC)
     From:        Valery Ushakov <uwe@stderr.spb.ru>
     Message-ID:  <20180921221501.725C37A202@mollari.NetBSD.org>

   |  But my point was that %hu seems technically the right thing to do and it
   | does shut up gcc.

 Agreed on the former (or at least, it is certainly technically acceptable),
 and if it has that effect (I haven't tested it) then that sounds like the ideal
 way to "fix" things.

 kre

Home	PR Database Search