Bug 24178

I think you are violating some alignment rule in C.

(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.

(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.
> 

(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

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.

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?

Can someone try sparc-linux, I would not doubt this could be reproduced there too.

(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).

(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

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.

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

(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.

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

Fixed.

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

Can you instead open a new bug please?  Thx.

(In reply to comment #16)
> Can you instead open a new bug please?  Thx.

Sure. PR45212.