NetBSD Problem Report #50852

From www@NetBSD.org  Fri Feb 26 10:54:47 2016
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 "Postmaster NetBSD.org" (verified OK))
	by mollari.NetBSD.org (Postfix) with ESMTPS id DB9B37ACCE
	for <gnats-bugs@gnats.NetBSD.org>; Fri, 26 Feb 2016 10:54:47 +0000 (UTC)
Message-Id: <20160226105447.073757ACDB@mollari.NetBSD.org>
Date: Fri, 26 Feb 2016 10:54:47 +0000 (UTC)
From: dcb314@hotmail.com
Reply-To: dcb314@hotmail.com
To: gnats-bugs@NetBSD.org
Subject: src/sys/arch/hp300/stand/common/ite.c:239: bad expression 
X-Send-Pr-Version: www-1.0

>Number:         50852
>Category:       port-hp300
>Synopsis:       src/sys/arch/hp300/stand/common/ite.c:239: bad expression
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    port-hp300-maintainer
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Fri Feb 26 10:55:00 +0000 2016
>Closed-Date:    Mon Feb 29 15:13:53 +0000 2016
>Last-Modified:  Mon Feb 29 15:13:53 +0000 2016
>Originator:     David Binderman
>Release:        cvs dated 20160226
>Organization:
>Environment:
>Description:
[src/sys/arch/hp300/stand/common/ite.c:239]: (error) Expression 'whichconsole=++whichconsole%(NITE+1)' depends on order of evaluation of side effects

Source code is

   whichconsole = ++whichconsole % (NITE+1);

Maybe better code

   whichconsole = (whichconsole + 1) % (NITE+1);

>How-To-Repeat:

>Fix:

>Release-Note:

>Audit-Trail:
From: Joerg Sonnenberger <joerg@britannica.bec.de>
To: gnats-bugs@NetBSD.org
Cc: port-hp300-maintainer@netbsd.org, gnats-admin@netbsd.org,
	netbsd-bugs@netbsd.org
Subject: Re: port-hp300/50852: src/sys/arch/hp300/stand/common/ite.c:239: bad
 expression
