[FFmpeg-devel] [PATCH 1/5] lavu/common.h: Fix UB in av_clipl_int32_c()

[FFmpeg-devel] [PATCH 1/5] lavu/common.h: Fix UB in av_clipl_int32_c()

[Tomas Härdin]

[-- Attachment #1: Type: text/plain, Size: 40 bytes --]

The entire patchset passes FATE

/Tomas

[-- Attachment #2: 0001-lavu-common.h-Fix-UB-in-av_clipl_int32_c.patch --]
[-- Type: text/x-patch, Size: 996 bytes --]

From c000b8a5e90883f28ce6c58960227e5825ac20d1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <git@haerdin.se>
Date: Wed, 15 May 2024 21:03:47 +0200
Subject: [PATCH 1/5] lavu/common.h: Fix UB in av_clipl_int32_c()

Found by value analysis
---
 libavutil/common.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libavutil/common.h b/libavutil/common.h
index 3e4c339893..ac68c0cfff 100644
--- a/libavutil/common.h
+++ b/libavutil/common.h
@@ -252,8 +252,8 @@ static av_always_inline av_const int16_t av_clip_int16_c(int a)
  */
 static av_always_inline av_const int32_t av_clipl_int32_c(int64_t a)
 {
-    if ((a+0x80000000u) & ~UINT64_C(0xFFFFFFFF)) return (int32_t)((a>>63) ^ 0x7FFFFFFF);
-    else                                         return (int32_t)a;
+    if ((a+UINT64_C(0x80000000)) & ~UINT64_C(0xFFFFFFFF)) return (int32_t)((a>>63) ^ 0x7FFFFFFF);
+    else                                                  return (int32_t)a;
 }
 
 /**
-- 
2.39.2


[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 2/5] lavu/common.h: Fix UB in av_clip_intp2_c()

[Tomas Härdin]

[-- Attachment #1: Type: text/plain, Size: 1 bytes --]



[-- Attachment #2: 0002-lavu-common.h-Fix-UB-in-av_clip_intp2_c.patch --]
[-- Type: text/x-patch, Size: 829 bytes --]

From 7b18f24c0bedfeebcdfb23ea837cea8d4c35cf30 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <git@haerdin.se>
Date: Thu, 16 May 2024 16:33:44 +0200
Subject: [PATCH 2/5] lavu/common.h: Fix UB in av_clip_intp2_c()

Found by value analysis
---
 libavutil/common.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavutil/common.h b/libavutil/common.h
index ac68c0cfff..715f0a594c 100644
--- a/libavutil/common.h
+++ b/libavutil/common.h
@@ -264,7 +264,7 @@ static av_always_inline av_const int32_t av_clipl_int32_c(int64_t a)
  */
 static av_always_inline av_const int av_clip_intp2_c(int a, int p)
 {
-    if (((unsigned)a + (1 << p)) & ~((2 << p) - 1))
+    if (((unsigned)a + (1U << p)) & ~((2U << p) - 1))
         return (a >> 31) ^ ((1 << p) - 1);
     else
         return a;
-- 
2.39.2


[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/5] lavu/common.h: Fix UB in av_clip_intp2_c()

[Andreas Rheinhardt]

Tomas Härdin:
>  static av_always_inline av_const int av_clip_intp2_c(int a, int p)
>  {
> -    if (((unsigned)a + (1 << p)) & ~((2 << p) - 1))
> +    if (((unsigned)a + (1U << p)) & ~((2U << p) - 1))
>          return (a >> 31) ^ ((1 << p) - 1);
>      else
>          return a;

This will support p == 30 (but not 31); but the first change is not UB
in this range.

- 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-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/5] lavu/common.h: Fix UB in av_clip_intp2_c()

[Tomas Härdin]

tor 2024-05-30 klockan 00:24 +0200 skrev Andreas Rheinhardt:
> Tomas Härdin:
> >  static av_always_inline av_const int av_clip_intp2_c(int a, int p)
> >  {
> > -    if (((unsigned)a + (1 << p)) & ~((2 << p) - 1))
> > +    if (((unsigned)a + (1U << p)) & ~((2U << p) - 1))
> >          return (a >> 31) ^ ((1 << p) - 1);
> >      else
> >          return a;
> 
> This will support p == 30 (but not 31); but the first change is not
> UB
> in this range.

p=31 is most definitely UB before this change. 1<<31 is signed overflow
with 32-bit int. The compiler has therefore been allowed to do whatever
for p=31 up until this point. To me it seems the intent of the code is
preserved

Personally I dislike bithacks because they are difficult to verify. A
good enough compiler will gain peephole optimizations for them sooner
or later anyway

/Tomas

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 3/5] lavu/common.h: Fix UB in av_clip_uintp2_c()

[Tomas Härdin]

[-- Attachment #1: Type: text/plain, Size: 1 bytes --]



[-- Attachment #2: 0003-lavu-common.h-Fix-UB-in-av_clip_uintp2_c.patch --]
[-- Type: text/x-patch, Size: 872 bytes --]

From f81730f8facc54ef23df79ac8d33075403b4f76f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <git@haerdin.se>
Date: Thu, 16 May 2024 16:37:58 +0200
Subject: [PATCH 3/5] lavu/common.h: Fix UB in av_clip_uintp2_c()

Found by value analysis
---
 libavutil/common.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libavutil/common.h b/libavutil/common.h
index 715f0a594c..8a3c4d2fcf 100644
--- a/libavutil/common.h
+++ b/libavutil/common.h
@@ -278,8 +278,8 @@ static av_always_inline av_const int av_clip_intp2_c(int a, int p)
  */
 static av_always_inline av_const unsigned av_clip_uintp2_c(int a, int p)
 {
-    if (a & ~((1<<p) - 1)) return (~a) >> 31 & ((1<<p) - 1);
-    else                   return  a;
+    if (a & ~((1U<<p) - 1)) return (~a) >> 31 & ((1U<<p) - 1);
+    else                    return  a;
 }
 
 /**
-- 
2.39.2


[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 3/5] lavu/common.h: Fix UB in av_clip_uintp2_c()

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1434 bytes --]

On Thu, May 30, 2024 at 12:14:09AM +0200, Tomas Härdin wrote:
> 

>  common.h |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 1fdb9cd4a83522921c2b15a1e76ff2f65ef61f57  0003-lavu-common.h-Fix-UB-in-av_clip_uintp2_c.patch
> From f81730f8facc54ef23df79ac8d33075403b4f76f Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <git@haerdin.se>
> Date: Thu, 16 May 2024 16:37:58 +0200
> Subject: [PATCH 3/5] lavu/common.h: Fix UB in av_clip_uintp2_c()
> 
> Found by value analysis
> ---
>  libavutil/common.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libavutil/common.h b/libavutil/common.h
> index 715f0a594c..8a3c4d2fcf 100644
> --- a/libavutil/common.h
> +++ b/libavutil/common.h
> @@ -278,8 +278,8 @@ static av_always_inline av_const int av_clip_intp2_c(int a, int p)
>   */
>  static av_always_inline av_const unsigned av_clip_uintp2_c(int a, int p)
>  {
> -    if (a & ~((1<<p) - 1)) return (~a) >> 31 & ((1<<p) - 1);
> -    else                   return  a;
> +    if (a & ~((1U<<p) - 1)) return (~a) >> 31 & ((1U<<p) - 1);
> +    else                    return  a;
>  }

LGTM

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In fact, the RIAA has been known to suggest that students drop out
of college or go to community college in order to be able to afford
settlements. -- The RIAA

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 4/5] lavu/intmath.h: Fix UB in ff_ctz_c() and ff_ctzll_c()

[Tomas Härdin]

[-- Attachment #1: Type: text/plain, Size: 1 bytes --]



[-- Attachment #2: 0004-lavu-intmath.h-Fix-UB-in-ff_ctz_c-and-ff_ctzll_c.patch --]
[-- Type: text/x-patch, Size: 1294 bytes --]

From f9a12089bc98dde0ccc2487d1442ec6ddb7705f6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <git@haerdin.se>
Date: Thu, 16 May 2024 18:10:58 +0200
Subject: [PATCH 4/5] lavu/intmath.h: Fix UB in ff_ctz_c() and ff_ctzll_c()

Found by value analysis
---
 libavutil/intmath.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libavutil/intmath.h b/libavutil/intmath.h
index c54d23b7bf..52e11a8d5f 100644
--- a/libavutil/intmath.h
+++ b/libavutil/intmath.h
@@ -119,7 +119,7 @@ static av_always_inline av_const int ff_ctz_c(int v)
         0, 1, 28, 2, 29, 14, 24, 3, 30, 22, 20, 15, 25, 17, 4, 8,
         31, 27, 13, 23, 21, 19, 16, 7, 26, 12, 18, 6, 11, 5, 10, 9
     };
-    return debruijn_ctz32[(uint32_t)((v & -v) * 0x077CB531U) >> 27];
+    return debruijn_ctz32[(uint32_t)((v & -(uint32_t)v) * 0x077CB531U) >> 27];
 }
 #endif
 
@@ -135,7 +135,7 @@ static av_always_inline av_const int ff_ctzll_c(long long v)
         63, 52, 6, 26, 37, 40, 33, 47, 61, 45, 43, 21, 23, 58, 17, 10,
         51, 25, 36, 32, 60, 20, 57, 16, 50, 31, 19, 15, 30, 14, 13, 12
     };
-    return debruijn_ctz64[(uint64_t)((v & -v) * 0x022FDD63CC95386DU) >> 58];
+    return debruijn_ctz64[(uint64_t)((v & -(uint64_t)v) * 0x022FDD63CC95386DU) >> 58];
 }
 #endif
 
-- 
2.39.2


[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/5] lavu/intmath.h: Fix UB in ff_ctz_c() and ff_ctzll_c()

[Rémi Denis-Courmont]

Can't we just use the compiler built-ins here? AFAIK, they (GCC, LLVM) use the same algorithm if the CPU doesn't support native CTZ. And they will pick the right instruction if CPU does have CTZ.

I get it that maybe it wasn't working so well 20 years ago, but we've increased compiler version requirements since then.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/5] lavu/intmath.h: Fix UB in ff_ctz_c() and ff_ctzll_c()

[Tomas Härdin]

tor 2024-05-30 klockan 10:54 +0300 skrev Rémi Denis-Courmont:
> Can't we just use the compiler built-ins here? AFAIK, they (GCC,
> LLVM) use the same algorithm if the CPU doesn't support native CTZ.
> And they will pick the right instruction if CPU does have CTZ.
> 
> I get it that maybe it wasn't working so well 20 years ago, but we've
> increased compiler version requirements since then.

