Bug 174240 - clang generates incorrect code for ports/devel/dev86 in i386 mode
Summary: clang generates incorrect code for ports/devel/dev86 in i386 mode
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: Normal Affects Only Me
Assignee: Martin Wilke
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-12-06 17:50 UTC by Keith White
Modified: 2013-03-28 14:00 UTC (History)
0 users

See Also:


Attachments
devel__dev86-1.diff (675 bytes, patch)
2013-03-10 18:43 UTC, Dimitry Andric
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith White 2012-12-06 17:50:00 UTC
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
	...
Comment 1 Mark Linimon freebsd_committer freebsd_triage 2013-03-10 05:10:38 UTC
Responsible Changed
From-To: freebsd-bugs->freebsd-ports-bugs

make this a ports PR.
Comment 2 Edwin Groothuis freebsd_committer 2013-03-10 05:12:03 UTC
Responsible Changed
From-To: freebsd-ports-bugs->miwi

miwi@ wants this submitter's PRs (via the GNATS Auto Assign Tool)
Comment 3 Edwin Groothuis freebsd_committer 2013-03-10 05:12:04 UTC
State Changed
From-To: open->feedback

Awaiting maintainers feedback (via the GNATS Auto Assign Tool)
Comment 4 Edwin Groothuis freebsd_committer 2013-03-10 05:12:04 UTC
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
Comment 5 Dimitry Andric freebsd_committer 2013-03-10 18:43:21 UTC
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.
Comment 6 Keith White 2013-03-10 20:46:41 UTC
> 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
Comment 7 Dimitry Andric freebsd_committer 2013-03-10 21:12:46 UTC
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
Comment 8 Keith White 2013-03-10 22:05:40 UTC
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
Comment 9 dfilter service freebsd_committer 2013-03-28 13:58:24 UTC
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"
Comment 10 Martin Wilke freebsd_committer 2013-03-28 13:58:36 UTC
State Changed
From-To: feedback->closed

Committed. Thanks!