[BUG mm] "fixed" i386 memcpy inlining buggy

[Christophe Saout]

Hi Denis,

the new i386 memcpy macro is a ticking timebomb.

I've been debugging a new mISDN crash, just to find out that a memcpy
was not inlined correctly.

Andrew, you should drop the fix-i386-memcpy.patch (or have it fixed).

This source code:

mISDN_pid_t pid;
[...]
memcpy(&st->mgr->pid, &pid, sizeof(mISDN_pid_t));

was compiled as:

lea0xffa4(%ebp),%esi< %esi is loaded
(   add$0x10,%ebx  )
(   mov%ebx,%eax   ) something else
(   call   1613  ) %esi preserved
mov0xffa0(%ebp),%edx
mov0x74(%edx),%edi  < %edi is loaded
add$0x20,%edi offset in structure added
!   mov$0x14,%esi!! < %esi overwritten!
mov%esi,%ecx< %ecx loaded
repz movsl %ds:(%esi),%es:(%edi)

Apparently the compiled decided that the value 0x14 could be reused
afterwards (which it does for an inlined memset of the same size some
instructions below) and clobbers %esi.

Looking at the macro:

__asm__ __volatile__(
""
: "=&D" (edi), "=&S" (esi)
: "0" ((long) to),"1" ((long) from)
: "memory"
);
}
if (n >= 5*4) {
/* large block: use rep prefix */
int ecx;
__asm__ __volatile__(
"rep ; movsl"
: "=&c" (ecx)

it seems obvious that the compiled assumes it can reuse %esi and %edi
for something else between the two __asm__ sections. These should
probably be merged.



signature.asc
Description: This is a digitally signed message part

Re: [BUG mm] "fixed" i386 memcpy inlining buggy

[Christophe Saout]

Am Mittwoch, den 06.04.2005, 13:14 +0300 schrieb Denis Vlasenko:

> Oh shit. I was trying to be too clever. I still run with this patch,
> so it must be happening very rarely.

Yes, that's right, it happened with code that's not in the mainline tree
but could have happened anywhere.

> Does this one compile ok?

Yes, the case that failed is now okay. I changed it slightly to assign
esi and edi directy on top of the functions, no asm section needed here.
The compiler will make sure that they have the correct values when
needed.

In the case above the compiler now uses %ebx to save the loop counter
instead of %esi.

In drivers/cdrom/cdrom.c I'm observing one very strange thing though.

It appears that the compiler decided to put the local variable edi on
the stack for some unexplicable reason (or possibly there is?). Since
the asm sections are memory barriers the compiler then saves the value
of %edi on the stack before entering the next assembler section.

1f1c:   a5  movsl  %ds:(%esi),%es:(%edi)
1f1d:   89 7d 84mov%edi,0xff84(%ebp)
1f20:   a5  movsl  %ds:(%esi),%es:(%edi)
1f21:   89 7d 84mov%edi,0xff84(%ebp)
1f24:   66 a5   movsw  %ds:(%esi),%es:(%edi)

(this is a constant 10 byte memcpy)

The only thing that would avoid this is to either tell the compiler to
never put esi/edi in memory (which I think is not possibly across
different versions of gcc) or to always generate a single asm section
for all the different cases.



signature.asc
Description: This is a digitally signed message part