[patch] libpostproc: mmx code uses stack below %esp
Hello! While trying to catch unrelated bug (that finally was not bug, but misconfiguration [-lavdopts fast was too fragile for broken mpeg-2 stream]), I've run mplayer under valgrind, and got bunch of warnings: ==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) [...same...] static av_always_inline void RENAME(do_a_deblock)(uint8_t *src, int step, int stride, PPContext *c){ [...same...] === 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.
On Fri, Jan 29, 2010 at 10:24:11PM +0300, Yuriy Kaminskiy wrote:
Hello! While trying to catch unrelated bug (that finally was not bug, but misconfiguration [-lavdopts fast was too fragile for broken mpeg-2 stream]), I've run mplayer under valgrind, and got bunch of warnings: ==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) [...same...] static av_always_inline void RENAME(do_a_deblock)(uint8_t *src, int step, int stride, PPContext *c){ [...same...] === 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
postprocess_template.c | 61 ++++++++++++++++++++++++------------------------- 1 file changed, 30 insertions(+), 31 deletions(-) ebb4e02086f03dd082ee8025fa50eb34bb456d0a postproc-invalid-stack-3.patch Index: MPlayer-20100125+lavc-mt/libpostproc/postprocess_template.c =================================================================== --- MPlayer-20100125+lavc-mt.orig/libpostproc/postprocess_template.c 2010-01-23 17:23:38.000000000 +0300 +++ MPlayer-20100125+lavc-mt/libpostproc/postprocess_template.c 2010-01-26 17:58:37.000000000 +0300 @@ -767,10 +767,10 @@ static inline void RENAME(doVertDefFilte */ #elif HAVE_MMX src+= stride*4; + {
+ DECLARE_ALIGNED(8, uint64_t, tmp)[4]; // make space for 4 8-byte vars
tabs are forbidden in our svn
__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 // 0 1 2 3 4 5 6 7 // %0 %0+%1 %0+2%1 eax+2%1 %0+4%1 eax+4%1 edx+%1 edx+2%1 // %0 eax eax+%1 eax+2%1 %0+4%1 edx edx+%1 edx+2%1 @@ -812,8 +812,8 @@ static inline void RENAME(doVertDefFilte "psubw %%mm3, %%mm1 \n\t" // 2H0 - 5H1 + 5H2 - H3 "psubw %%mm2, %%mm0 \n\t" // 2L0 - 5L1 + 5L2 - 2L3 "psubw %%mm3, %%mm1 \n\t" // 2H0 - 5H1 + 5H2 - 2H3 - "movq %%mm0, (%%"REG_c") \n\t" // 2L0 - 5L1 + 5L2 - 2L3 - "movq %%mm1, 8(%%"REG_c") \n\t" // 2H0 - 5H1 + 5H2 - 2H3 + "movq %%mm0, %3 \n\t" // 2L0 - 5L1 + 5L2 - 2L3 + "movq %%mm1, 8+%3 \n\t" // 2H0 - 5H1 + 5H2 - 2H3
"movq (%%"REG_a", %1, 2), %%mm0 \n\t" "movq %%mm0, %%mm1 \n\t" @@ -822,8 +822,8 @@ static inline void RENAME(doVertDefFilte
"psubw %%mm0, %%mm2 \n\t" // L3 - L4 "psubw %%mm1, %%mm3 \n\t" // H3 - H4 - "movq %%mm2, 16(%%"REG_c") \n\t" // L3 - L4 - "movq %%mm3, 24(%%"REG_c") \n\t" // H3 - H4 + "movq %%mm2, 16+%3 \n\t" // L3 - L4 + "movq %%mm3, 24+%3 \n\t" // H3 - H4 "paddw %%mm4, %%mm4 \n\t" // 2L2 "paddw %%mm5, %%mm5 \n\t" // 2H2 "psubw %%mm2, %%mm4 \n\t" // 2L2 - L3 + L4 @@ -871,8 +871,8 @@ static inline void RENAME(doVertDefFilte "psubw %%mm2, %%mm0 \n\t" // 2L4 - 5L5 + 5L6 - 2L7 "psubw %%mm3, %%mm1 \n\t" // 2H4 - 5H5 + 5H6 - 2H7
- "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) [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB The worst form of inequality is to try to make unequal things equal. -- Aristotle
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]; that's on 4.1.2, what with other gcc versions - don't know, maybe v4 safer).
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
On 02.02.2010 22:20, Michael Niedermayer wrote:
On Tue, Feb 02, 2010 at 04:47:44PM +0300, Yuriy Kaminskiy wrote: 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 ... Eww, but context here is actually "aligned space on stack" (I think that's why some code can work at all: else gcc would die with "not enough registers" even without -fPIC): ==== cut postprocess_template.c:3163 === static void RENAME(postProcess)(const uint8_t src[], int srcStride, uint8_t dst[], int dstStride, int width, int height, const QP_STORE_T QPs[], int QPStride, int isColor, PPContext *c2) { DECLARE_ALIGNED(8, PPContext, c)= *c2; //copy to stack for faster access [...:3503...] else if(t==2) RENAME(doVertDefFilter)(dstBlock, stride, &c); [...and all other use that &c too...]
On Tue, Feb 02, 2010 at 11:08:01PM +0300, Yuriy Kaminskiy wrote:
On 02.02.2010 22:20, Michael Niedermayer wrote:
On Tue, Feb 02, 2010 at 04:47:44PM +0300, Yuriy Kaminskiy wrote: 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 ... Eww, but context here is actually "aligned space on stack" (I think that's why some code can work at all: else gcc would die with "not enough registers" even without -fPIC): ==== cut postprocess_template.c:3163 === static void RENAME(postProcess)(const uint8_t src[], int srcStride, uint8_t dst[], int dstStride, int width, int height, const QP_STORE_T QPs[], int QPStride, int isColor, PPContext *c2) { DECLARE_ALIGNED(8, PPContext, c)= *c2; //copy to stack for faster access
hmm, i forgot that ... anyway does gcc add additional instructions with your code to align the new variable or not? [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB When the tyrant has disposed of foreign enemies by conquest or treaty, and there is nothing more to fear from them, then he is always stirring up some war or other, in order that the people may require a leader. -- Plato
On Tue, Feb 02, 2010 at 11:08:01PM +0300, Yuriy Kaminskiy wrote:
On 02.02.2010 22:20, Michael Niedermayer wrote:
On Tue, Feb 02, 2010 at 04:47:44PM +0300, Yuriy Kaminskiy wrote: 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 ... Eww, but context here is actually "aligned space on stack" (I think that's why some code can work at all: else gcc would die with "not enough registers" even without -fPIC): ==== cut postprocess_template.c:3163 === static void RENAME(postProcess)(const uint8_t src[], int srcStride, uint8_t dst[], int dstStride, int width, int height, const QP_STORE_T QPs[], int QPStride, int isColor, PPContext *c2) { DECLARE_ALIGNED(8, PPContext, c)= *c2; //copy to stack for faster access hmm, i forgot that ... anyway does gcc add additional instructions with your code to align the new variable or not? hmm. no, it seems uses "aligned" offset, but does not /realign/ stack so if stack /was/ misaligned (($esp % 8)) - it will be bad on other hand, DECLARE_ALIGNED (with same alignment) already used for this
On 02.02.2010 23:12, Michael Niedermayer wrote: ppcontext copy, so i doubt it will make situation worse (and i doubt it is possible - such misaligned stack would be bad for double variables [I know that larger alignment - 16 bytes for SSE - is certainly problematic on some OSes])
On Tue, Feb 02, 2010 at 11:55:46PM +0300, Yuriy Kaminskiy wrote:
On Tue, Feb 02, 2010 at 11:08:01PM +0300, Yuriy Kaminskiy wrote:
On 02.02.2010 22:20, Michael Niedermayer wrote:
On Tue, Feb 02, 2010 at 04:47:44PM +0300, Yuriy Kaminskiy wrote: 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 ... Eww, but context here is actually "aligned space on stack" (I think that's why some code can work at all: else gcc would die with "not enough registers" even without -fPIC): ==== cut postprocess_template.c:3163 === static void RENAME(postProcess)(const uint8_t src[], int srcStride, uint8_t dst[], int dstStride, int width, int height, const QP_STORE_T QPs[], int QPStride, int isColor, PPContext *c2) { DECLARE_ALIGNED(8, PPContext, c)= *c2; //copy to stack for faster access hmm, i forgot that ... anyway does gcc add additional instructions with your code to align the new variable or not? hmm. no, it seems uses "aligned" offset, but does not /realign/ stack so if stack /was/ misaligned (($esp % 8)) - it will be bad on other hand, DECLARE_ALIGNED (with same alignment) already used for this
On 02.02.2010 23:12, Michael Niedermayer wrote: ppcontext copy, so i doubt it will make situation worse (and i doubt it is possible - such misaligned stack would be bad for double variables [I know that larger alignment - 16 bytes for SSE - is certainly problematic on some OSes])
then patch ok [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Frequently ignored awnser#1 FFmpeg bugs should be sent to our bugtracker. User questions about the command line tools should be sent to the ffmpeg-user ML. And questions about how to use libav* should be sent to the libav-user ML.
On Tue, Feb 02, 2010 at 09:57:26PM +0100, Michael Niedermayer wrote:
On Tue, Feb 02, 2010 at 11:55:46PM +0300, Yuriy Kaminskiy wrote:
On Tue, Feb 02, 2010 at 11:08:01PM +0300, Yuriy Kaminskiy wrote:
On 02.02.2010 22:20, Michael Niedermayer wrote:
On Tue, Feb 02, 2010 at 04:47:44PM +0300, Yuriy Kaminskiy wrote: 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 ... Eww, but context here is actually "aligned space on stack" (I think that's why some code can work at all: else gcc would die with "not enough registers" even without -fPIC): ==== cut postprocess_template.c:3163 === static void RENAME(postProcess)(const uint8_t src[], int srcStride, uint8_t dst[], int dstStride, int width, int height, const QP_STORE_T QPs[], int QPStride, int isColor, PPContext *c2) { DECLARE_ALIGNED(8, PPContext, c)= *c2; //copy to stack for faster access hmm, i forgot that ... anyway does gcc add additional instructions with your code to align the new variable or not? hmm. no, it seems uses "aligned" offset, but does not /realign/ stack so if stack /was/ misaligned (($esp % 8)) - it will be bad on other hand, DECLARE_ALIGNED (with same alignment) already used for this
On 02.02.2010 23:12, Michael Niedermayer wrote: ppcontext copy, so i doubt it will make situation worse (and i doubt it is possible - such misaligned stack would be bad for double variables [I know that larger alignment - 16 bytes for SSE - is certainly problematic on some OSes])
then patch ok
v5 applied, if it fails somewhere ill switch to v4 and sorry again for the long delay, it seems everyone forgot about this patch [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I know you won't believe me, but the highest form of Human Excellence is to question oneself and others. -- Socrates
On Tue, Sep 20, 2011 at 09:40:48PM +0200, Michael Niedermayer wrote:
On Tue, Feb 02, 2010 at 09:57:26PM +0100, Michael Niedermayer wrote:
On Tue, Feb 02, 2010 at 11:55:46PM +0300, Yuriy Kaminskiy wrote:
On Tue, Feb 02, 2010 at 11:08:01PM +0300, Yuriy Kaminskiy wrote:
On 02.02.2010 22:20, Michael Niedermayer wrote:
On Tue, Feb 02, 2010 at 04:47:44PM +0300, Yuriy Kaminskiy wrote: 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 ... Eww, but context here is actually "aligned space on stack" (I think that's why some code can work at all: else gcc would die with "not enough registers" even without -fPIC): ==== cut postprocess_template.c:3163 === static void RENAME(postProcess)(const uint8_t src[], int srcStride, uint8_t dst[], int dstStride, int width, int height, const QP_STORE_T QPs[], int QPStride, int isColor, PPContext *c2) { DECLARE_ALIGNED(8, PPContext, c)= *c2; //copy to stack for faster access hmm, i forgot that ... anyway does gcc add additional instructions with your code to align the new variable or not? hmm. no, it seems uses "aligned" offset, but does not /realign/ stack so if stack /was/ misaligned (($esp % 8)) - it will be bad on other hand, DECLARE_ALIGNED (with same alignment) already used for this
On 02.02.2010 23:12, Michael Niedermayer wrote: ppcontext copy, so i doubt it will make situation worse (and i doubt it is possible - such misaligned stack would be bad for double variables [I know that larger alignment - 16 bytes for SSE - is certainly problematic on some OSes])
then patch ok
v5 applied, if it fails somewhere ill switch to v4
v5 fails for ivan with libpostproc/postprocess_template.c: In function 'dering_MMX2': libpostproc/postprocess_template.c:1045:5: error: can't find a register in class 'GENERAL_REGS' while reloading 'asm' libpostproc/postprocess_template.c:1045:5: error: 'asm' operand has impossible constraints v4 works for him, so i will switch to v4 [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I do not agree with what you have to say, but I'll defend to the death your right to say it. -- Voltaire
participants (3)
-
Michael Niedermayer -
michaelni＠gmx.at
-
yumkam＠mail.ru