Bug 24178 - [4.0/4.1 regression] generates code that produces unaligned access exceptions
Summary: [4.0/4.1 regression] generates code that produces unaligned access exceptions
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 4.1.0
: P5 normal
Target Milestone: 4.0.3
Assignee: Richard Henderson
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2005-10-03 18:35 UTC by tsv
Modified: 2010-08-06 20:00 UTC (History)
6 users (show)

See Also:
Host:
Target: alpha-*-linux-gnu
Build:
Known to work: 3.4.5
Known to fail: 4.0.2 4.1.0
Last reconfirmed: 2005-11-02 07:13:35


Attachments
proposed patch (769 bytes, patch)
2005-11-02 07:43 UTC, Richard Henderson
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description tsv 2005-10-03 18:35:51 UTC
gcc version 4.0.1 20050727 (Red Hat 4.0.1-5)
gcc version 4.0.2
GNU C version 4.1.0 20050927 (experimental) (alpha-unknown-linux-gnu)

All tested versions generate the same wrong code for the following testcase
(EV4/EV5 only with -O2 optimization):
#include <stdio.h>
#include <stddef.h>

typedef struct Foo
{
   void *p1;
   long p2;
   void *p3;
   unsigned char p4;
   char p5[3];
} Foo;

#ifdef __DECC
#undef offsetof
#define offsetof(TYPE, MEMBER) ((char *)(&(((TYPE *)0)-> MEMBER )))
#endif


void foo(const char *fn)
{
   Foo *p;

   p = (Foo *)(fn - offsetof(Foo, p5));
   p->p4 = 1;
}

int main(int argc, char **argv)
{
   Foo p;
#if defined(__alpha__) && defined(__linux__)
# include <asm/sysinfo.h>
# include <asm/unistd.h>

   unsigned int buf[2] =
     {
         SSIN_UACPROC, UAC_SIGBUS | UAC_NOPRINT
     };
   syscall(__NR_osf_setsysinfo, SSI_NVPAIRS, buf, 1, 0, 0, 0);
#endif

   printf("Offset - %d\n", offsetof(Foo, p5));

   foo(p.p5);
}

The assembler for the function "foo" is:
        .globl foo
        .ent foo
$foo..ng:
foo:
        .frame $30,0,$26,0
        .prologue 0
        ldah $2,256($31)
        ldl $1,-4($16)
        zapnot $1,247,$1
        bis $1,$2,$2
        stl $2,-4($16)
        ret $31,($26),1
        .end foo

The offset of "p5" member is 25 bytes, but compiler thinks that "p5" is aligned in "foo" function and issue "ldl" on unaligned address.
DECC compiler generates "ldl $1, -1($16)" instruction.

It might be not very good coding practice, but it taken from Mozilla source.