I think we still support MSVC, but maybe we shouldn't? It's possible to
cross-compile for Windows either way.

/Tomas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/5] lavu/intmath.h: Fix UB in ff_ctz_c() and ff_ctzll_c()

[Hendrik Leppkes]

On Thu, May 30, 2024 at 11:50 AM Tomas Härdin <git@haerdin.se> wrote:
>
> tor 2024-05-30 klockan 10:54 +0300 skrev Rémi Denis-Courmont:
> > Can't we just use the compiler built-ins here? AFAIK, they (GCC,
> > LLVM) use the same algorithm if the CPU doesn't support native CTZ.
> > And they will pick the right instruction if CPU does have CTZ.
> >
> > I get it that maybe it wasn't working so well 20 years ago, but we've
> > increased compiler version requirements since then.
>
> I think we still support MSVC, but maybe we shouldn't? It's possible to
> cross-compile for Windows either way.
>

This is not going to happen.

- Hendrik
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/5] lavu/intmath.h: Fix UB in ff_ctz_c() and ff_ctzll_c()

[Rémi Denis-Courmont]

Le 30 mai 2024 12:50:05 GMT+03:00, "Tomas Härdin" <git@haerdin.se> a écrit :
>tor 2024-05-30 klockan 10:54 +0300 skrev Rémi Denis-Courmont:
>> Can't we just use the compiler built-ins here? AFAIK, they (GCC,
>> LLVM) use the same algorithm if the CPU doesn't support native CTZ.
>> And they will pick the right instruction if CPU does have CTZ.
>> 
>> I get it that maybe it wasn't working so well 20 years ago, but we've
>> increased compiler version requirements since then.
>
>I think we still support MSVC, but maybe we shouldn't? It's possible to
>cross-compile for Windows either way.

