clang (i386) compiles ports/devel/dev86/.../as/genlist.c:build_number() to an infinite loop when optimization -O2 is turned on. Fix: Fix to clang unknown. clang generates correct code when "power" is made volatile. ==== devel/dev86 port patch ==== ...keith--l3mbzhIP1InxoAHyWs7i4Aa7ayD9UtrcBNNWPV4J1SDExNoH Content-Type: text/plain; name="file.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="file.diff" --- dev86-0.16.18/as/genlist.c.orig 2001-06-23 16:13:19.000000000 -0400 +++ dev86-0.16.18/as/genlist.c 2012-12-06 12:11:26.000000000 -0500 @@ -113,7 +113,7 @@ { static unsigned powers_of_10[] = {1, 10, 100, 1000, 10000,}; unsigned char digit; - unsigned char power; + volatile unsigned char power; register unsigned power_of_10; power = 5; /* actually 1 more than power */ ==== How-To-Repeat: Extract build_number() from ports/devel/dev86/.../as/genlist.c === bn.c === PUBLIC char *build_number(num, width, where) unsigned num; unsigned width; register char *where; { static unsigned powers_of_10[] = {1, 10, 100, 1000, 10000,}; unsigned char digit; unsigned char power; register unsigned power_of_10; power = 5; /* actually 1 more than power */ do { for (digit = '0', power_of_10 = (powers_of_10 - 1)[power]; num >= power_of_10; num -= power_of_10) ++digit; if (power <= width) *where++ = digit; } while (--power != 0); return where; } ============ clang -m32 --version FreeBSD clang version 3.2 (branches/release_32 168974) 20121130 Target: i386-unknown-freebsd10.0 Thread model: posix clang -O2 -DPUBLIC="" -m32 -c -save-temps bn.c cat bn.s ... build_number: # @build_number # BB#0: # %for.body.lr.ph .align 16, 0x90 .LBB0_1: # %for.body # =>This Inner Loop Header: Depth=1 jmp .LBB0_1 ...
Responsible Changed From-To: freebsd-bugs->freebsd-ports-bugs make this a ports PR.
Responsible Changed From-To: freebsd-ports-bugs->miwi miwi@ wants this submitter's PRs (via the GNATS Auto Assign Tool)
State Changed From-To: open->feedback Awaiting maintainers feedback (via the GNATS Auto Assign Tool)
Maintainer of devel/dev86, Please note that PR ports/174240 has just been submitted. If it contains a patch for an upgrade, an enhancement or a bug fix you agree on, reply to this email stating that you approve the patch and a committer will take care of it. The full text of the PR can be found at: http://www.freebsd.org/cgi/query-pr.cgi?pr=ports/174240 -- Edwin Groothuis via the GNATS Auto Assign Tool edwin@FreeBSD.org
The array access to powers_of_10 in build_number() is undefined, so the loop is optimized away. The fix is to change the expression: power_of_10 = (powers_of_10 - 1)[power] into: power_of_10 = powers_of_10[power - 1] Attached diff makes it so.
> The array access to powers_of_10 in build_number() is undefined, so the > loop is optimized away. The fix is to change the expression: > > power_of_10 = (powers_of_10 - 1)[power] > > into: > > power_of_10 = powers_of_10[power - 1] > > Attached diff makes it so. Thanks, it's not my code. Surely clang should generate something even if the expression is undefined? Compare the difference between the code generated for: *(powers_of_10 - 1 + power) and "arithmetically equivalent" *(powers_of_10 + power - 1) or (powers_of_10 + power)[-1] [Tested on arm, i386. Working code is generated on amd64.] ...keith
On Mar 10, 2013, at 21:46 , kwhite@site.uottawa.ca wrote: >> The array access to powers_of_10 in build_number() is undefined, so = the >> loop is optimized away. The fix is to change the expression: >>=20 >> power_of_10 =3D (powers_of_10 - 1)[power] >>=20 >> into: >>=20 >> power_of_10 =3D powers_of_10[power - 1] >>=20 >> Attached diff makes it so. >=20 > Thanks, it's not my code. >=20 > Surely clang should generate something even if the expression is = undefined? No, if the behaviour is undefined, the compiler may insert anything it = wants. Including code to wipe your root device, launch the nuclear = missiles, etc. :-) In practice, it will either simply optimize away the "impossible" code, = or insert ud2 instructions, which cause the program to raise SIGILL at = runtime. Another possibility, which unfortunately occurs frequently, = with many compilers, is that it emits code which *seems* to work = properly. But it could break under any number of (sometimes very = subtle) circumstances. An interesting read about undefined behaviour can be found here: = http://blog.regehr.org/archives/213
On Sun, 10 Mar 2013, Dimitry Andric wrote: > On Mar 10, 2013, at 21:46 , kwhite@site.uottawa.ca wrote: >>> The array access to powers_of_10 in build_number() is undefined, so the >>> loop is optimized away. The fix is to change the expression: >>> >>> power_of_10 = (powers_of_10 - 1)[power] >>> >>> into: >>> >>> power_of_10 = powers_of_10[power - 1] >>> >>> Attached diff makes it so. >> >> Thanks, it's not my code. >> >> Surely clang should generate something even if the expression is undefined? > > No, if the behaviour is undefined, the compiler may insert anything it wants. Including code to wipe your root device, launch the nuclear missiles, etc. :-) > ... Indeed, and without warm and fuzzy warnings with "-Wall" when you compile it! There's a subtle difference between: "powers_of_ten + power - 1" and "powers_of_ten - 1 + power" [Those darned association rules. Ouch.] Thanks for your time. ...keith
Author: miwi Date: Thu Mar 28 13:58:05 2013 New Revision: 315469 URL: http://svnweb.freebsd.org/changeset/ports/315469 Log: - Fix build with clang PR: 174240 Submitted by: Keith White <kwhite@uottawa.ca> Approved by: maintainer Added: head/devel/dev86/files/patch-as__genlist.c (contents, props changed) Added: head/devel/dev86/files/patch-as__genlist.c ============================================================================== --- /dev/null 00:00:00 1970 (empty, because file is newly added) +++ head/devel/dev86/files/patch-as__genlist.c Thu Mar 28 13:58:05 2013 (r315469) @@ -0,0 +1,11 @@ +--- as/genlist.c.orig 2001-06-23 22:13:19.000000000 +0200 ++++ as/genlist.c 2013-03-10 18:40:54.000000000 +0100 +@@ -119,7 +119,7 @@ register char *where; + power = 5; /* actually 1 more than power */ + do + { +- for (digit = '0', power_of_10 = (powers_of_10 - 1)[power]; ++ for (digit = '0', power_of_10 = powers_of_10[power - 1]; + num >= power_of_10; num -= power_of_10) + ++digit; + if (power <= width) _______________________________________________ svn-ports-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-ports-all To unsubscribe, send any mail to "svn-ports-all-unsubscribe@freebsd.org"
State Changed From-To: feedback->closed Committed. Thanks!