Thank you.
Comment 1 Andrew Pinski 2005-10-03 19:03:34 UTC
I think you are violating some alignment rule in C.
Comment 2 Falk Hueffner 2005-10-03 20:48:22 UTC
(In reply to comment #0)

> The offset of "p5" member is 25 bytes, but compiler thinks that "p5" is aligned
> in "foo" function

You are casting a pointer to a Foo* that doesn't have proper alignment for
a Foo. This is undefined behavior, so anything can happen, including an
unaligned access at some later point. This is not a gcc bug.

> It might be not very good coding practice, but it taken from Mozilla source.

Please report it there instead.
Comment 3 tsv 2005-10-04 05:59:03 UTC
(In reply to comment #2)
> (In reply to comment #0)
> 
> > The offset of "p5" member is 25 bytes, but compiler thinks that "p5" is aligned
> > in "foo" function
> 
> You are casting a pointer to a Foo* that doesn't have proper alignment for
> a Foo. This is undefined behavior, so anything can happen, including an
> unaligned access at some later point. This is not a gcc bug.
Correct me if I wrong, but loading long from passed pointer to char minus 4
is not correct too. I could agree if the compiler did substruct 25 from pointer and added 24 and that did load (it does it in not optimized version). In this case it would me me who passed incorrect pointer (in fact DEC C did that) and I agree with compiler that warns me about it.
I can't test right now, but if I recall gcc 3.4 didn't have such problems.

> 
> > It might be not very good coding practice, but it taken from Mozilla source.
> 
> Please report it there instead.
> 
Comment 4 Falk Hueffner 2005-10-04 07:32:44 UTC
(In reply to comment #3)

> Correct me if I wrong, but loading long from passed pointer to char minus 4
> is not correct too.

You are right, I misread the example, the pointer that is cast to Foo* is
actually correctly aligned. Here is a slightly clearer test case:

typedef struct {
    long l;
    char p4;
} Foo;

void foo(char *fn) {
    Foo *p = (Foo *) (fn - 9);
    p->p4 = 1;
}

int main(int argc, char **argv)
{
#if defined(__alpha__) && defined(__linux__)
# include <asm/sysinfo.h>
# include <asm/unistd.h>
   syscall(__NR_osf_setsysinfo, SSI_NVPAIRS,
	   (int[]){SSIN_UACPROC, UAC_SIGBUS | UAC_NOPRINT}, 1, 0, 0, 0);
#endif

   Foo p;
   foo(((char *) &p) + 9);
   return 0;
}

This is very similar to a gcc 3.3 bug that disappeared in 3.4: http://bugs.debian.org/301983
Comment 5 tsv 2005-10-14 13:30:29 UTC
I tried check the difference between 3.4 and 4.x. The 3.4 emit instruction that correct passed pointer (in my case -25) and then stores value to member of structure (p4) at correct offset (24).
In gcc 4.x the "store_field" is called with target rtx defined as "MEM plus const_int -25", but in gcc 3.4 - just MEM. The "store_bit_field" adds member offset to target rtx and it became "MEM plus const_int -1". Later on "get_aligned_mem" in alpha.c tries to convert it to "load long", but at that point it thinks that MEM is aligned (which is not true).
Unfortunatelly, I don't have deep knowledge of gcc :) and could not figure out what is correct place for check of alignment restrictions for such cases:
expand_assignment, store_field or store_bit_field functions. All of them seems to have alignemnt check logic.

Thank you.
Comment 6 Falk Hueffner 2005-10-15 15:04:01 UTC
OK, let's have a look at this somewhat minimal example:  

struct S {
    long l;
    unsigned char c;
};
unsigned long f(unsigned char *p10) {
    struct S *p = (struct S *) (p10 + 10);
    return p->c;
}

What we want is of course:

     ldl     v0,18(a0)
     and     v0,0xff,v0

but we get:

     ldl     v0,16(a0)
     extbl   v0,0x2,v0

The problem comes from get_aligned_mem being passed

(mem/s:QI (plus:DI (reg/v/f:DI 70 [ p10 ]
		   (const_int 18 [0x12])) [0 S1 A64])

However, get_aligned_mem assumes the *base* without offset is
aligned, so it generates bogus code.

get_aligned_mem is being called from alpha_expand_mov_nobwx because
aligned_memory_operand says "yeah" to the above rtx.
aligned_memory_operand is supposed to do:

;; Return 1 if this memory address is a known aligned register plus
;; a constant.

which is in fact what get_aligned_mem needs. However, the first thing
aligned_memory_operand checks is

  if (MEM_ALIGN (op) >= 32)
    return 1;

which happens to be true, since the base *with* offset is aligned.
This check seems bogus, since the base is still unaligned. However,
given the scary comments surrounding it I don't really dare touching
it. Richard, can you give a hint?
Comment 7 Andrew Pinski 2005-10-22 21:59:31 UTC
Can someone try sparc-linux, I would not doubt this could be reproduced there too.
Comment 8 Falk Hueffner 2005-10-22 22:42:01 UTC
(In reply to comment #7)
> Can someone try sparc-linux, I would not doubt this could be reproduced there
> too.

Actually, it can't, as I tried to explain in comment #6, it is probably a
bug in config/alpha/predicates.md (and it in fact goes away if I remove
those two lines).
Comment 9 tsv 2005-10-23 18:22:33 UTC
(In reply to comment #8)
> (In reply to comment #7)
> > Can someone try sparc-linux, I would not doubt this could be reproduced there
> > too.
> 
> Actually, it can't, as I tried to explain in comment #6, it is probably a
> bug in config/alpha/predicates.md (and it in fact goes away if I remove
> those two lines).
> 
By commenting those two lines gcc start to use ldx_u instructions a lot.
If I add simular function to your example like this:
unsigned long f1(struct S *p)
{
   return p->c;
}

I get:
f1:
        .frame $30,0,$26,0
        .prologue 0
        ldl $0,8($16)
        and $0,0xff,$0
        ret $31,($26),1
        .end f1

f:
        .frame $30,0,$26,0
        .prologue 0
        lda $1,18($16)
        ldq_u $0,18($16)
        extbl $0,$1,$0
        ret $31,($26),1
        .end f

The best case is :) the "f" function should have the same code as "f1" except the offset should be 18.
After tracing store_bit_field function I came to following:
- if target (pointer to structure) already known and represented as mem/s (reg ...), then "store_bit_field" generates code to access structure member
- if target (pointer to structure) is calculated as same pointer plus offset and represented as mem/s (reg plus const int...), the "store_bit_bit" just adds member offset to "const int" (adjust_address) and then works with new offset which alignment is not known and we get bogus code.

Very good code is generated if I define pointer as volatile:
unsigned long f2(unsigned char *p10)
{
      volatile struct S *p = (struct S *) (p10 + 10);
      return p->c;
}
the assembler is:
f2:
        .frame $30,0,$26,0
        .prologue 0
        lda $16,10($16)
        ldl $0,8($16)
        and $0,0xff,$0
        ret $31,($26),1
        .end f2

(this is simular that gcc 3.4 produces)

So, my opinion that it should be fixed on upper level (somewhere in store_bit_field function).

Thank you
Comment 10 Richard Henderson 2005-11-02 07:43:38 UTC
Created attachment 10112 [details]
proposed patch

The root problem is that get_aligned_mem and aligned_memory_operand
didn't match up.  Adding a bit of code to get_aligned_mem to handle
MEM_ALIGN as well appears to cure the problem.  I've not yet tested
this beyond Falk's trivial testcase.
Comment 11 Richard Henderson 2005-11-02 18:20:11 UTC
Subject: Bug 24178

Author: rth
Date: Wed Nov  2 18:20:07 2005
New Revision: 106388

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=106388
Log:
        PR target/24178
        * config/alpha/alpha.c (get_aligned_mem): Honor alignment given
        by MEM_ALIGN.

Added:
    trunk/gcc/testsuite/gcc.target/alpha/pr24178.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/alpha/alpha.c

Comment 12 tsv 2005-11-02 22:26:10 UTC
(In reply to comment #11)
> Subject: Bug 24178
> 
> Author: rth
> Date: Wed Nov  2 18:20:07 2005
> New Revision: 106388
> 
> URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=106388
> Log:
>         PR target/24178
>         * config/alpha/alpha.c (get_aligned_mem): Honor alignment given
>         by MEM_ALIGN.
> 
> Added:
>     trunk/gcc/testsuite/gcc.target/alpha/pr24178.c
> Modified:
>     trunk/gcc/ChangeLog
>     trunk/gcc/config/alpha/alpha.c
> 

Thank you Richard. It does generates a better code.
Comment 13 Richard Henderson 2005-11-03 00:33:19 UTC
Subject: Bug 24178

Author: rth
Date: Thu Nov  3 00:33:09 2005
New Revision: 106417

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=106417
Log:
        PR target/24178
        * config/alpha/alpha.c (get_aligned_mem): Honor alignment given
        by MEM_ALIGN.

Added:
    branches/gcc-4_0-branch/gcc/testsuite/gcc.target/alpha/pr24178.c
      - copied unchanged from r106416, trunk/gcc/testsuite/gcc.target/alpha/pr24178.c
Modified:
    branches/gcc-4_0-branch/gcc/ChangeLog
    branches/gcc-4_0-branch/gcc/config/alpha/alpha.c

Comment 14 Richard Henderson 2005-11-03 00:34:25 UTC
Fixed.
Comment 15 Uroš Bizjak 2010-08-06 19:33:04 UTC
This one started to fail on mainline recently.

f:
	.frame $30,0,$26,0
	.prologue 0
	lda $1,18($16)
	ldq_u $0,18($16)
	extbl $0,$1,$0
	ret $31,($26),1
	.end f

The testcase passed with r161055 [1] and failed with r161806 [2].

In between is Richi's mem-ref2 merge...

[1] http://gcc.gnu.org/ml/gcc-testresults/2010-06/msg02114.html
[2] http://gcc.gnu.org/ml/gcc-testresults/2010-07/msg00417.html
Comment 16 Richard Biener 2010-08-06 19:45:53 UTC
Can you instead open a new bug please?  Thx.
Comment 17 Uroš Bizjak 2010-08-06 20:00:45 UTC
(In reply to comment #16)
> Can you instead open a new bug please?  Thx.

Sure. PR45212.