I don't get how that prevents using the GCC and Clang builtins (on GCC and Clang).
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/5] lavu/intmath.h: Fix UB in ff_ctz_c() and ff_ctzll_c()

[Tomas Härdin]

tor 2024-05-30 klockan 16:06 +0300 skrev Rémi Denis-Courmont:
> 
> 
> Le 30 mai 2024 12:50:05 GMT+03:00, "Tomas Härdin" <git@haerdin.se> a
> écrit :
> > tor 2024-05-30 klockan 10:54 +0300 skrev Rémi Denis-Courmont:
> > > Can't we just use the compiler built-ins here? AFAIK, they (GCC,
> > > LLVM) use the same algorithm if the CPU doesn't support native
> > > CTZ.
> > > And they will pick the right instruction if CPU does have CTZ.
> > > 
> > > I get it that maybe it wasn't working so well 20 years ago, but
> > > we've
> > > increased compiler version requirements since then.
> > 
> > I think we still support MSVC, but maybe we shouldn't? It's
> > possible to
> > cross-compile for Windows either way.
> 
> I don't get how that prevents using the GCC and Clang builtins (on
> GCC and Clang).

Does MSVC have builtins for these? Do all compilers we support?

/Tomas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/5] lavu/intmath.h: Fix UB in ff_ctz_c() and ff_ctzll_c()

[Rémi Denis-Courmont]

Le 30 mai 2024 17:03:09 GMT+03:00, "Tomas Härdin" <git@haerdin.se> a écrit :
>> I don't get how that prevents using the GCC and Clang builtins (on
>> GCC and Clang).
>
>Does MSVC have builtins for these?

I don't know, but insofar as MSVC is used for x86, it should use x86 instructions rather than the complex fallback algo anyway, be it via built-ins, or assembler.

Either way, I don't see how that detracts from using the built-ins on compilers that do have them.

> Do all compilers we support?

No, unless all other compilers are C23 (CTZ and CLZ are in stdbit.h). Again, that's hardly a reason not to use built-ins where available.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/5] lavu/intmath.h: Fix UB in ff_ctz_c() and ff_ctzll_c()

[Ronald S. Bultje]

Hi,

On Thu, May 30, 2024 at 10:03 AM Tomas Härdin <git@haerdin.se> wrote:

> tor 2024-05-30 klockan 16:06 +0300 skrev Rémi Denis-Courmont:
> >
> >
> > Le 30 mai 2024 12:50:05 GMT+03:00, "Tomas Härdin" <git@haerdin.se> a
> > écrit :
> > > tor 2024-05-30 klockan 10:54 +0300 skrev Rémi Denis-Courmont:
> > > > Can't we just use the compiler built-ins here? AFAIK, they (GCC,
> > > > LLVM) use the same algorithm if the CPU doesn't support native
> > > > CTZ.
> > > > And they will pick the right instruction if CPU does have CTZ.
> > > >
> > > > I get it that maybe it wasn't working so well 20 years ago, but
> > > > we've
> > > > increased compiler version requirements since then.
> > >
> > > I think we still support MSVC, but maybe we shouldn't? It's
> > > possible to
> > > cross-compile for Windows either way.
> >
> > I don't get how that prevents using the GCC and Clang builtins (on
> > GCC and Clang).
>
> Does MSVC have builtins for these? Do all compilers we support?
>

I think what Remi is suggesting is that someone (maybe Remi himself, maybe
you, maybe me, maybe someone else) should send a patch to use the built-ins
when available. Where not, we would continue using the C versions.

All of this is unrelated to this patch, which would continue to be useful
for the C fallback.

Ronald
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/5] lavu/intmath.h: Fix UB in ff_ctz_c() and ff_ctzll_c()

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1521 bytes --]

On Thu, May 30, 2024 at 10:54:46AM +0300, Rémi Denis-Courmont wrote:
> Can't we just use the compiler built-ins here? AFAIK, they (GCC, LLVM) use the same algorithm if the CPU doesn't support native CTZ. And they will pick the right instruction if CPU does have CTZ.
> 
> I get it that maybe it wasn't working so well 20 years ago, but we've increased compiler version requirements since then.

ffmpeg is written in C not GNU-C nor LLVM-C
so we need to have non buggy C code

A modern compiler should turn a built-in into a efficient piece of code
but so should it recognize that efficient piece of code and turn it into
a single instruction if such exist.

In the end with a modern compiler it shouldnt matter how you write this
a loop, some optimized standard implementation or a built in
the disavantage of a builtin is that it requires #ifs and checks
for each compiler.
So we should go with the simplest/cleanest that reliably produces optimal code
across all supported platforms (and also consider potential future platforms
so we dont have a maintaince nightmare but something that we can write once
and expect it will be close to optimal forever)

Above though is off topic in this thread, which was a bugfix that we need anyway
(also for backporting)

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Does the universe only have a finite lifespan? No, its going to go on
forever, its just that you wont like living in it. -- Hiranya Peiri

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/5] lavu/intmath.h: Fix UB in ff_ctz_c() and ff_ctzll_c()

[Rémi Denis-Courmont]

