Bug 25199 - crashing code output from -mtune=pentium4 but not -mtune=pentiumpro
Summary: crashing code output from -mtune=pentium4 but not -mtune=pentiumpro
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 4.0.2
: P3 normal
Target Milestone: 4.0.3
Assignee: Jakub Jelinek
URL: http://gcc.gnu.org/ml/gcc-patches/200...
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2005-12-01 10:25 UTC by Caolan McNamara
Modified: 2008-08-29 07:22 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Known to work: 4.1.0
Known to fail: 4.0.2
Last reconfirmed: 2005-12-01 14:55:58


Attachments
preprocessed code (129.48 KB, application/x-bzip)
2005-12-01 10:27 UTC, Caolan McNamara
Details
ok output (90.66 KB, application/x-bzip)
2005-12-01 10:27 UTC, Caolan McNamara
Details
not ok output (89.73 KB, application/x-bzip)
2005-12-01 10:28 UTC, Caolan McNamara
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Caolan McNamara 2005-12-01 10:25:40 UTC
A fairly tricky one. OOo compiled with -mtune=pentium4 will crash e.g. http://qa.openoffice.org/issues/show_bug.cgi?id=58368 eventually on drawing stuff. Problem traced to a particular file. Compied with -mtune=pentiumpro instead and crash dissappears.

So taking -E code, attached here as salgdi.ii, comiling as 

a)
g++ -S -Os -fno-strict-aliasing -fvisibility=hidden -mtune=pentiumpro -fvisibility-inlines-hidden -g -fno-exceptions -fpic -o pentiumpro-salgdi.s salgdi.ii
g++ -c -fpic -o ../../../unxlngi6.pro/slo/salgdi.o pentiumpro-salgdi.s

and all is well, and as 

b)
++ -S -Os -fno-strict-aliasing -fvisibility=hidden -mtune=pentium4 -fvisibility-inlines-hidden -g -fno-exceptions -fpic -o pentium4-salgdi.s salgdi.ii
g++ -c -fpic -o ../../../unxlngi6.pro/slo/salgdi.o pentium4-salgdi.s

all is not well. 

.s also attached in an attempt to be useful
Comment 1 Caolan McNamara 2005-12-01 10:27:12 UTC
Created attachment 10374 [details]
preprocessed code
Comment 2 Caolan McNamara 2005-12-01 10:27:38 UTC
Created attachment 10375 [details]
ok output
Comment 3 Caolan McNamara 2005-12-01 10:28:10 UTC
Created attachment 10376 [details]
not ok output
Comment 4 Caolan McNamara 2005-12-01 10:28:53 UTC
crash is 

X11SalGraphics::SetXORMode (this=0x34b3f10, bSet=0 '\0')
    at /home/caolan/ooo/SRC680_m142/vcl/unx/source/gdi/salgdi.cxx:695
695         if( !bXORMode_ == bSet )

where a bitfield is compare to unsigned char
Comment 5 Jakub Jelinek 2005-12-01 14:38:15 UTC
Finding a bug in 1M of assembly is really hard without knowing where exactly
to look at.  Nevertheless, I suspect:
movl    237(%edi), %esi
in _ZN14X11SalGraphics10SetXORModeEh
(whereas -mpentiumpro .s has
movb    237(%esi), %al
).  I haven't verified in detail, but it looks possible the 32bit bitfield
(out of which only 14 bits are occupied) might be the last thing in the
class, so if the bitfield happened to end up at END_OF_PAGE-240 bytes
and nothing was mapped in the next page, a crash could happen.
Comment 6 Jakub Jelinek 2005-12-01 14:55:58 UTC
I can't understand how movqi_1 can not consider aligned_operand at all.
Will dig more...
Comment 7 Caolan McNamara 2005-12-01 14:59:47 UTC
Program received signal SIGSEGV, Segmentation fault.
[witching to Thread -1209017888 (LWP 28260)]
X11SalGraphics::SetXORMode (this=0x3cb0f10, bSet=0 '\0')
    at /home/caolan/ooo/SRC680_m142/vcl/unx/source/gdi/salgdi.cxx:695
695         if( !bXORMode_ == bSet )
(gdb) x/i $pc
0x20bfe89 <_ZN14X11SalGraphics10SetXORModeEh+11>:       mov    0xed(%edi),%esi
(gdb) x/1b $edi+0xed
0x3cb0ffd:      0x00
(gdb) x/4b $edi+0xed
0x3cb0ffd:      0x00    0x00    0x00    Cannot access memory at address 0x3cb1000
Comment 8 Andrew Pinski 2005-12-01 15:24:10 UTC
I thought this was already fixed (or at least was in 4.1.0), let me find the patch which might had fixed it).
Comment 9 Andrew Pinski 2005-12-01 15:25:36 UTC
Yes see PR 18019, it says it was fixed for 4.0.0.
Comment 10 Jakub Jelinek 2005-12-01 16:39:08 UTC
Well, clearly it is not fixed, as proven on stock gcc-4_0-branch with:
/* { dg-do compile } */
/* { dg-options "-m32 -Os -fpic -mtune=pentium4" } */

struct S
{
  void *p[30];
  unsigned char a0 : 1;
  unsigned char a1 : 1;
  unsigned char a2 : 1;
  unsigned char a3 : 1;
  unsigned char a4 : 1;
  unsigned char a5 : 1;
  unsigned char a6 : 1;
  unsigned char a7 : 1;
  unsigned char a8 : 1;
  unsigned char a9 : 1;
  unsigned char a10 : 1;
  unsigned char a11 : 1;
  unsigned char a12 : 1;
  unsigned char a13 : 1;
};

