Hmm, didn't we fix this? ...

        movw    $115, (%rax)
        movw    $122, 2(%rax)
        movw    $98, 4(%rax)
        movq    (%rax), %rdi
        call    print_colour

Ahm, not exactly a dup of PR31309.

Shorter (non-runtime) testcase:

struct colour
{
  unsigned short red;
  unsigned short green;
  unsigned short blue;
};

void print_colour(struct colour col);

void foo(struct colour *c)
{
  print_colour(*c);
}

The problem is that struct colour is laid out like

    arg 0 <parm_decl 0x2b4f8506e2d0 c
        type <pointer_type 0x2b4f85160780 type <record_type 0x2b4f85160540 colour>
            unsigned DI
            size <integer_cst 0x2b4f85068b70 constant invariant 64>
            unit size <integer_cst 0x2b4f85068ba0 constant invariant 8>
            align 64 symtab 0 alias set -1 canonical type 0x2b4f85160780>
        used unsigned DI file t.c line 10 col 25 size <integer_cst 0x2b4f85068b70 64> unit size <integer_cst 0x2b4f85068ba0 8>
        align 64 context <function_decl 0x2b4f8515fea0 foo> initial <pointer_type 0x2b4f85160780>
        (reg/v/f:DI 58 [ c ]) arg-type <pointer_type 0x2b4f85160780>
        incoming-rtl (reg:DI 5 di [ c ])>>

which doesn't match its natural alignment nor size.

That's the layout of 'c', a pointer variable.  We don't see the layout
of the record_type here.

Forget that.  load_register_parameters converts

(mem/s:BLK (reg/v/f:DI 58 [ c ]) [0 S6 A16])

to

(mem/s:BLK (reg/v/f:DI 58 [ c ]) [0 S6 A16])

here:

          else if (partial == 0 || args[i].pass_on_stack)
            {
              rtx mem = validize_mem (args[i].value);

Errm.  change_address_1 simply builds a DImode mem (with alignment
info properly retained)

#0  0x00000000005e80c2 in adjust_address_1 (memref=0x2b0751d81520, 
    mode=DImode, offset=0, validate=0, adjust=1)
    at /space/rguenther/src/svn/gcc-4_3-branch/gcc/emit-rtl.c:1910
#1  0x00000000005e396c in operand_subword (op=0x2b0751d81520, offset=0, 
    validate_address=1, mode=BLKmode)
    at /space/rguenther/src/svn/gcc-4_3-branch/gcc/emit-rtl.c:1346
#2  0x00000000005e3a28 in operand_subword_force (op=0x2b0751d81520, offset=0, 
    mode=BLKmode)
    at /space/rguenther/src/svn/gcc-4_3-branch/gcc/emit-rtl.c:1374
#3  0x000000000060fd27 in move_block_to_reg (regno=5, x=0x2b0751d81520, 
    nregs=1, mode=BLKmode)
    at /space/rguenther/src/svn/gcc-4_3-branch/gcc/expr.c:1560
#4  0x000000000054f7b9 in load_register_parameters (args=0x7fff59711810, 
    num_actuals=1, call_fusage=0x7fff597119d0, flags=0, is_sibcall=0, 
    sibcall_failure=0x7fff597119ac)
    at /space/rguenther/src/svn/gcc-4_3-branch/gcc/calls.c:1681
#5  0x00000000005526b4 in expand_call (exp=0x2b075142d460, target=0x0, 
    ignore=1) at /space/rguenther/src/svn/gcc-4_3-branch/gcc/calls.c:2763