Le 31 mai 2024 03:41:40 GMT+03:00, Michael Niedermayer <michael@niedermayer.cc> a écrit :
>On Thu, May 30, 2024 at 10:54:46AM +0300, Rémi Denis-Courmont wrote:
>> Can't we just use the compiler built-ins here? AFAIK, they (GCC, LLVM) use the same algorithm if the CPU doesn't support native CTZ. And they will pick the right instruction if CPU does have CTZ.
>> 
>> I get it that maybe it wasn't working so well 20 years ago, but we've increased compiler version requirements since then.
>
>ffmpeg is written in C not GNU-C nor LLVM-C
>so we need to have non buggy C code

What does that mean here?

We can put compilers in 3 sets:
1) GCC+Clang
2) MSVC+ICC
3) others

Note that ICC and others are *not* tested (at least in FATE). MSVC and ICC are using intrinsics (in x86/intmath.h).

The problem is, GCC and Clang use intrinsics only if fast_clz is manually selected. This is silly. That's just requiring more platform-specific code added for no reasons.

If you want to test this code, DO write a test that calls the C version always. DO NOT force all unknown or unoptimised FFmpeg platforms to use the C just because we need to keep supporting hypothetical C11-conforming C compilers.

Note that I never said that we should remove the standard code.

>A modern compiler should turn a built-in into a efficient piece of code
>but so should it recognize that efficient piece of code and turn it into
>a single instruction if such exist.

I doubt any compiler will detect that the Debujin algorithm can be replaced by a CTZ.  The *official* standard solution is to use <stdbit.h> (C23-only unfortunately), so there are no reasons why compilers should even care to deal with this by now.

>In the end with a modern compiler it shouldnt matter how you write this
>a loop, some optimized standard implementation or a built in
>the disavantage of a builtin is that it requires #ifs and checks
>for each compiler.

A modern compiler supports STDC bit-wise functions from C23, and therefore doesn't care about this.

>So we should go with the simplest/cleanest that reliably produces optimal code
>across all supported platforms (and also consider potential future platforms
>so we dont have a maintaince nightmare but something that we can write once
>and expect it will be close to optimal forever)

So what do you do about the existing x86 special cases and the fast_clz case?

AFAICT, the only way to (arguably) do that is to switch to the C23 bit manipulation functions, and provide fallback for old compilers.

That's clearly out of the scope of this patch, and I bet some people would object anyway because they'll find the names of the new functions ugly and/or too long.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/5] lavu/intmath.h: Fix UB in ff_ctz_c() and ff_ctzll_c()

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1014 bytes --]

On Thu, May 30, 2024 at 12:14:23AM +0200, Tomas Härdin wrote:
> 

>  intmath.h |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 5ae92c415924602aeca4e0fcb3bf886a4c1911f1  0004-lavu-intmath.h-Fix-UB-in-ff_ctz_c-and-ff_ctzll_c.patch
> From f9a12089bc98dde0ccc2487d1442ec6ddb7705f6 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <git@haerdin.se>
> Date: Thu, 16 May 2024 18:10:58 +0200
> Subject: [PATCH 4/5] lavu/intmath.h: Fix UB in ff_ctz_c() and ff_ctzll_c()
> 
> Found by value analysis
> ---
>  libavutil/intmath.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

LGTM

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you drop bombs on a foreign country and kill a hundred thousand
innocent people, expect your government to call the consequence
"unprovoked inhuman terrorist attacks" and use it to justify dropping
more bombs and killing more people. The technology changed, the idea is old.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 5/5] lavu/mathematics: Return early if either a or b is zero

[Tomas Härdin]

[-- Attachment #1: Type: text/plain, Size: 119 bytes --]

This doesn't really fix anything, it just makes the value analysis
easier. I don't feel strongly about it.

/Tomas

[-- Attachment #2: 0005-lavu-mathematics-Return-early-if-either-a-or-b-is-ze.patch --]
[-- Type: text/x-patch, Size: 1252 bytes --]

From cf9c56d7d4d7325d51ba6d99259431be7fca1f67 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <git@haerdin.se>
Date: Mon, 20 May 2024 14:46:01 +0200
Subject: [PATCH 5/5] lavu/mathematics: Return early if either a or b is zero

This removes the need to check b further down
---
 libavutil/mathematics.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/libavutil/mathematics.c b/libavutil/mathematics.c
index 61aeb7c029..06f6e61e78 100644
--- a/libavutil/mathematics.c
+++ b/libavutil/mathematics.c
@@ -71,6 +71,9 @@ int64_t av_rescale_rnd(int64_t a, int64_t b, int64_t c, enum AVRounding rnd)
         rnd -= AV_ROUND_PASS_MINMAX;
     }
 
+    if (a == 0 || b == 0)
+        return 0;
+
     if (a < 0)
         return -(uint64_t)av_rescale_rnd(-FFMAX(a, -INT64_MAX), b, c, rnd ^ ((rnd >> 1) & 1));
 
@@ -85,7 +88,7 @@ int64_t av_rescale_rnd(int64_t a, int64_t b, int64_t c, enum AVRounding rnd)
         else {
             int64_t ad = a / c;
             int64_t a2 = (a % c * b + r) / c;
-            if (ad >= INT32_MAX && b && ad > (INT64_MAX - a2) / b)
+            if (ad >= INT32_MAX && ad > (INT64_MAX - a2) / b)
                 return INT64_MIN;
             return ad * b + a2;
         }
-- 
2.39.2


[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 5/5] lavu/mathematics: Return early if either a or b is zero

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 683 bytes --]

On Thu, May 30, 2024 at 12:15:27AM +0200, Tomas Härdin wrote:
> This doesn't really fix anything, it just makes the value analysis
> easier. I don't feel strongly about it.

if it doesnt fix anything and it doesnt make the code faster
for our usecases, then i would say that it shouldnt be in a
production build

