Bug 16176 - [3.4 Regression] Miscompilation of unaligned data in MIPS backend (SB1 flavor)
Summary: [3.4 Regression] Miscompilation of unaligned data in MIPS backend (SB1 flavor)
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 3.4.0
: P1 normal
Target Milestone: 3.4.1
Assignee: rsandifo@gcc.gnu.org
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2004-06-24 15:38 UTC by Paul Koning
Modified: 2005-07-23 22:49 UTC (History)
2 users (show)

See Also:
Host: i386-netbsd
Target: mips64el-elf
Build: i386-netbsd
Known to work: 3.3.3 4.0.0
Known to fail:
Last reconfirmed:


Attachments
Proposed patch (854 bytes, patch)
2004-06-24 22:19 UTC, rsandifo@gcc.gnu.org
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Paul Koning 2004-06-24 15:38:24 UTC
Configure spec:
Configured with: /p3/jim/GCC/gcc-3.4.0/configure
--prefix=/usr/local/EQLGCC_v340/mips64el --enable-languages=c,c++
--target=mips64el-elf --with-newlib --with-arch=sb1 --with-tune=sb1
--with-gnu-as --with-gnu-ld
Thread model: single

The following trivial test program is miscompiled in a way that causes a crash:

struct foo
{
    short x;
    struct foo *p __attribute__((packed));
    short z;
};

struct foo *list;

int walklist(struct foo *p)
{
    int t = 0;
    while (p)
    {
        t += p->x;
        p = p->p;
    }
    return t;
}

The generated code looks like this:

	.file	1 "test.c"
	.section .mdebug.abiO64
	.previous
	.text
	.align	2
	.globl	walklist
	.ent	walklist
walklist:
	.frame	$sp,0,$31		# vars= 0, regs= 0/0, args= 0, gp= 0
	.mask	0x00000000,0
	.fmask	0x00000000,0
	.set	noreorder
	.set	nomacro
	
	beq	$4,$0,$L6
	move	$3,$0

$L4:
	lh	$2,0($4)
	lwl	$4,5($4)
	lwr	$4,2($4)
	bne	$4,$0,$L4
	addu	$3,$3,$2

$L6:
	j	$31
	move	$2,$3

	.set	macro
	.set	reorder
	.end	walklist

	.comm	list,4,4

The error is that an unaligned load is a two-instruction sequence, so you CANNOT
use the same register as source and destination.  The code as shown will crash
when it comes to the null pointer at the end of the list.

The above is with -O1; with -O2 the code is reordered slightly but the code is
wrong in the same way.
Comment 1 Paul Koning 2004-06-24 15:42:31 UTC
The processor selection doesn't actually matter; I tried the compile with
march/mtune set to sr71000, r5000, rm9000, and r4000; all generate the same
broken code.
Comment 2 Paul Koning 2004-06-24 17:24:05 UTC
GCC 3.4.1 in the 20040618 snapshot has the same bug.
Comment 3 rsandifo@gcc.gnu.org 2004-06-24 18:19:10 UTC
Mea culpa.  Working on a fix.
Comment 4 rsandifo@gcc.gnu.org 2004-06-24 22:19:09 UTC
Created attachment 6624 [details]
Proposed patch

Please try the attached patch.	I'll test it tomorrow (once the regression
tests for 16144 are complete).
Comment 5 CVS Commits 2004-06-25 18:25:08 UTC
Subject: Bug 16176

CVSROOT:	/cvs/gcc
Module name:	gcc
Changes by:	rsandifo@gcc.gnu.org	2004-06-25 18:24:52

Modified files:
	gcc            : ChangeLog 
	gcc/config/mips: mips.c 
	gcc/testsuite  : ChangeLog 
Added files:
	gcc/testsuite/gcc.c-torture/execute: 20040625-1.c 

Log message:
	PR target/16176
	* config/mips/mips.c (mips_expand_unaligned_load): Use a temporary
	register for the destination of the lwl or ldl.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&r1=2.4145&r2=2.4146
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/config/mips/mips.c.diff?cvsroot=gcc&r1=1.421&r2=1.422
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/ChangeLog.diff?cvsroot=gcc&r1=1.3898&r2=1.3899
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/gcc.c-torture/execute/20040625-1.c.diff?cvsroot=gcc&r1=NONE&r2=1.1

Comment 6 Paul Koning 2004-06-25 18:36:53 UTC
Sorry for the delayed response.  This patch appears to fix the problem.
Comment 7 rsandifo@gcc.gnu.org 2004-06-25 18:42:49 UTC
Patch applied:

    http://gcc.gnu.org/ml/gcc-patches/2004-06/msg02085.html

Mark, you know what I'm going to ask ;)  Is this OK for 3.4.1?  Or should
it wait until 3.4.2?

Perhaps the patch isn't quite as obvious as the one you approved earlier
today.  But IMO it _is_ still obvious.  And safe.  The bug here is also
much more serious, since it only shows up at run time (rather than
link time).

I suppose every patch carries some amount of risk.  But like I say,
I think the risk in this case is very small, and I think it's obvious
that the new code is theoretically more correct.  From that POV,
I'd imagine the risk is more of introducing an ICE than introducing
another wrong code bug.

Err... couldn't decide whether to delete that last paragraph or not ;)
Comment 8 Mark Mitchell 2004-06-25 18:53:25 UTC
Subject: Re:  [3.4 Regression] Miscompilation of unaligned
 data in MIPS backend (SB1 flavor)

rsandifo at gcc dot gnu dot org wrote:

> ------- Additional Comments From rsandifo at gcc dot gnu dot org  2004-06-25 18:42 -------
> Patch applied:
> 
>     http://gcc.gnu.org/ml/gcc-patches/2004-06/msg02085.html
> 
> Mark, you know what I'm going to ask ;)  Is this OK for 3.4.1?  Or should
> it wait until 3.4.2?

OK for 3.4.1.

Comment 9 CVS Commits 2004-06-28 13:58:56 UTC
Subject: Bug 16176

CVSROOT:	/cvs/gcc
Module name:	gcc
Branch: 	gcc-3_4-branch
Changes by:	rsandifo@gcc.gnu.org	2004-06-28 13:58:45

Modified files:
	gcc            : ChangeLog 
	gcc/config/mips: mips.c 
	gcc/testsuite  : ChangeLog 
Added files:
	gcc/testsuite/gcc.c-torture/execute: 20040625-1.c 

Log message:
	PR target/16176
	* config/mips/mips.c (mips_expand_unaligned_load): Use a temporary
	register for the destination of the lwl or ldl.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=2.2326.2.524&r2=2.2326.2.525
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/config/mips/mips.c.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=1.362.4.12&r2=1.362.4.13
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/ChangeLog.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=1.3389.2.217&r2=1.3389.2.218
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/gcc.c-torture/execute/20040625-1.c.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=NONE&r2=1.1.2.1

Comment 10 rsandifo@gcc.gnu.org 2004-06-28 14:00:33 UTC
Patch applied to 3.4.