Thank you for your answers. As you have pointed out, these patches are full of unrelated changes that are not important for safety. Most of them were never intended to be posted here, the diff we submitted was the one of an experimental branch and we apologize to have made such a mistake. We will resubmit the patch with only essential patches in a more appropriate format very soon (git format-patch).
@Michael These errors come with the Clang compiler, aren't they? We are aware that support for inline assembly may differ from one compiler to another despite the "higly-compatibility" that is stated. The safety patches we are proposing do not rely on them. @Carl @Kieran So far, we passed the FATE tests. The output is slightly different because we have merged contiguous assembly statement such that the compiler can no longer insert instruction between them, but the differences are only instruction swaps A.B.C -> B.A.C. Frédéric Recoules De: "Michael Niedermayer" <mich...@niedermayer.cc> À: "ffmpeg-devel" <ffmpeg-devel@ffmpeg.org> Envoyé: Samedi 4 Avril 2020 00:39:13 Objet: Re: [FFmpeg-devel] [inline assembly compliance] Issues and patches On Fri, Apr 03, 2020 at 10:41:58PM +0200, FRÉDÉRIC RECOULES wrote: > Dear developpers, > > > > we are academic researchers working in automated program analysis. > We are currently interested in checking compliance of inline asm chunks > as found in C programs. > > While benchmarking our tool and technique, we found a number of issues in > FFMPEG. We report them to you, as well as adequate patches. > Actually, we found 59 significant compliance issues in your code. > We join 3 patches for some of them, together with explanations and > we can send you other patches on demand. > > > * All these bugs are related to compliance between the block of asm and its > surrounding "contract" (in gcc-style notation). They are akin to undefined or > implementation-defined behaviours in C: they currently do not manifest > themselves in your program, but at some point in time with compiler > optimizations becoming more and more aggressive or changes in undocumented > compiler choices regarding asm chunks, they can suddenly trigger a > (hard-to-find) bug. > > * The typical problems come from the compiler missing dataflow information > and performing undue optimizations on this wrong basis, or the compiler > allocating an already used register. Actually, we demonstrate "in lab" > problems > with all these categories of bugs in case of inlining > (especially with LTO enabler) or code refactoring. > > * Some of those issues may seems benign or irrealistic but it cost nothing > to patch so, why not do it? > > > We would be very interested to hear your opinion on these matters. > Are you interested in such errors and patches? > Also, besides the patches, we are currently working on a code analyzer > prototype designed to check asm compliance and to propose patches when the > chunk is not compliant. This is still work in progress and we are finalizing > it. > The errors and patches I reported to you came from my prototype. > In case such a prototype would be made available, would you consider using > it? > > > > Best regards > > Frédéric Recoules This breaks build with shared libs In file included from src/libavcodec/x86/hpeldsp_init.c:104: src/libavcodec/x86/rnd_template.c:102:30: error: dereference of pointer to incomplete type 'const uint8_t []' "m" (*(const uint8_t (*)[])pixels), ^ src/libavcodec/x86/rnd_template.c:188:30: error: dereference of pointer to incomplete type 'const uint8_t []' "m" (*(const uint8_t (*)[])pixels), ^ In file included from src/libavcodec/x86/hpeldsp_init.c:139: src/libavcodec/x86/rnd_template.c:102:30: error: dereference of pointer to incomplete type 'const uint8_t []' "m" (*(const uint8_t (*)[])pixels), ^ src/libavcodec/x86/rnd_template.c:188:30: error: dereference of pointer to incomplete type 'const uint8_t []' "m" (*(const uint8_t (*)[])pixels), ^ 4 errors generated. src/ffbuild/common.mak:59: recipe for target 'libavcodec/x86/hpeldsp_init.o' failed make: *** [libavcodec/x86/hpeldsp_init.o] Error 1 [...] > diff --git a/libavcodec/x86/rnd_template.c b/libavcodec/x86/rnd_template.c > index 09946bd23f..1be010e066 100644 > --- a/libavcodec/x86/rnd_template.c > +++ b/libavcodec/x86/rnd_template.c > @@ -30,146 +30,164 @@ > #include "inline_asm.h" > > // put_pixels > -av_unused STATIC void DEF(put, pixels8_xy2)(uint8_t *block, const uint8_t > *pixels, > - ptrdiff_t line_size, int h) > +av_unused STATIC void DEF(put, pixels8_xy2)(uint8_t *block, > + const uint8_t *pixels, > + ptrdiff_t line_size, int h) unrelated whitespace change > { > - MOVQ_ZERO(mm7); > - SET_RND(mm6); // =2 for rnd and =1 for no_rnd version > - __asm__ volatile( > - "movq (%1), %%mm0 \n\t" > - "movq 1(%1), %%mm4 \n\t" > - "movq %%mm0, %%mm1 \n\t" > - "movq %%mm4, %%mm5 \n\t" > - "punpcklbw %%mm7, %%mm0 \n\t" > - "punpcklbw %%mm7, %%mm4 \n\t" > - "punpckhbw %%mm7, %%mm1 \n\t" > - "punpckhbw %%mm7, %%mm5 \n\t" > - "paddusw %%mm0, %%mm4 \n\t" > - "paddusw %%mm1, %%mm5 \n\t" > - "xor %%"FF_REG_a", %%"FF_REG_a" \n\t" > - "add %3, %1 \n\t" > - ".p2align 3 \n\t" > - "1: \n\t" > - "movq (%1, %%"FF_REG_a"), %%mm0 \n\t" > - "movq 1(%1, %%"FF_REG_a"), %%mm2 \n\t" > - "movq %%mm0, %%mm1 \n\t" > - "movq %%mm2, %%mm3 \n\t" > - "punpcklbw %%mm7, %%mm0 \n\t" > - "punpcklbw %%mm7, %%mm2 \n\t" > - "punpckhbw %%mm7, %%mm1 \n\t" > - "punpckhbw %%mm7, %%mm3 \n\t" > - "paddusw %%mm2, %%mm0 \n\t" > - "paddusw %%mm3, %%mm1 \n\t" > - "paddusw %%mm6, %%mm4 \n\t" > - "paddusw %%mm6, %%mm5 \n\t" > - "paddusw %%mm0, %%mm4 \n\t" > - "paddusw %%mm1, %%mm5 \n\t" > - "psrlw $2, %%mm4 \n\t" > - "psrlw $2, %%mm5 \n\t" > - "packuswb %%mm5, %%mm4 \n\t" > - "movq %%mm4, (%2, %%"FF_REG_a") \n\t" > - "add %3, %%"FF_REG_a" \n\t" > + x86_reg i = 0; > + __asm__ ( > + MOVQ_ZERO_TPL(mm7) > + SET_RND_TPL(mm6) // =2 for rnd and =1 for no_rnd version > + "movq (%[pixels]), %%mm0 \n\t" > + "movq 1(%[pixels]), %%mm4 \n\t" > + "movq %%mm0, %%mm1 \n\t" > + "movq %%mm4, %%mm5 \n\t" > + "punpcklbw %%mm7, %%mm0 \n\t" > + "punpcklbw %%mm7, %%mm4 \n\t" > + "punpckhbw %%mm7, %%mm1 \n\t" > + "punpckhbw %%mm7, %%mm5 \n\t" > + "paddusw %%mm0, %%mm4 \n\t" > + "paddusw %%mm1, %%mm5 \n\t" > + "add %[line_size], %[pixels] \n\t" > + ".p2align 3 \n\t" > + "1: \n\t" > + "movq (%[pixels], %[i]), %%mm0 \n\t" > + "movq 1(%[pixels], %[i]), %%mm2 \n\t" > + "movq %%mm0, %%mm1 \n\t" > + "movq %%mm2, %%mm3 \n\t" > + "punpcklbw %%mm7, %%mm0 \n\t" > + "punpcklbw %%mm7, %%mm2 \n\t" > + "punpckhbw %%mm7, %%mm1 \n\t" > + "punpckhbw %%mm7, %%mm3 \n\t" > + "paddusw %%mm2, %%mm0 \n\t" > + "paddusw %%mm3, %%mm1 \n\t" > + "paddusw %%mm6, %%mm4 \n\t" > + "paddusw %%mm6, %%mm5 \n\t" > + "paddusw %%mm0, %%mm4 \n\t" > + "paddusw %%mm1, %%mm5 \n\t" > + "psrlw $2, %%mm4 \n\t" > + "psrlw $2, %%mm5 \n\t" > + "packuswb %%mm5, %%mm4 \n\t" > + "movq %%mm4, (%[block], %[i]) \n\t" > + "add %[line_size], %[i] \n\t" this is full of unrelated reformating, making the patchset unreviewable each class of fixes should be in its own patch probably too but especially whitespace changes / reformating should be in a seperate patch so the reviewer can see what is changed Also its very important that the patch makes it clear not just what is changed but WHY each individual change is done. You may be assuming this is sloppy written code and do a few quick fixes and this is true for some parts. Bbut some of the inline asm code we had is written as it is because writing it "nicer" didnt work with some compilers. Especially anything that requires the compiler to be smart and optimize code or find the one possible solution can be a problem. I wanted to mention a few more things like nasm/yasm and when they might be worth looking at but i think thats a few steps too much at once. lets first see what fixes there actually are Can you resubmit thispatchset in a more reviewable form ? Thanks [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB If the United States is serious about tackling the national security threats related to an insecure 5G network, it needs to rethink the extent to which it values corporate profits and government espionage over security.-Bruce Schneier _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".