but not feeling strongly about this either

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Breaking DRM is a little like attempting to break through a door even
though the window is wide open and the only thing in the house is a bunch
of things you dont want and which you would get tomorrow for free anyway

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 5/5] lavu/mathematics: Return early if either a or b is zero

[Tomas Härdin]

fre 2024-05-31 klockan 02:22 +0200 skrev Michael Niedermayer:
> On Thu, May 30, 2024 at 12:15:27AM +0200, Tomas Härdin wrote:
> > This doesn't really fix anything, it just makes the value analysis
> > easier. I don't feel strongly about it.
> 
> if it doesnt fix anything and it doesnt make the code faster
> for our usecases, then i would say that it shouldnt be in a
> production build
> 
> but not feeling strongly about this either

I'll hold off on this one for the time being since again it's mostly
there to make analysis easier. I have an even more *interesting* change
that defaults to the long division version in more cases which makes
things slower for the sake of correctness

I'll push the other four patches in a day or two since the only real
feedback was cosmetic, and we can discuss cosmetics until the cows come
home

/Tomas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/5] lavu/common.h: Fix UB in av_clipl_int32_c()

[Andreas Rheinhardt]

Tomas Härdin:
>   */
>  static av_always_inline av_const int32_t av_clipl_int32_c(int64_t a)
>  {
> -    if ((a+0x80000000u) & ~UINT64_C(0xFFFFFFFF)) return (int32_t)((a>>63) ^ 0x7FFFFFFF);
> -    else                                         return (int32_t)a;
> +    if ((a+UINT64_C(0x80000000)) & ~UINT64_C(0xFFFFFFFF)) return (int32_t)((a>>63) ^ 0x7FFFFFFF);
> +    else                                                  return (int32_t)a;

IMO (uint64_t)a + 0x80000000 is more readable. (Maybe it would even be
good to use >> 32 instead of ~UINT64_C(0xFFFFFFFF)?)

- 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-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/5] lavu/common.h: Fix UB in av_clipl_int32_c()

[Tomas Härdin]

tor 2024-05-30 klockan 00:31 +0200 skrev Andreas Rheinhardt:
> Tomas Härdin:
> >   */
> >  static av_always_inline av_const int32_t av_clipl_int32_c(int64_t
> > a)
> >  {
> > -    if ((a+0x80000000u) & ~UINT64_C(0xFFFFFFFF)) return
> > (int32_t)((a>>63) ^ 0x7FFFFFFF);
> > -    else                                         return
> > (int32_t)a;
> > +    if ((a+UINT64_C(0x80000000)) & ~UINT64_C(0xFFFFFFFF)) return
> > (int32_t)((a>>63) ^ 0x7FFFFFFF);
> > +    else                                                  return
> > (int32_t)a;
> 
> IMO (uint64_t)a + 0x80000000 is more readable. (Maybe it would even
> be
> good to use >> 32 instead of ~UINT64_C(0xFFFFFFFF)?)

It already uses UINT64_C, hence why I used it.

>> 32 would work also. Does it make any difference performance wise?

/Tomas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/5] lavu/common.h: Fix UB in av_clipl_int32_c()

[Rémi Denis-Courmont]

Hi,

Le 30 mai 2024 01:13:14 GMT+03:00, "Tomas Härdin" <git@haerdin.se> a écrit :
>The entire patchset passes FATE

Is the version in riscv/intmath.h safe? It looks to me that the GCC codegen for not only RV64 but also AArch{32,64} and x86-64 is better than this.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/5] lavu/common.h: Fix UB in av_clipl_int32_c()

[Tomas Härdin]

tor 2024-05-30 klockan 09:41 +0300 skrev Rémi Denis-Courmont:
> Hi,
> 
> Le 30 mai 2024 01:13:14 GMT+03:00, "Tomas Härdin" <git@haerdin.se> a
> écrit :
> > The entire patchset passes FATE
> 
> Is the version in riscv/intmath.h safe? It looks to me that the GCC
> codegen for not only RV64 but also AArch{32,64} and x86-64 is better
> than this.

I haven't checked. It seems weird to me to have two different C
versions. We shouldn't rely on type punning. The standard compliant way
is to use memcpy()

/Tomas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/5] lavu/common.h: Fix UB in av_clipl_int32_c()

[Rémi Denis-Courmont]

Le 30 mai 2024 12:40:20 GMT+03:00, "Tomas Härdin" <git@haerdin.se> a écrit :
>tor 2024-05-30 klockan 09:41 +0300 skrev Rémi Denis-Courmont:
>> Hi,
>> 
>> Le 30 mai 2024 01:13:14 GMT+03:00, "Tomas Härdin" <git@haerdin.se> a
>> écrit :
>> > The entire patchset passes FATE
>> 
>> Is the version in riscv/intmath.h safe? It looks to me that the GCC
>> codegen for not only RV64 but also AArch{32,64} and x86-64 is better
>> than this.
>
>I haven't checked. It seems weird to me to have two different C
>versions.

The common one ends up horrendously bad on RV, and presumably on MIPS and some other RISC ISA.

> We shouldn't rely on type punning.

Because?

We should depend on punning as long as it conforms to the standard.

> The standard compliant way
>is to use memcpy()

That's way worse than union in terms of how proactively the compiler needs to optimise, and both approaches are as confirming.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/5] lavu/common.h: Fix UB in av_clipl_int32_c()

[Tomas Härdin]

