| Message ID | 20200122145210.6898-1-andreas.rheinhardt@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel,1/4] avcodec/startcode: Use AV_RN due to UBSan warning about unaligned access | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
Am Mi., 22. Jan. 2020 um 15:57 Uhr schrieb Andreas Rheinhardt <andreas.rheinhardt@gmail.com>: > > when directly accessing a buffer via a pointer to uint8_t cast to a > pointer to uint64_t/uint32_t. This happened only if HAVE_FAST_UNALIGNED > was set, so it was ok, but UBSan nevertheless complained about unaligned > accesses. So simply use AV_RNxx to read the buffer; this also improves > readability. Is there a speed impact? Carl Eugen
On Wed, Jan 22, 2020 at 4:06 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > Am Mi., 22. Jan. 2020 um 15:57 Uhr schrieb Andreas Rheinhardt > <andreas.rheinhardt@gmail.com>: > > > > when directly accessing a buffer via a pointer to uint8_t cast to a > > pointer to uint64_t/uint32_t. This happened only if HAVE_FAST_UNALIGNED > > was set, so it was ok, but UBSan nevertheless complained about unaligned > > accesses. So simply use AV_RNxx to read the buffer; this also improves > > readability. > > Is there a speed impact? > > The assembly produced by both Clang as well as GCC was completely unchanged (for non-UBSan builds). - Andreas
Am Mi., 22. Jan. 2020 um 16:21 Uhr schrieb Andreas Rheinhardt <andreas.rheinhardt@gmail.com>: > > On Wed, Jan 22, 2020 at 4:06 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > > > Am Mi., 22. Jan. 2020 um 15:57 Uhr schrieb Andreas Rheinhardt > > <andreas.rheinhardt@gmail.com>: > > > > > > when directly accessing a buffer via a pointer to uint8_t cast to a > > > pointer to uint64_t/uint32_t. This happened only if HAVE_FAST_UNALIGNED > > > was set, so it was ok, but UBSan nevertheless complained about unaligned > > > accesses. So simply use AV_RNxx to read the buffer; this also improves > > > readability. > > > > Is there a speed impact? > > > The assembly produced by both Clang as well as GCC was completely > unchanged (for non-UBSan builds). (on x86?) Great! Carl Eugen
On Wed, Jan 22, 2020 at 4:23 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > Am Mi., 22. Jan. 2020 um 16:21 Uhr schrieb Andreas Rheinhardt > <andreas.rheinhardt@gmail.com>: > > > > On Wed, Jan 22, 2020 at 4:06 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> > wrote: > > > > > Am Mi., 22. Jan. 2020 um 15:57 Uhr schrieb Andreas Rheinhardt > > > <andreas.rheinhardt@gmail.com>: > > > > > > > > when directly accessing a buffer via a pointer to uint8_t cast to a > > > > pointer to uint64_t/uint32_t. This happened only if > HAVE_FAST_UNALIGNED > > > > was set, so it was ok, but UBSan nevertheless complained about > unaligned > > > > accesses. So simply use AV_RNxx to read the buffer; this also > improves > > > > readability. > > > > > > Is there a speed impact? > > > > > The assembly produced by both Clang as well as GCC was completely > > unchanged (for non-UBSan builds). > > (on x86?) > > Yes: x86-64. - Andreas
Andreas Rheinhardt: > when directly accessing a buffer via a pointer to uint8_t cast to a > pointer to uint64_t/uint32_t. This happened only if HAVE_FAST_UNALIGNED > was set, so it was ok, but UBSan nevertheless complained about unaligned > accesses. So simply use AV_RNxx to read the buffer; this also improves > readability. > > This affected many FATE-tests (namely 228 of them; e.g. the cbs-h264 > tests) and it has also been reported in tickets #8138 and #8485. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavcodec/startcode.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/libavcodec/startcode.c b/libavcodec/startcode.c > index 9efdffe8c6..d84f326521 100644 > --- a/libavcodec/startcode.c > +++ b/libavcodec/startcode.c > @@ -25,6 +25,7 @@ > * @author Michael Niedermayer <michaelni@gmx.at> > */ > > +#include "libavutil/intreadwrite.h" > #include "startcode.h" > #include "config.h" > > @@ -38,14 +39,14 @@ int ff_startcode_find_candidate_c(const uint8_t *buf, int size) > */ > #if HAVE_FAST_64BIT > while (i < size && > - !((~*(const uint64_t *)(buf + i) & > - (*(const uint64_t *)(buf + i) - 0x0101010101010101ULL)) & > + !((~AV_RN64(buf + i) & > + (AV_RN64(buf + i) - 0x0101010101010101ULL)) & > 0x8080808080808080ULL)) > i += 8; > #else > while (i < size && > - !((~*(const uint32_t *)(buf + i) & > - (*(const uint32_t *)(buf + i) - 0x01010101U)) & > + !((~AV_RN32(buf + i) & > + (AV_RN32(buf + i) - 0x01010101U)) & > 0x80808080U)) > i += 4; > #endif > Ping for this and the other unmerged patches of this patchset. - Andreas
On Wed, Jan 22, 2020 at 04:25:40PM +0100, Andreas Rheinhardt wrote: > On Wed, Jan 22, 2020 at 4:23 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > > > Am Mi., 22. Jan. 2020 um 16:21 Uhr schrieb Andreas Rheinhardt > > <andreas.rheinhardt@gmail.com>: > > > > > > On Wed, Jan 22, 2020 at 4:06 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> > > wrote: > > > > > > > Am Mi., 22. Jan. 2020 um 15:57 Uhr schrieb Andreas Rheinhardt > > > > <andreas.rheinhardt@gmail.com>: > > > > > > > > > > when directly accessing a buffer via a pointer to uint8_t cast to a > > > > > pointer to uint64_t/uint32_t. This happened only if > > HAVE_FAST_UNALIGNED > > > > > was set, so it was ok, but UBSan nevertheless complained about > > unaligned > > > > > accesses. So simply use AV_RNxx to read the buffer; this also > > improves > > > > > readability. > > > > > > > > Is there a speed impact? > > > > > > > The assembly produced by both Clang as well as GCC was completely > > > unchanged (for non-UBSan builds). > > > > (on x86?) > > > > Yes: x86-64. are any other architectures affected or is it the same for all major ones? thx [...]
diff --git a/libavcodec/startcode.c b/libavcodec/startcode.c index 9efdffe8c6..d84f326521 100644 --- a/libavcodec/startcode.c +++ b/libavcodec/startcode.c @@ -25,6 +25,7 @@ * @author Michael Niedermayer <michaelni@gmx.at> */ +#include "libavutil/intreadwrite.h" #include "startcode.h" #include "config.h" @@ -38,14 +39,14 @@ int ff_startcode_find_candidate_c(const uint8_t *buf, int size) */ #if HAVE_FAST_64BIT while (i < size && - !((~*(const uint64_t *)(buf + i) & - (*(const uint64_t *)(buf + i) - 0x0101010101010101ULL)) & + !((~AV_RN64(buf + i) & + (AV_RN64(buf + i) - 0x0101010101010101ULL)) & 0x8080808080808080ULL)) i += 8; #else while (i < size && - !((~*(const uint32_t *)(buf + i) & - (*(const uint32_t *)(buf + i) - 0x01010101U)) & + !((~AV_RN32(buf + i) & + (AV_RN32(buf + i) - 0x01010101U)) & 0x80808080U)) i += 4; #endif
when directly accessing a buffer via a pointer to uint8_t cast to a pointer to uint64_t/uint32_t. This happened only if HAVE_FAST_UNALIGNED was set, so it was ok, but UBSan nevertheless complained about unaligned accesses. So simply use AV_RNxx to read the buffer; this also improves readability. This affected many FATE-tests (namely 228 of them; e.g. the cbs-h264 tests) and it has also been reported in tickets #8138 and #8485. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- libavcodec/startcode.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)