void foo (struct S *x, unsigned char y)
{
  if (! x->a12 == y)
    {
      x->a12 = y;
      x->a3 = 0;
      x->a5 = 0;
      x->a6 = 0;
      x->a7 = 0;
      x->a8 = 0;
      x->a9 = 0;
      x->a10 = 0;
      x->a11 = 0;
    }
}

and eventhough it works on 4.1 branch, there are no signs of it actually being
fixed in any way.

IMHO, if movqi_1 wants to use movl instead of movb or movzbl, it needs to satisfy
some predicate (whether it is aligned_operand or something else doesn't matter
much) that tells it that the optimization is safe (i.e. won't fall off the cliff).  Another thing is that aligned_operand certainly should use
MEM_ALIGN (), otherwise it will succeed only in far fewer cases than it
really could.  What the predicate does ATM can be used as fallback if MEM_ALIGN (op) < 32.  Now, we could perhaps safely use movl even if not aligned,
because i?86 is not a strict alignment target, if we were guaranteed we aren't
at the end of the cliff, but the question is if it is worthwhile in that case.
E.g. we could look at compute_builtin_object_size (MEM_EXPR (op), 2) and if
op is say a pointer to a middle of some structure, then we could safely use movl.
Is movl a win when the address isn't 32-bits aligned?
Comment 11 Jakub Jelinek 2005-12-02 22:55:41 UTC
Subject: Bug 25199

Author: jakub
Date: Fri Dec  2 22:55:35 2005
New Revision: 107955

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=107955
Log:
	PR target/25199
	* config/i386/i386.md (movqi_1): Only force imovx for alternative
	5 if operand 1 is not aligned.  Undo previous constraint change.
	* config/i386/predicates.md (aligned_operand): Use MEM_ALIGN.
testsuite/
	* gcc.target/i386/movq-2.c: New test.
	* gcc.target/i386/movq.c: Remove target i?86, instead add
	dg-require-effective-target ilp32.

Added:
    trunk/gcc/testsuite/gcc.target/i386/movq-2.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/i386/i386.md
    trunk/gcc/config/i386/predicates.md
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gcc.target/i386/movq.c

Comment 12 Jakub Jelinek 2005-12-02 22:57:10 UTC
Subject: Bug 25199

Author: jakub
Date: Fri Dec  2 22:57:03 2005
New Revision: 107956

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=107956
Log:
	PR target/25199
	* config/i386/i386.md (movqi_1): Only force imovx for alternative
	5 if operand 1 is not aligned.  Undo previous constraint change.
	* config/i386/predicates.md (aligned_operand): Use MEM_ALIGN.
testsuite/
	* gcc.target/i386/movq-2.c: New test.
	* gcc.target/i386/movq.c: Remove target i?86, instead add
	dg-require-effective-target ilp32.

Added:
    branches/gcc-4_1-branch/gcc/testsuite/gcc.target/i386/movq-2.c
Modified:
    branches/gcc-4_1-branch/gcc/ChangeLog
    branches/gcc-4_1-branch/gcc/config/i386/i386.md
    branches/gcc-4_1-branch/gcc/config/i386/predicates.md
    branches/gcc-4_1-branch/gcc/testsuite/ChangeLog
    branches/gcc-4_1-branch/gcc/testsuite/gcc.target/i386/movq.c

Comment 13 Jakub Jelinek 2005-12-02 22:58:39 UTC
Subject: Bug 25199

Author: jakub
Date: Fri Dec  2 22:58:33 2005
New Revision: 107957

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=107957
Log:
	PR target/25199
	* config/i386/i386.md (movqi_1): Only force imovx for alternative
	5 if operand 1 is not aligned.  Undo previous constraint change.
	* config/i386/predicates.md (aligned_operand): Use MEM_ALIGN.

	Backport from mainline:
	2005-06-07  Dale Johannesen  <dalej@apple.com>

	* config/i386/i386.md (movqi_1): Fix case where source
	is memory and destination EDI.
testsuite/
	* gcc.target/i386/movq-2.c: New test.
	* gcc.target/i386/movq.c: Remove target i?86, instead add
	dg-require-effective-target ilp32.

	Backport from mainline:
	2005-06-07  Dale Johannesen  <dalej@apple.com>

	* gcc.target/i386/movq.c: New.

Added:
    branches/gcc-4_0-branch/gcc/testsuite/gcc.target/i386/movq-2.c
    branches/gcc-4_0-branch/gcc/testsuite/gcc.target/i386/movq.c
Modified:
    branches/gcc-4_0-branch/gcc/ChangeLog
    branches/gcc-4_0-branch/gcc/config/i386/i386.md
    branches/gcc-4_0-branch/gcc/config/i386/predicates.md
    branches/gcc-4_0-branch/gcc/testsuite/ChangeLog

Comment 14 Jakub Jelinek 2005-12-12 09:15:56 UTC
Fixed in CVS.
Comment 15 Andrew Pinski 2008-08-29 06:54:32 UTC
I think this testcase is too strict in some cases.  With a modified compiler, we get:
        movb    123(%eax), %dl
        movl    %edx, %esi
...
        movb    120(%eax), %al
        movl    %eax, %esi

Which is correct as far as I can tell, as we only say the lower half of %esi is defined.  
Comment 16 Jakub Jelinek 2008-08-29 07:22:30 UTC
It is not incorrect, but is a code quality regression, which counts too.
So I'd prefer the testcase to stay as is and let the RA get fixed.