tor 2024-05-30 klockan 14:50 +0300 skrev Rémi Denis-Courmont:
> 
> 
> Le 30 mai 2024 12:40:20 GMT+03:00, "Tomas Härdin" <git@haerdin.se> a
> écrit :
> > tor 2024-05-30 klockan 09:41 +0300 skrev Rémi Denis-Courmont:
> > > Hi,
> > > 
> > > Le 30 mai 2024 01:13:14 GMT+03:00, "Tomas Härdin"
> > > <git@haerdin.se> a
> > > écrit :
> > > > The entire patchset passes FATE
> > > 
> > > Is the version in riscv/intmath.h safe? It looks to me that the
> > > GCC
> > > codegen for not only RV64 but also AArch{32,64} and x86-64 is
> > > better
> > > than this.
> > 
> > I haven't checked. It seems weird to me to have two different C
> > versions.
> 
> The common one ends up horrendously bad on RV, and presumably on MIPS
> and some other RISC ISA.
> 
> > We shouldn't rely on type punning.
> 
> Because?
> 
> We should depend on punning as long as it conforms to the standard.

My mistake, I forgot type punning is allowed in C. It's UB in C++

> > The standard compliant way
> > is to use memcpy()
> 
> That's way worse than union in terms of how proactively the compiler
> needs to optimise, and both approaches are as confirming.

A good compiler will do the same thing

Maybe I can get the riscv version covered by Eva as well. That's beyond
the scope of this patchset

/Tomas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/5] lavu/common.h: Fix UB in av_clipl_int32_c()

[Rémi Denis-Courmont]

Le 30 mai 2024 17:07:21 GMT+03:00, "Tomas Härdin" <git@haerdin.se> a écrit :
>> We should depend on punning as long as it conforms to the standard.
>
>My mistake, I forgot type punning is allowed in C. It's UB in C++
>
>> > The standard compliant way
>> > is to use memcpy()
>> 
>> That's way worse than union in terms of how proactively the compiler
>> needs to optimise, and both approaches are as confirming.
>
>A good compiler will do the same thing

True, and I don't care very much about memcpy vs union, as they both rely on matching representation. AFAIR, FFmpeg tends to use unions though.

>
>Maybe I can get the riscv version covered by Eva as well. That's beyond
>the scope of this patchset

IMHO, this specific patch (and the following one) are beating dead horses. Sure there may be theoretical UB in the current code, but if there is a *better* implementation, better switch to that than bike shedding the fix for the UB.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/5] lavu/common.h: Fix UB in av_clipl_int32_c()

[Tomas Härdin]

tor 2024-05-30 klockan 17:28 +0300 skrev Rémi Denis-Courmont:
> 
> 
> Le 30 mai 2024 17:07:21 GMT+03:00, "Tomas Härdin" <git@haerdin.se> a
> écrit :
> > > We should depend on punning as long as it conforms to the
> > > standard.
> > 
> > My mistake, I forgot type punning is allowed in C. It's UB in C++
> > 
> > > > The standard compliant way
> > > > is to use memcpy()
> > > 
> > > That's way worse than union in terms of how proactively the
> > > compiler
> > > needs to optimise, and both approaches are as confirming.
> > 
> > A good compiler will do the same thing
> 
> True, and I don't care very much about memcpy vs union, as they both
> rely on matching representation. AFAIR, FFmpeg tends to use unions
> though.
> 
> > 
> > Maybe I can get the riscv version covered by Eva as well. That's
> > beyond
> > the scope of this patchset
> 
> IMHO, this specific patch (and the following one) are beating dead
> horses. Sure there may be theoretical UB in the current code, but if
> there is a *better* implementation, better switch to that than bike
> shedding the fix for the UB.

Are you saying that UB is acceptable? You know the compiler is free to
assume signed arithmetic doesn't overflow, right? If so then what other
UB might we accept?

/Tomas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/5] lavu/common.h: Fix UB in av_clipl_int32_c()

[Rémi Denis-Courmont]

Le torstaina 30. toukokuuta 2024, 18.32.19 EEST Tomas Härdin a écrit :
> Are you saying that UB is acceptable?

Are you imitating Thilo and grand-standing by putting words in my mouth?

Yes and so -1 for you.

-- 
レミ・デニ-クールモン
http://www.remlab.net/



_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/5] lavu/common.h: Fix UB in av_clipl_int32_c()

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 853 bytes --]

On Thu, May 30, 2024 at 06:38:42PM +0300, Rémi Denis-Courmont wrote:
> Le torstaina 30. toukokuuta 2024, 18.32.19 EEST Tomas Härdin a écrit :
> > Are you saying that UB is acceptable?
> 

no, thats not what Remi meant IIUC


> Are you imitating Thilo and grand-standing by putting words in my mouth?

That seems unneccesarily insulting towards Thilo

also i think you quickly feel attacked and in some cases the other person
may have had no intend to attack you. And things can be just plain
misunderstandings

I certainly have missunderstood people many times and i certainly was
missunderstood by people also many times.

thx


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

While the State exists there can be no freedom; when there is freedom there
will be no State. -- Vladimir Lenin

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/5] lavu/common.h: Fix UB in av_clipl_int32_c()

[Rémi Denis-Courmont]

Le 31 mai 2024 04:03:24 GMT+03:00, Michael Niedermayer <michael@niedermayer.cc> a écrit :
>> Are you imitating Thilo and grand-standing by putting words in my mouth?
>
>That seems unneccesarily insulting towards Thilo

This is not an insult in the first place, and besides Thilo did put words in mouth to discredit me as shown by the mailing mist archives.

And you had no problems with that. You also had no problems with Thilo calling Kieran, Derek and myself trolls on this very mailing list, and that was an actual insult. So care to explain why you have a problem with this and not that?

Because it sure looks like blatant double standards to me, and against me.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/5] lavu/common.h: Fix UB in av_clipl_int32_c()

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1933 bytes --]

On Mon, Jun 03, 2024 at 10:32:17AM +0300, Rémi Denis-Courmont wrote:
> 
> 
> Le 31 mai 2024 04:03:24 GMT+03:00, Michael Niedermayer <michael@niedermayer.cc> a écrit :
> >> Are you imitating Thilo and grand-standing by putting words in my mouth?
> >
> >That seems unneccesarily insulting towards Thilo
> 
> This is not an insult in the first place, and besides Thilo did put words in mouth to discredit me as shown by the mailing mist archives.
> 
> And you had no problems with that. You also had no problems with Thilo calling Kieran, Derek and myself trolls on this very mailing list, and that was an actual insult. So care to explain why you have a problem with this and not that?

