On Tue, Feb 02, 2010 at 04:47:44PM +0300, Yuriy Kaminskiy wrote:
On 30.01.2010 15:48, Michael Niedermayer wrote:
On Fri, Jan 29, 2010 at 10:24:11PM +0300, Yuriy Kaminskiy wrote:
==32414== ==32414== Invalid write of size 8 ==32414== at 0x874CF44: postProcess_MMX2 (in /path/to/mplayer) ==32414== Address 0xbeffa9d0 is just below the stack ptr. To suppress, use: --workaround-gcc296-bugs=yes I, of course, don't use gcc-2.96 ;-) I've looked into libpostproc/postprocess_template.c, and, indeed, it uses memory below %esp: === cut === static inline void RENAME(doVertDefFilter)(uint8_t src[], int stride, PPContext *c) { [...] __asm__ volatile( "pxor %%mm7, %%mm7 \n\t" "lea -40(%%"REG_SP"), %%"REG_c" \n\t" // make space for 4 8-byte vars "and "ALIGN_MASK", %%"REG_c" \n\t" // align static inline void RENAME(dering)(uint8_t src[], int stride, PPContext *c) static av_always_inline void RENAME(do_a_deblock)(uint8_t *src, int step, int stride, PPContext *c){ === cut === Not sure if this *must* be fixed, but it feels unsafe, so... Patch attached; doVertDefFilter and do_a_deblock changes should not affect speed, not sure about dering one.
please put a START/STOP TIMER around things to make sure
tried, failed to acquire conclusive results; original/v3/v4 variants sometimes [tiny bit] slower, sometimes [even less] faster (for all functions; at least, my assumption that doVertDef and do_a changes "safer", and dering "more problematic" seems wrong; btw, doVertDef should be checked with MMX2 and 3DNOW disabled, dering - with MMX2 or 3DNOW enabled [do_a_deblock - either way]).
+ DECLARE_ALIGNED(8, uint64_t, tmp)[4]; // make space for 4 8-byte vars
tabs are forbidden in our svn
och; i know; sorry
- "movq (%%"REG_c"), %%mm2 \n\t" // 2L0 - 5L1 + 5L2 - 2L3 - "movq 8(%%"REG_c"), %%mm3 \n\t" // 2H0 - 5H1 + 5H2 - 2H3 + "movq %3, %%mm2 \n\t" // 2L0 - 5L1 + 5L2 - 2L3 + "movq 8+%3, %%mm3 \n\t" // 2H0 - 5H1 + 5H2 - 2H3
i have a bad feeling about this, what makes you belive this will work better than the last time such syntax was tried also my gcc replaces "o" with 4+%0 happily by 4+(%edx)
works for me (tm), and it should work with gas, because it is supposed to be used in this way; well, and for cc not to use 1234(%esp) here would be very strange (not that we can rely on that). anyway, alternative version [v4], that does not use 8+%3/"o" attached;
or v5, that does not attempt to save one register in dering (seems, that was not needed even for -fPIC -fomit-frame-pointer, and -fPIC -fno-omit-frame-pointer was broken before
[plus postproc asm is not PIC anyway];
there are much more people who insist on -fPIC because they are confused than there are people actually need code free of relocations I think after reading over this again the best solution would be to use the context as temporary space, we have a "m"(c->pQPb) anyway so if we put a pointer into a register to the context we could address pQPb and the temp aligned space easily. That way we dont need to create any aligned space on the stack ... [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The educated differ from the uneducated as much as the living from the dead. -- Aristotle