no idea if that is supposed to work (and thus *movdi_1_rex64 shouldn't match)
or if not ...

So, the problem is in move_block_to_reg that copies the argument piecewise
to regs like

  for (i = 0; i < nregs; i++)
    emit_move_insn (gen_rtx_REG (word_mode, regno + i),
                    operand_subword_force (x, i, mode));

not accounting for the fact that the last reg may be only partially filled
from the memory.  But the necessary information to deal with this is also
not available in this function currently.

FWIW, I think the error is in the caller of move_block_to_reg. 
move_block_to_reg can make use of a load_multiple instruction, which really
loads full regs.  I.e. it would be unreasonable to require changes in
move_block_to_reg to handle non-power-of-2 sizes.  Hence the caller
(load_register_parameters) needs to handle this.  I'm not sure if the
n_aligned_regs thingy could be misused for this, or if one simply should
opencode the special case of the last register being partial.

Index: calls.c
===================================================================
--- calls.c     (revision 134659)
+++ calls.c     (working copy)
@@ -2708,7 +2708,7 @@ expand_call (tree exp, rtx target, int i
         and whose alignment does not permit a direct copy into registers,
         make a group of pseudos that correspond to each register that we
         will later fill.  */
-      if (STRICT_ALIGNMENT)
+      if (1 || STRICT_ALIGNMENT)
        store_unaligned_arguments_into_pseudos (args, num_actuals);
 
       /* Now store any partially-in-registers parm.

also fixes this.

Witnessed in g++ 4.3.2

g++ (GCC) 4.3.2 20081105 (Red Hat 4.3.2-7)

Still broken on the trunk as well.

Happens for me too on Linux/AMD64 with:

gcc (Debian 4.3.4-2) 4.3.4

and

gcc (Debian 4.4.1-4) 4.4.1

It also happens with structs of sizes 3, 5 and 7 for me.

An easy (non-runtime) test case for different struct sizes can be made via:

struct test
{
  unsigned char size[TEST_SIZE];
};

void bar(struct test);

void foo(struct test *t)
{
  bar(*t);
}

Just define the struct size via "-DTEST_SIZE=size", where "size" is the desired size of the struct, on the gcc command line.

One can witness that for struct sizes of 1, 2, 4, 8 and everything above 8 it works just fine.

Here's the assembly output, generated via 4.3.4, for the working struct sizes:

struct size of 1:

	movq	-8(%rbp), %rax
	movzbl	(%rax), %edi
	call	bar

struct size of 2:

	movq	-8(%rbp), %rax
	movzwl	(%rax), %edi
	call	bar


struct size of 4:

	movq	-8(%rbp), %rax
	movl	(%rax), %edi
	call	bar


struct size of 8:

	movq	-8(%rbp), %rax
	movq	(%rax), %edi
	call	bar

For all sizes above 8, it does also generate correct assembly, that means it will only read the correct number of bytes. Here is an example with a struct size of 11:

	movq	-8(%rbp), %rdx
	movq	(%rdx), %rdi
	movzbl	8(%rdx), %ecx
	movzbl	9(%rdx), %eax
	salq	$8, %rax
	orq	%rax, %rcx
	movzbl	10(%rdx), %eax
	salq	$16, %rax
	movq	%rax, %rsi
	orq	%rcx, %rsi
	call	bar

As you can see it uses one "movq" to read eight bytes at once and then three "movzbl" to read the remaining three bytes. The assembly for this case was generated with -O0. It will look differently for other optimization levels, but is still using the same basic logic.

For struct sizes of 3, 5, 6 or 7 the compiler always generates:

	movq	-8(%rbp), %rax
	movq	(%rax), %rdi
	call	bar

for me, which is the reported incorrect behavior.

Still present on a recent GCC snapshot:

Johannes' Code compiled with "(Ubuntu 20090912-1ubuntu2) 4.5.0 20090912 (experimental) [trunk revision 151650]" and -O0 -DTEST_SIZE=5:

	subq	$16, %rsp
	movq	%rdi, -8(%rbp)
	movq	-8(%rbp), %rax
	movq	(%rax), %rdi
	call	bar

Still happening on 4.5.0 20091228 (and gcc 4.4.1) on Ubuntu 9.10/amd64). I found that it goes away in 3 separate instances when turning on -finline-functions. The last 2 instances of memory corruption I am running into are functions in vtables, which can't be inlined (and therefore work around this bug) until de-virtualization is eliminated.

This seems like a pretty serious and common problem; is there any chance it will be fixed in 4.5?

The issue is in a very twisted piece of GCC where the chances to break sth are
bigger that to fix sth ;)

And it isn't a regression, so it's not on too many peoples radar (nor does
it seem to happen in practice and thus affect those who may be able to fix it).

But, let's CC some more people ;)

Also broken at least since GCC 2.95 on i?86 with

struct colour
{
  unsigned char red;
  unsigned char green;
  unsigned char blue;
};

void print_colour(struct colour col) __attribute__((regparm(3)));

void
foo(struct colour *c)
{
  print_colour(*c);
}

PR37954 looks like a dup for arm.

*** Bug 50499 has been marked as a duplicate of this bug. ***

From PR50499, we have this similar test case that fails on powerpc64-linux:

struct S50 { char i[9]; };
extern void bar (struct S50);
void
foo (struct S50 *p)
{
  bar (*p);
}

Gives (using -m64 -O1):

        ...
	ld 3,0(3)
	ld 4,8(9)
	bl bar

I just ran into this and verified that it exists in gcc svn trunk
gcc (GCC) 4.9.0 20140221 (experimental)

__________________________________
Type your response ABOVE THIS LINE to reply

------------------------------------------------------------------------------------------------------------
Customer: tromey at gcc dot gnu.org
Subject: [Bug middle-end/36043] gcc reads 8 bytes for a struct of size 6 which leads to sigsegv
------------------------------------------------------------------------------------------------------------

Jamie Forrest | FEB 23, 2014 | 12:02AM UTC



------------------------------------------------------------------------------------------------------------

tromey | FEB 22, 2014 | 09:10PM UTC  | Original message 

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36043

David Fries <david at fries dot net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |david at fries dot net

--- Comment #20 from David Fries <david at fries dot net> ---
I just ran into this and verified that it exists in gcc svn trunk
gcc (GCC) 4.9.0 20140221 (experimental)


------------------------------------------------------------------------------------------------------------
This message was sent to gcc-bugzilla@gcc.gnu.org in reference to Case #: 4164.
------------------------------------------------------------------------------------------------------------




[[0958b287b2c175eb67793a138c0536bfc6a0b07d-206330137]]

Here's another segfaulting test case, for x86-64. This fails with GCC 4.6.2 and GCC 4.8.1.
This GCC bug is really hard to work around, since it can strike anywhere without warning. Is it possible that one of the patches above (e.g., Richard Biener's comment #9) could be adopted into 4.8.x or 4.9.x?


#include <cstdio>
#include <stdint.h>
#include <sys/mman.h>

struct int24_t { uint8_t m_Internal[3]; };

void foo(int24_t a) { puts("2"); }

int main() {
    const int OS_PAGE_SIZE = 0x1000;
    const int COUNT = (OS_PAGE_SIZE) / sizeof(int24_t);
    printf("sizeof int24_t = %d\n", (int)sizeof(int24_t));

    void* arena1 = mmap(0, OS_PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
    int24_t* ptr = (int24_t*)mmap(0, OS_PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
    munmap(arena1, OS_PAGE_SIZE);

    puts("1");
    foo(ptr[COUNT-1]);
    puts("3");
}

(In reply to Michael Matz from comment #8)
> FWIW, I think the error is in the caller of move_block_to_reg. 
> move_block_to_reg can make use of a load_multiple instruction, which really
> loads full regs.  I.e. it would be unreasonable to require changes in
> move_block_to_reg to handle non-power-of-2 sizes.  Hence the caller
> (load_register_parameters) needs to handle this.  I'm not sure if the
> n_aligned_regs thingy could be misused for this, or if one simply should
> opencode the special case of the last register being partial.

That would be sth like

Index: gcc/calls.c
===================================================================
--- gcc/calls.c (revision 208124)
+++ gcc/calls.c (working copy)
@@ -1984,7 +1984,26 @@ load_register_parameters (struct arg_dat
                    emit_move_insn (ri, x);
                }
              else
-               move_block_to_reg (REGNO (reg), mem, nregs, args[i].mode);
+               {
+                 if (size % UNITS_PER_WORD == 0
+                     || MEM_ALIGN (mem) % BITS_PER_WORD == 0)
+                   move_block_to_reg (REGNO (reg), mem, nregs, args[i].mode);
+                 else
+                   {
+                     if (nregs > 1)
+                       move_block_to_reg (REGNO (reg), mem,
+                                          nregs - 1, args[i].mode);
+                     rtx dest = gen_rtx_REG (word_mode,
+                                             REGNO (reg) + nregs - 1);
+                     rtx src = operand_subword_force (mem,
+                                                      nregs - 1, args[i].mode);
+                     rtx tem = extract_bit_field (src, size * BITS_PER_UNIT,
+                                                  0, 1, dest, word_mode,
+                                                  word_mode);
+                     if (tem != dest)
+                       convert_move (dest, tem, 1);
+                   }
+               }
            }
 
          /* When a parameter is a block, and perhaps in other cases, it is

it's similar to what store_unaligned_arguments_into_pseudos would end up
doing but only for the last register (so it's probably easier to dispatch
to that and handle !STRICT_ALIGNMENT targets there).

Anyway, the generated code is of course "horrible".

foo:
.LFB0:
        .cfi_startproc
        movq    %rdi, %rcx
        movzwl  (%rdi), %edx
        movzwl  2(%rdi), %edi
        salq    $16, %rdi
        movq    %rdi, %rax
        movzwl  4(%rcx), %edi
        orq     %rdx, %rax
        salq    $32, %rdi
        orq     %rax, %rdi
        jmp     print_colour

for some reason extract_bit_field doesn't consider using a 4-byte load
for the first part.  With AVX one could also use a masked load (and thus
implement the extv/insv pattern family?  not sure if it is valid to
reject non-byte boundary variants).  But if we end up using
extract_bit_field more and more it's worth optimizing it further to
avoid the above mess... (we end up using extract_split_bit_field).

Author: amodra
Date: Wed Apr 15 07:29:01 2015
New Revision: 222115

URL: https://gcc.gnu.org/viewcvs?rev=222115&root=gcc&view=rev
Log:
	PR target/65408
	PR target/58744
	PR middle-end/36043
	* calls.c (load_register_parameters): Don't load past end of
	mem unless suitably aligned.


Added:
    trunk/gcc/testsuite/gcc.dg/pr65408.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/calls.c
    trunk/gcc/testsuite/ChangeLog

Author: amodra
Date: Thu Apr 30 11:11:34 2015
New Revision: 222616

URL: https://gcc.gnu.org/viewcvs?rev=222616&root=gcc&view=rev
Log:
gcc/
	PR target/65408
	PR target/58744
	PR middle-end/36043
	* calls.c (load_register_parameters): Don't load past end of
	mem unless suitably aligned.
gcc/testsuite/
	* gcc.dg/pr65408.c: New.


Added:
    branches/gcc-5-branch/gcc/testsuite/gcc.dg/pr65408.c
Modified:
    branches/gcc-5-branch/gcc/ChangeLog
    branches/gcc-5-branch/gcc/calls.c
    branches/gcc-5-branch/gcc/testsuite/ChangeLog

Fixed for 5.2

Thanks!