about "you had no problems with that"
First you dont know and cannot know what i have a problem with and what not.
(so please dont claim such a thing)

Second, i do not remember thilo calling you a troll. What i remember (and thats from
my memory) thilo preceived one of kierans replies as trolling and then did not
reply to that. There are 2 things here.
Preceiving a reply as trolling is not the same as calling someone a troll.
And the 2nd thing, i was not happy that the NAB 2024 invitation was not
resend or pinged to ensure people knew they could go there and help if
they wanted.
So in fact i was not ok with that.

and third, iam a human being, and i will certainly miss some things, in fact
iam not even trying to catch every statement thats abbrasive and point
it out. But when i do conciously notice something that seems that it could
hurt someone or be against the team spirit then i will try to point it out.
My goal is not to "warn" anyone its really meant to just bring it to the speakers
attention as (s)he may even be unware

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

There will always be a question for which you do not know the correct answer.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/5] lavu/common.h: Fix UB in av_clipl_int32_c()

[James Almer]

On 5/30/2024 12:32 PM, Tomas Härdin wrote:
> tor 2024-05-30 klockan 17:28 +0300 skrev Rémi Denis-Courmont:
>>
>>
>> Le 30 mai 2024 17:07:21 GMT+03:00, "Tomas Härdin" <git@haerdin.se> a
>> écrit :
>>>> We should depend on punning as long as it conforms to the
>>>> standard.
>>>
>>> My mistake, I forgot type punning is allowed in C. It's UB in C++
>>>
>>>>> The standard compliant way
>>>>> is to use memcpy()
>>>>
>>>> That's way worse than union in terms of how proactively the
>>>> compiler
>>>> needs to optimise, and both approaches are as confirming.
>>>
>>> A good compiler will do the same thing
>>
>> True, and I don't care very much about memcpy vs union, as they both
>> rely on matching representation. AFAIR, FFmpeg tends to use unions
>> though.
>>
>>>
>>> Maybe I can get the riscv version covered by Eva as well. That's
>>> beyond
>>> the scope of this patchset
>>
>> IMHO, this specific patch (and the following one) are beating dead
>> horses. Sure there may be theoretical UB in the current code, but if
>> there is a *better* implementation, better switch to that than bike
>> shedding the fix for the UB.
> 
> Are you saying that UB is acceptable? You know the compiler is free to
> assume signed arithmetic doesn't overflow, right? If so then what other
> UB might we accept?

He did not say that... He said we should switch to a better 
implementation rather than trying to fix the existing potentially buggy one.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/5] lavu/common.h: Fix UB in av_clipl_int32_c()

[Tomas Härdin]

tor 2024-05-30 klockan 12:42 -0300 skrev James Almer:
> On 5/30/2024 12:32 PM, Tomas Härdin wrote:
> > tor 2024-05-30 klockan 17:28 +0300 skrev Rémi Denis-Courmont:
> > > 
> > > 
> > > Le 30 mai 2024 17:07:21 GMT+03:00, "Tomas Härdin"
> > > <git@haerdin.se> a
> > > écrit :
> > > > > We should depend on punning as long as it conforms to the
> > > > > standard.
> > > > 
> > > > My mistake, I forgot type punning is allowed in C. It's UB in
> > > > C++
> > > > 
> > > > > > The standard compliant way
> > > > > > is to use memcpy()
> > > > > 
> > > > > That's way worse than union in terms of how proactively the
> > > > > compiler
> > > > > needs to optimise, and both approaches are as confirming.
> > > > 
> > > > A good compiler will do the same thing
> > > 
> > > True, and I don't care very much about memcpy vs union, as they
> > > both
> > > rely on matching representation. AFAIR, FFmpeg tends to use
> > > unions
> > > though.
> > > 
> > > > 
> > > > Maybe I can get the riscv version covered by Eva as well.
> > > > That's
> > > > beyond
> > > > the scope of this patchset
> > > 
> > > IMHO, this specific patch (and the following one) are beating
> > > dead
> > > horses. Sure there may be theoretical UB in the current code, but
> > > if
> > > there is a *better* implementation, better switch to that than
> > > bike
> > > shedding the fix for the UB.
> > 
> > Are you saying that UB is acceptable? You know the compiler is free
> > to
> > assume signed arithmetic doesn't overflow, right? If so then what
> > other
> > UB might we accept?
> 
> He did not say that... He said we should switch to a better 
> implementation rather than trying to fix the existing potentially
> buggy one.

I have a fix for demonstrable UB and Rémi is problematizing it. It is
not a "theoretical" UB - that's not how UB works. Any compiler doing
basic value analysis will find it, and is therefore free to do whatever
it wants, for example deleting all calls to av_clipl_int32_c().

We could certainly replace some of these functions with intrinsics, but
that's not what this patchset is about. I don't know what set of
compilers we support. I don't know what intrinsics they support. Am I
to be compelled to figure that out, and provide the necessary
intrinsics for all of them?

This may all seem trivial, and it is, but this patchset is also a test
balloon. Line struggle is important. What I see is the stalling of
fixes of *known broken code*. That is not encouraging.

/Tomas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/5] lavu/common.h: Fix UB in av_clipl_int32_c()

[Rémi Denis-Courmont]

Le torstaina 30. toukokuuta 2024, 19.48.13 EEST Tomas Härdin a écrit :
> > > Are you saying that UB is acceptable? You know the compiler is free
> > > to
> > > assume signed arithmetic doesn't overflow, right? If so then what
> > > other
> > > UB might we accept?
> > 
> > He did not say that... He said we should switch to a better
> > implementation rather than trying to fix the existing potentially
> > buggy one.
> 
> I have a fix for demonstrable UB and Rémi is problematizing it.

