Sean McGovern: > Andreas: > > On Wed, Mar 13, 2024 at 7:31 AM Andreas Rheinhardt > <andreas.rheinha...@outlook.com> wrote: >> >> Also fix an effective-type violation. >> Exposed by >> https://fate.ffmpeg.org/report.cgi?time=20240312011016&slot=ppc-linux-gcc-13.2-ubsan-altivec-qemu >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinha...@outlook.com> >> --- >> libavcodec/ppc/h264dsp.c | 35 +++++++++++++++++------------------ >> 1 file changed, 17 insertions(+), 18 deletions(-) >> >> diff --git a/libavcodec/ppc/h264dsp.c b/libavcodec/ppc/h264dsp.c >> index c02733dda2..f50f2553a2 100644 >> --- a/libavcodec/ppc/h264dsp.c >> +++ b/libavcodec/ppc/h264dsp.c >> @@ -401,30 +401,29 @@ static inline void write16x4(uint8_t *dst, int >> dst_stride, >> register vec_u8 r0, register vec_u8 r1, >> register vec_u8 r2, register vec_u8 r3) { >> DECLARE_ALIGNED(16, unsigned char, result)[64]; >> - uint32_t *src_int = (uint32_t *)result, *dst_int = (uint32_t *)dst; >> - int int_dst_stride = dst_stride/4; >> + uint32_t *src_int = (uint32_t *)result; >> >> vec_st(r0, 0, result); >> vec_st(r1, 16, result); >> vec_st(r2, 32, result); >> vec_st(r3, 48, result); >> /* FIXME: there has to be a better way!!!! */ >> - *dst_int = *src_int; >> - *(dst_int+ int_dst_stride) = *(src_int + 1); >> - *(dst_int+ 2*int_dst_stride) = *(src_int + 2); >> - *(dst_int+ 3*int_dst_stride) = *(src_int + 3); >> - *(dst_int+ 4*int_dst_stride) = *(src_int + 4); >> - *(dst_int+ 5*int_dst_stride) = *(src_int + 5); >> - *(dst_int+ 6*int_dst_stride) = *(src_int + 6); >> - *(dst_int+ 7*int_dst_stride) = *(src_int + 7); >> - *(dst_int+ 8*int_dst_stride) = *(src_int + 8); >> - *(dst_int+ 9*int_dst_stride) = *(src_int + 9); >> - *(dst_int+10*int_dst_stride) = *(src_int + 10); >> - *(dst_int+11*int_dst_stride) = *(src_int + 11); >> - *(dst_int+12*int_dst_stride) = *(src_int + 12); >> - *(dst_int+13*int_dst_stride) = *(src_int + 13); >> - *(dst_int+14*int_dst_stride) = *(src_int + 14); >> - *(dst_int+15*int_dst_stride) = *(src_int + 15); >> + AV_WN32(dst, AV_RN32A(src_int + 0)); >> + AV_WN32(dst + dst_stride, AV_RN32A(src_int + 1)); >> + AV_WN32(dst + 2 * dst_stride, AV_RN32A(src_int + 2)); >> + AV_WN32(dst + 3 * dst_stride, AV_RN32A(src_int + 3)); >> + AV_WN32(dst + 4 * dst_stride, AV_RN32A(src_int + 4)); >> + AV_WN32(dst + 5 * dst_stride, AV_RN32A(src_int + 5)); >> + AV_WN32(dst + 6 * dst_stride, AV_RN32A(src_int + 6)); >> + AV_WN32(dst + 7 * dst_stride, AV_RN32A(src_int + 7)); >> + AV_WN32(dst + 8 * dst_stride, AV_RN32A(src_int + 8)); >> + AV_WN32(dst + 9 * dst_stride, AV_RN32A(src_int + 9)); >> + AV_WN32(dst + 10 * dst_stride, AV_RN32A(src_int + 10)); >> + AV_WN32(dst + 11 * dst_stride, AV_RN32A(src_int + 11)); >> + AV_WN32(dst + 12 * dst_stride, AV_RN32A(src_int + 12)); >> + AV_WN32(dst + 13 * dst_stride, AV_RN32A(src_int + 13)); >> + AV_WN32(dst + 14 * dst_stride, AV_RN32A(src_int + 14)); >> + AV_WN32(dst + 15 * dst_stride, AV_RN32A(src_int + 15)); >> } >> >> /** @brief performs a 6x16 transpose of data in src, and stores it to dst >> -- >> 2.40.1 >> >> _______________________________________________ >> 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". > > First of all, thank you for looking into this. > > Second, do we feel that this change covers the FIXME immediately above > it that exclaims "there has to be a better way!!!!"? > If so, we can remove the comment.
I don't think so. This code comes from a time when FFmpeg did not care about the effective-type-rules or about alignment due to undefined behaviour; it only cared about alignment when it led to crashes. The old discussion confirms this: https://ffmpeg.org/pipermail/ffmpeg-devel/2007-May/034609.html https://ffmpeg.org/pipermail/ffmpeg-devel/2007-May/034612.html contains the following: "As I said, I submitted this patch in order to have PPC users get some speed-up now rather than having a hypothetic optimal code when some of us who work on Altivec sit down and work on it. I do think it's better to have a committed faster code that leaves room for improvement than a fastest code that never sees the light." The fixme relates to this; it was probably considered advantageous to avoid storing the vectors in stack buffers. > > I did not perform a full FATE run as it is expensive on my QEMU setup, > but I can confirm that this fixes the checkasm-h264dsp test under GCC > UBsan there as well as on a POWER7 (ppc64) and a POWER9 (ppc64le). > Ok, will apply then. Thanks for testing. - Andreas _______________________________________________ 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".