Date: Fri, 26 Feb 2016 13:58:36 +0100

 On Fri, Feb 26, 2016 at 10:55:00AM +0000, dcb314@hotmail.com wrote:
 > >Description:
 > [src/sys/arch/hp300/stand/common/ite.c:239]: (error) Expression 'whichconsole=++whichconsole%(NITE+1)' depends on order of evaluation of side effects
 > 
 > Source code is
 > 
 >    whichconsole = ++whichconsole % (NITE+1);
 > 
 > Maybe better code
 > 
 >    whichconsole = (whichconsole + 1) % (NITE+1);

 Independent of the question of which code is better, I don't think the
 error is correct. The location of the assignment (whichconsole) is not
 changed by the right operand of the =, so the evaluation of the left and
 right operand is invariant under evaluation order of the side effect.
 The side effect of *updating* whichconsole as part of the assignment is
 sequenced *after* the evaluation of the right operand (6.5.16 (3)
 sentence 4. As such, the value here is perfectly well defined according
 to the standard. This is quite different from an expression like
     *(++p) = *(++p); 

 Joerg

From: David Binderman <dcb314@hotmail.com>
To: "gnats-bugs@NetBSD.org" <gnats-bugs@netbsd.org>,
	"port-hp300-maintainer@netbsd.org" <port-hp300-maintainer@netbsd.org>,
	"gnats-admin@netbsd.org" <gnats-admin@netbsd.org>, "netbsd-bugs@netbsd.org"
	<netbsd-bugs@netbsd.org>
Cc: 
Subject: RE: port-hp300/50852: src/sys/arch/hp300/stand/common/ite.c:239:
 bad expression
Date: Fri, 26 Feb 2016 16:45:21 +0000

 Hello there=2C=0A=
 =0A=
 ----------------------------------------=0A=
 > On Fri=2C Feb 26=2C 2016 at 10:55:00AM +0000=2C dcb314@hotmail.com wrote:=
 =0A=
 >>>Description:=0A=
 >> [src/sys/arch/hp300/stand/common/ite.c:239]: (error) Expression 'whichco=
 nsole=3D++whichconsole%(NITE+1)' depends on order of evaluation of side eff=
 ects=0A=
 >>=0A=
 >> Source code is=0A=
 >>=0A=
 >> whichconsole =3D ++whichconsole % (NITE+1)=3B=0A=
 >>=0A=
 >> Maybe better code=0A=
 >>=0A=
 >> whichconsole =3D (whichconsole + 1) % (NITE+1)=3B=0A=
 >=0A=
 > Independent of the question of which code is better=2C I don't think the=
 =0A=
 > error is correct. =0A=
 =0A=
 False=2C the error is valid.=0A=
 =0A=
 There are two writes to whichconsole=2C one on the left of the assignment=
 =2C=0A=
 one on the right.=0A=
 =0A=
 The ISO standard doesn't define which order those writes occur.=0A=
 See Steve Summit's C FAQ for sequence points=2C section 3.8.=0A=
 =0A=
 =0A=
 Regards=0A=
 =0A=
 David Binderman=0A=
 =0A=
 31 years a C programmer=2C including C and C++ compiler development.=0A=
  		 	   		  =

From: "Christos Zoulas" <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/50852 CVS commit: src/sys/arch/hp300/stand/common
Date: Fri, 26 Feb 2016 13:11:11 -0500

 Module Name:	src
 Committed By:	christos
 Date:		Fri Feb 26 18:11:11 UTC 2016

 Modified Files:
 	src/sys/arch/hp300/stand/common: ite.c

 Log Message:
 PR/50852: David Binderman: make code more readable (although it was correct
 anyway)


 To generate a diff of this commit:
 cvs rdiff -u -r1.17 -r1.18 src/sys/arch/hp300/stand/common/ite.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: Joerg Sonnenberger <joerg@britannica.bec.de>
To: David Binderman <dcb314@hotmail.com>
Cc: "gnats-bugs@NetBSD.org" <gnats-bugs@netbsd.org>,
	"port-hp300-maintainer@netbsd.org" <port-hp300-maintainer@netbsd.org>,
	"gnats-admin@netbsd.org" <gnats-admin@netbsd.org>,
	"netbsd-bugs@netbsd.org" <netbsd-bugs@netbsd.org>
Subject: Re: port-hp300/50852: src/sys/arch/hp300/stand/common/ite.c:239: bad
 expression
Date: Fri, 26 Feb 2016 21:24:14 +0100

 On Fri, Feb 26, 2016 at 04:45:21PM +0000, David Binderman wrote:
 > Hello there,
 > 
 > ----------------------------------------
 > > On Fri, Feb 26, 2016 at 10:55:00AM +0000, dcb314@hotmail.com wrote:
 > >>>Description:
 > >> [src/sys/arch/hp300/stand/common/ite.c:239]: (error) Expression 'whichconsole=++whichconsole%(NITE+1)' depends on order of evaluation of side effects
 > >>
 > >> Source code is
 > >>
 > >> whichconsole = ++whichconsole % (NITE+1);
 > >>
 > >> Maybe better code
 > >>
 > >> whichconsole = (whichconsole + 1) % (NITE+1);
 > >
 > > Independent of the question of which code is better, I don't think the
 > > error is correct. 
 > 
 > False, the error is valid.
 > 
 > There are two writes to whichconsole, one on the left of the assignment,
 > one on the right.
 > 
 > The ISO standard doesn't define which order those writes occur.
 > See Steve Summit's C FAQ for sequence points, section 3.8.

 You are wrong. There is no write from the left operand of the
 assignment. The write is the result of the assignment and as written
 before is explicitly sequenced by the standard.

 Joerg

From: Dave Huang <khym@azeotrope.org>
To: Joerg Sonnenberger <joerg@britannica.bec.de>
Cc: "gnats-bugs@NetBSD.org" <gnats-bugs@netbsd.org>,
	"gnats-admin@netbsd.org" <gnats-admin@netbsd.org>,
	"netbsd-bugs@netbsd.org" <netbsd-bugs@netbsd.org>
Subject: Re: port-hp300/50852: src/sys/arch/hp300/stand/common/ite.c:239: bad
 expression
Date: Fri, 26 Feb 2016 17:04:46 -0600

 On Fri, Feb 26, 2016 at 09:24:14PM +0100, Joerg Sonnenberger wrote:
 > On Fri, Feb 26, 2016 at 04:45:21PM +0000, David Binderman wrote:
 > > >> Source code is
 > > >>
 > > >> whichconsole = ++whichconsole % (NITE+1);
 > > >>
 > > >> Maybe better code
 > > >>
 > > >> whichconsole = (whichconsole + 1) % (NITE+1);
 > > >
 > > > Independent of the question of which code is better, I don't think the
 > > > error is correct. 
 > > 
 > > False, the error is valid.
 > > 
 > > There are two writes to whichconsole, one on the left of the assignment,
 > > one on the right.
 > > 
 > > The ISO standard doesn't define which order those writes occur.
 > > See Steve Summit's C FAQ for sequence points, section 3.8.
 > 
 > You are wrong. There is no write from the left operand of the
 > assignment. The write is the result of the assignment and as written
 > before is explicitly sequenced by the standard.

 How is the code in question different from i = ++i;, which I've always
 been told is undefined? The assignment operator does not introduce a
 sequence point.

 gcc thinks it's wrong too:

 % cat t.c
 #define NITE 4

 int main(void)
 {
   int whichconsole = 0;
   whichconsole = ++whichconsole % (NITE+1);
   return 0;
 }

 % cc -Wall t.c
 t.c: In function ‘main’:
 t.c:6:16: warning: operation on ‘whichconsole’ may be undefined [-Wsequence-point]
    whichconsole = ++whichconsole % (NITE+1);
                 ^

From: Joerg Sonnenberger <joerg@britannica.bec.de>
To: Dave Huang <khym@azeotrope.org>
Cc: "gnats-bugs@NetBSD.org" <gnats-bugs@netbsd.org>,
	"gnats-admin@netbsd.org" <gnats-admin@netbsd.org>,
	"netbsd-bugs@netbsd.org" <netbsd-bugs@netbsd.org>
Subject: Re: port-hp300/50852: src/sys/arch/hp300/stand/common/ite.c:239: bad
 expression
Date: Sat, 27 Feb 2016 00:10:21 +0100

 On Fri, Feb 26, 2016 at 05:04:46PM -0600, Dave Huang wrote:
 > On Fri, Feb 26, 2016 at 09:24:14PM +0100, Joerg Sonnenberger wrote:
 > > On Fri, Feb 26, 2016 at 04:45:21PM +0000, David Binderman wrote:
 > > > >> Source code is
 > > > >>
 > > > >> whichconsole = ++whichconsole % (NITE+1);
 > > > >>
 > > > >> Maybe better code
 > > > >>
 > > > >> whichconsole = (whichconsole + 1) % (NITE+1);
 > > > >
 > > > > Independent of the question of which code is better, I don't think the
 > > > > error is correct. 
 > > > 
 > > > False, the error is valid.
 > > > 
 > > > There are two writes to whichconsole, one on the left of the assignment,
 > > > one on the right.
 > > > 
 > > > The ISO standard doesn't define which order those writes occur.
 > > > See Steve Summit's C FAQ for sequence points, section 3.8.
 > > 
 > > You are wrong. There is no write from the left operand of the
 > > assignment. The write is the result of the assignment and as written
 > > before is explicitly sequenced by the standard.
 > 
 > How is the code in question different from i = ++i;, which I've always
 > been told is undefined? The assignment operator does not introduce a
 > sequence point.

 At least in C11, it does.

 Joerg

From: Dave Huang <khym@azeotrope.org>
To: Joerg Sonnenberger <joerg@britannica.bec.de>
Cc: "gnats-bugs@NetBSD.org" <gnats-bugs@netbsd.org>,
	"gnats-admin@netbsd.org" <gnats-admin@netbsd.org>,
	"netbsd-bugs@netbsd.org" <netbsd-bugs@netbsd.org>
Subject: Re: port-hp300/50852: src/sys/arch/hp300/stand/common/ite.c:239: bad
 expression
Date: Fri, 26 Feb 2016 17:40:11 -0600

 On Sat, Feb 27, 2016 at 12:10:21AM +0100, Joerg Sonnenberger wrote:
 > On Fri, Feb 26, 2016 at 05:04:46PM -0600, Dave Huang wrote:
 > > How is the code in question different from i = ++i;, which I've always
 > > been told is undefined? The assignment operator does not introduce a
 > > sequence point.
 > 
 > At least in C11, it does.

 Hmm, well I'm no language lawyer, but footnote 84 in section 6.5 of
 http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf (the C1X
 draft) says "This paragraph renders undefined statement expressions
 such as i = ++i + 1;"

 However, I did see someone say that i = ++i + 1 is well-defined
 starting in *C++* 11.

From: Joerg Sonnenberger <joerg@britannica.bec.de>
To: Dave Huang <khym@azeotrope.org>
Cc: "gnats-bugs@NetBSD.org" <gnats-bugs@netbsd.org>,
	"gnats-admin@netbsd.org" <gnats-admin@netbsd.org>,
	"netbsd-bugs@netbsd.org" <netbsd-bugs@netbsd.org>
Subject: Re: port-hp300/50852: src/sys/arch/hp300/stand/common/ite.c:239: bad
 expression
Date: Sat, 27 Feb 2016 08:44:19 +0100

 On Fri, Feb 26, 2016 at 05:40:11PM -0600, Dave Huang wrote:
 > On Sat, Feb 27, 2016 at 12:10:21AM +0100, Joerg Sonnenberger wrote:
 > > On Fri, Feb 26, 2016 at 05:04:46PM -0600, Dave Huang wrote:
 > > > How is the code in question different from i = ++i;, which I've always
 > > > been told is undefined? The assignment operator does not introduce a
 > > > sequence point.
 > > 
 > > At least in C11, it does.
 > 
 > Hmm, well I'm no language lawyer, but footnote 84 in section 6.5 of
 > http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf (the C1X
 > draft) says "This paragraph renders undefined statement expressions
 > such as i = ++i + 1;"

 I think that's one is just a left-over from earlier versions, since
 sequencing rules are explicit for that case. The second example
 "a[i++]=i" on the other hand is still undefined.

 Joerg

From: David Binderman <dcb314@hotmail.com>
To: "gnats-bugs@NetBSD.org" <gnats-bugs@netbsd.org>,
	"port-hp300-maintainer@netbsd.org" <port-hp300-maintainer@netbsd.org>,
	"gnats-admin@netbsd.org" <gnats-admin@netbsd.org>, "netbsd-bugs@netbsd.org"
	<netbsd-bugs@netbsd.org>
Cc: 
Subject: RE: port-hp300/50852: src/sys/arch/hp300/stand/common/ite.c:239:
 bad expression
Date: Sat, 27 Feb 2016 07:52:56 +0000

 Hello there=2C=0A=
 =0A=
 ----------------------------------------=0A=
 > However=2C I did see someone say that i =3D ++i + 1 is well-defined=0A=
 > starting in *C++* 11.=0A=
 =0A=
 The code in question is C code. Let's leave what happens in C++ aside=2C=0A=
 since it is only muddying the issue. =0A=
 =0A=
 Let's also leave aside what happens in various C language versions and conc=
 entrate =0A=
 on the language version the compiler uses for the netBSD code.=0A=
 =0A=
 =0A=
 $ more feb27b.c=0A=
 =0A=
 #define NITE 4=0A=
 =0A=
 int=0A=
 main()=0A=
 {=0A=
 =A0=A0=A0 int whichconsole =3D 0=3B=0A=
 =A0=A0=A0 whichconsole =3D ++whichconsole % (NITE+1)=3B=0A=
 =A0=A0=A0 return 0=3B=0A=
 }=0A=
 =0A=
 First=2C with a recent version of clang=2C a well known C compiler.=0A=
 =0A=
 $ ~/llvm/results/bin/clang -c -g -O2 -Wall -Wextra feb27b.c=0A=
 feb27b.c:8:17: warning: multiple unsequenced modifications to 'whichconsole=
 '=0A=
 =A0=A0=A0=A0=A0 [-Wunsequenced]=0A=
 =A0=A0=A0=A0=A0=A0=A0 whichconsole =3D ++whichconsole % (NITE+1)=3B=0A=
 =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 ~ ^=0A=
 1 warning generated.=0A=
 =0A=
 Next a recent version of gcc.=0A=
 =0A=
 $ ~/gcc/results/bin/gcc -c -g -O2 -Wall -Wextra feb27b.c=0A=
 feb27b.c: In function =91main=92:=0A=
 feb27b.c:8:15: warning: operation on =91whichconsole=92 may be undefined [-=
 Wsequence-point]=0A=
 =A0 whichconsole =3D ++whichconsole % (NITE+1)=3B=0A=
 =A0 ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~=0A=
 $ =0A=
 =0A=
 Two different compilers=2C in their default language standard=2C think the =
 code falls=0A=
 foul of the C sequence point rules. =0A=
 =0A=
 I think it is *much* more likely that the original code is wrong than two s=
 eparate compilers=0A=
 have the same bug in them.=0A=
 =0A=
 =0A=
 Regards=0A=
 =0A=
 David Binderman=0A=
  		 	   		  =

From: Joerg Sonnenberger <joerg@britannica.bec.de>
To: David Binderman <dcb314@hotmail.com>
Cc: "gnats-bugs@NetBSD.org" <gnats-bugs@netbsd.org>,
	"port-hp300-maintainer@netbsd.org" <port-hp300-maintainer@netbsd.org>,
	"gnats-admin@netbsd.org" <gnats-admin@netbsd.org>,
	"netbsd-bugs@netbsd.org" <netbsd-bugs@netbsd.org>
Subject: Re: port-hp300/50852: src/sys/arch/hp300/stand/common/ite.c:239: bad
 expression
Date: Sat, 27 Feb 2016 19:36:09 +0100

 On Sat, Feb 27, 2016 at 07:52:56AM +0000, David Binderman wrote:
 > Hello there,
 > 
 > ----------------------------------------
 > > However, I did see someone say that i = ++i + 1 is well-defined
 > > starting in *C++* 11.
 > 
 > The code in question is C code. Let's leave what happens in C++ aside,
 > since it is only muddying the issue. 

 I never talked about C++. I explicitly talke about C11.

 > Let's also leave aside what happens in various C language versions and concentrate 
 > on the language version the compiler uses for the netBSD code.
 > 
 > 
 > $ more feb27b.c
 > 
 > #define NITE 4
 > 
 > int
 > main()
 > {
 >     int whichconsole = 0;
 >     whichconsole = ++whichconsole % (NITE+1);
 >     return 0;
 > }
 > 
 > First, with a recent version of clang, a well known C compiler.
 > 
 > $ ~/llvm/results/bin/clang -c -g -O2 -Wall -Wextra feb27b.c
 > feb27b.c:8:17: warning: multiple unsequenced modifications to 'whichconsole'
 >       [-Wunsequenced]
 >         whichconsole = ++whichconsole % (NITE+1);
 >                      ~ ^
 > 1 warning generated.
 > 
 > Next a recent version of gcc.
 > 
 > $ ~/gcc/results/bin/gcc -c -g -O2 -Wall -Wextra feb27b.c
 > feb27b.c: In function ‘main’:
 > feb27b.c:8:15: warning: operation on ‘whichconsole’ may be undefined [-Wsequence-point]
 >   whichconsole = ++whichconsole % (NITE+1);
 >   ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
 > $ 
 > 
 > Two different compilers, in their default language standard, think the code falls
 > foul of the C sequence point rules. 
 > 
 > I think it is *much* more likely that the original code is wrong than two separate compilers
 > have the same bug in them.

 Actually, it is quite likely that the specific warnings for the sequence
 point rules just haven't been updated. You keep ignoring the explicit
 reference to the standard I gave. It is quite explicit that a sequence
 point exists. Ironically, for the pre-increment version, it is highly
 likely that no sane compiler ever did not do the correct thing, but
 that's beside the point.

 Joerg

From: David Binderman <dcb314@hotmail.com>
To: "gnats-bugs@NetBSD.org" <gnats-bugs@netbsd.org>,
	"port-hp300-maintainer@netbsd.org" <port-hp300-maintainer@netbsd.org>,
	"gnats-admin@netbsd.org" <gnats-admin@netbsd.org>, "netbsd-bugs@netbsd.org"
	<netbsd-bugs@netbsd.org>
Cc: 
Subject: RE: port-hp300/50852: src/sys/arch/hp300/stand/common/ite.c:239:
 bad expression
Date: Sun, 28 Feb 2016 17:48:54 +0000

 Hello there=2C=0A=
 =0A=
 ----------------------------------------=0A=
 > I never talked about C++. I explicitly talke about C11.=0A=
 =0A=
 I can't seem to find out which version of the language standard the code=0A=
 in question in port-hp300 gets compiled to.=0A=
 =0A=
 There certainly seems to be a lot (> 200=2C000) of uses of -std=3Dgnu99 in=
 =0A=
 a recent x86 build. All other language settings seem to get mentioned=0A=
 < 500 times=2C so to odds are high source code file ite.c gets compiled wit=
 h -std=3Dgnu99.=0A=
 =0A=
 Once we've got that=2C and then we've agreed what the three language=0A=
 standards do in this area=2C (C90=2C C99=2C C11)=2C then we might be able t=
 o=0A=
 agree *finally* whether the code is in error.=0A=
 =0A=
 Of course=2C the reliable programmer makes it work reliably in all versions=
  of C.=0A=
 =0A=
 The code is certainly in error in C90=2C maybe C99=2C maybe not C11.=0A=
 =A0=0A=
 I had a look at=0A=
 =0A=
 http://en.cppreference.com/w/c/language/eval_order=0A=
 =0A=
 but I couldn't make much sense of it. I think the Rules section might=0A=
 have the nitty gritty.=0A=
 =0A=
 =0A=
 Regards=0A=
 =0A=
 David Binderman=0A=
  		 	   		  =

From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: port-hp300/50852: src/sys/arch/hp300/stand/common/ite.c:239: bad
 expression
Date: Mon, 29 Feb 2016 15:09:10 +0000

 On Sat, Feb 27, 2016 at 06:40:01PM +0000, Joerg Sonnenberger wrote:
  >  Ironically, for the pre-increment version, it is highly
  >  likely that no sane compiler ever did not do the correct thing, but
  >  that's beside the point.

 That's not true. A reasonable implementation of pre/post-increment
 effects is to write them back at the next sequence point, so you'd get

    t1 = whichconsole + 1
    t2 = whichconsole + 1
    t3 = t2 % (NITE+1)
    whichconsole = t3
    whichconsole = t1

 which after further obvious simplifications turns into

    whichconsole = whichconsole + 1

 aka The Wrong Thing.

 Anyway, regardless of whether you're channeling Dan Pop it's all
 irrelevant; code of this form should not be allowed to exist.

 -- 
 David A. Holland
 dholland@netbsd.org

State-Changed-From-To: open->closed
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Mon, 29 Feb 2016 15:13:53 +0000
State-Changed-Why:
Christos fixed it, thanks.


>Unformatted:

Home	PR Database Search