Andreas made cosmetic arguments against this patch before I had even seen the 
patch, forget comment on it.

> It is not a "theoretical" UB - that's not how UB works.

It is a *theoretical* UB if you can not prove that it leads to misbehaviour in 
any *practical* use. In theory, all UB is *potentially* fatal. Emphasis on 
potentially.

So yes, while all UB instances are bad and deserve fixing, they are not all 
equally bad nor urgent. UB that is proven to lead to remote code execution is 
way worse than theoretical UB that has only been proven in literature, and is 
not known or even seriously suspected to lead to broken optimisations.

> Any compiler doing
> basic value analysis will find it, and is therefore free to do whatever
> it wants, for example deleting all calls to av_clipl_int32_c().

That is formally true. But it is also formally true that, by that same logic, 
since there is most certainly some UB instance left elsewhere in the codebase, 
the entirety of libavutil could be elided by the compiler. In other words, in 
theory, FFmpeg does not work at all. Does that mean that we should give up on 
the project here and now?

> We could certainly replace some of these functions with intrinsics, but
> that's not what this patchset is about.

I am not sure what is your point because nobody said that av_clipl_int32_c() 
should be replaced by intrinsics.

> I don't know what set of compilers we support.

That is irrelevant since all C99, C11 and C23 compilers support the proposed 
substitute code as long as <stdint.h> defines int32_t.

> I don't know what intrinsics they support.

Also irrelevant.

> Am I to be compelled to figure that out, and provide the necessary
> intrinsics for all of them?

No, and you are the only person to have made an implication to the contrary as 
far as *this* patch is concerned.

-- 
雷米‧德尼-库尔蒙
http://www.remlab.net/



_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/5] lavu/common.h: Fix UB in av_clipl_int32_c()

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1773 bytes --]

On Thu, May 30, 2024 at 08:49:12PM +0300, Rémi Denis-Courmont wrote:
> Le torstaina 30. toukokuuta 2024, 19.48.13 EEST Tomas Härdin a écrit :
> > > > Are you saying that UB is acceptable? You know the compiler is free
> > > > to
> > > > assume signed arithmetic doesn't overflow, right? If so then what
> > > > other
> > > > UB might we accept?
> > > 
> > > He did not say that... He said we should switch to a better
> > > implementation rather than trying to fix the existing potentially
> > > buggy one.
> > 
> > I have a fix for demonstrable UB and Rémi is problematizing it.
> 
> Andreas made cosmetic arguments against this patch before I had even seen the 
> patch, forget comment on it.
> 

> > It is not a "theoretical" UB - that's not how UB works.
> 
> It is a *theoretical* UB if you can not prove that it leads to misbehaviour in 

If the function doesnt get called with values triggering UB then its not UB.

If the function gets called with values triggering the signed overflow then its UB
And its a bug unless the applications intended behavior is undefined.

also i would not bet on that the function produces the correct output for
input values that trigger UB on every platform

The case where this really could be a problem is if its used with compile
time constants that would trigger the overflow because in these cases
the optimizer can assume the whole codepath leading to it can be
removed.

IMHO we should simply fix UB instead of arguing over how bad it could be
or when.

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Those who would give up essential Liberty, to purchase a little
temporary Safety, deserve neither Liberty nor Safety -- Benjamin Franklin

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/5] lavu/common.h: Fix UB in av_clipl_int32_c()

[Rémi Denis-Courmont]

Le torstaina 30. toukokuuta 2024, 22.07.13 EEST Michael Niedermayer a écrit :
> If the function doesnt get called with values triggering UB then its not UB.

As Tomas pointed out, that statement is actually false. Specifically, if the 
compiler can prove that the function can be called with values triggering UB, 
then the code is UB, even if those offending values do not actually occur in a 
given instance of the program.

The C specification is known to contradict causality.

For instance, if you have pass an uninitialised value to av_clipl_int32_c(), 
then the code is UB, even if the actual value in the register or stack slot is 
never one that could trigger UB. Of course, usage of uninitialised values is a 
bad practice, but it is not, per se, UB.

-- 
レミ・デニ-クールモン
http://www.remlab.net/



_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/5] lavu/common.h: Fix UB in av_clipl_int32_c()

[Tomas Härdin]

tor 2024-05-30 klockan 20:49 +0300 skrev Rémi Denis-Courmont:
> Le torstaina 30. toukokuuta 2024, 19.48.13 EEST Tomas Härdin a écrit 
> > It is not a "theoretical" UB - that's not how UB works.
> 
> It is a *theoretical* UB if you can not prove that it leads to
> misbehaviour in 
> any *practical* use. In theory, all UB is *potentially* fatal.
> Emphasis on 
> potentially.

The issue is that compilers can change without notice

> > Any compiler doing
> > basic value analysis will find it, and is therefore free to do
> > whatever
> > it wants, for example deleting all calls to av_clipl_int32_c().
> 
> That is formally true. But it is also formally true that, by that
> same logic, 
> since there is most certainly some UB instance left elsewhere in the
> codebase, 
> the entirety of libavutil could be elided by the compiler.

I mean, part of what I'm doing with my little value analysis experiment
is finding these instances of UB throughout lavu and fixing them, so
that no compiler will perform what we might consider dubious or
unexpected optimizations. It is far more powerful than fuzzing when it
comes to discovering bugs

I'm looking at rational.c at the moment, and there are definitely some
dubious codepaths. There have also been fixes for signed overflow in
rational.c already. I wouldn't be surprised if there are more corner
cases that can be triggered by some demuxer or decoder that fuzzing
hasn't discovered yet

Maybe in some glorious future we'll have a version of C where signed
overflow is defined behavior

/Tomas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/5] lavu/common.h: Fix UB in av_clipl_int32_c()

[Tomas Härdin]

Pushed patches 1-4

/Tomas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".