gcc builtin memcpy is bogus?

gcc builtin memcpy is bogus?

[Matthew Wilcox]

This is a fun one ... I don't know what to blame for this.

struct rfd {
        u16 status;
        u16 command;
        u32 link;
        u32 rbd;
        u16 actual_size;
        u16 size;
};

static void e100_rx_rfa_add_tail(struct nic *nic, struct rx_list *curr)
{
        struct rfd *rfd = (struct rfd *)curr->skb->data;
        
        memcpy(rfd, &nic->blank_rfd, sizeof(struct rfd));
}

I'm using gcc (GCC) 3.3.2 (Debian) for this.

The kernel doesn't use -fno-builtin or -fno-builtin-memcpy so it attempts
to optimise this by doing 4-byte loads and stores.  Problem is, the
destination is misaligned (deliberately), so the kernel emits unaligned
messages.  Adding -fno-builtin to the command line turns this back into
a call to memcpy.

So how should this be fixed?

 - Improve gcc's builtin memcpy to work on non-aligned structs (unlikely)?
 - Somehow tag the pointer or the struct that it's unaligned?
 - Compile with -fno-builtin-memcpy (and probably a few others too)?

Other suggestions?

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: gcc builtin memcpy is bogus?

[Andreas Schwab]

Matthew Wilcox <willy@debian.org> writes:

> This is a fun one ... I don't know what to blame for this.
>
> struct rfd {
>         u16 status;
>         u16 command;
>         u32 link;
>         u32 rbd;
>         u16 actual_size;
>         u16 size;
> };
>
> static void e100_rx_rfa_add_tail(struct nic *nic, struct rx_list *curr)
> {
>         struct rfd *rfd = (struct rfd *)curr->skb->data;
>         
>         memcpy(rfd, &nic->blank_rfd, sizeof(struct rfd));
> }
>
> I'm using gcc (GCC) 3.3.2 (Debian) for this.
>
> The kernel doesn't use -fno-builtin or -fno-builtin-memcpy so it attempts
> to optimise this by doing 4-byte loads and stores.  Problem is, the
> destination is misaligned (deliberately), so the kernel emits unaligned
> messages.  Adding -fno-builtin to the command line turns this back into
> a call to memcpy.
>
> So how should this be fixed?
>
>  - Improve gcc's builtin memcpy to work on non-aligned structs (unlikely)?

The builtin memcpy is correct, casting unaligned data is invoking
undefined behaviour here.

>  - Somehow tag the pointer or the struct that it's unaligned?
>  - Compile with -fno-builtin-memcpy (and probably a few others too)?
>
> Other suggestions?

- Don't use the casted address, but rather the original pointer in the
  memcpy call.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: gcc builtin memcpy is bogus?

[David Mosberger]

>>>>> On Mon, 8 Dec 2003 16:01:24 +0000, Matthew Wilcox <willy@debian.org> said:

  Matthew> This is a fun one ... I don't know what to blame for this.

  Matthew> struct rfd {
  Matthew>   u16 status;
  Matthew>   u16 command;
  Matthew>   u32 link;
  Matthew>   u32 rbd;
  Matthew>   u16 actual_size;
  Matthew>   u16 size;
  Matthew> };

  Matthew> The kernel doesn't use -fno-builtin or -fno-builtin-memcpy
  Matthew> so it attempts to optimise this by doing 4-byte loads and
  Matthew> stores.  Problem is, the destination is misaligned
  Matthew> (deliberately), so the kernel emits unaligned messages.
  Matthew> Adding -fno-builtin to the command line turns this back
  Matthew> into a call to memcpy.

The code is wrong.  If it's 2-byte aligned, use

	attribute ((packed) (aligned (2))

to mark it as such (check on the syntax of the attributes, I probably
got it wrong).

	--david

Re: gcc builtin memcpy is bogus?

[Matthew Wilcox]

On Mon, Dec 08, 2003 at 05:25:27PM +0100, Andreas Schwab wrote:
> - Don't use the casted address, but rather the original pointer in the
>   memcpy call.

Thanks, that works nicely.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain