public inbox for newlib@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] newlib: fix build with <gcc-5 versions
@ 2022-03-14  3:25 Mike Frysinger
  2022-03-14  7:36 ` Sebastian Huber
                   ` (2 more replies)
  0 siblings, 3 replies; 24+ messages in thread
From: Mike Frysinger @ 2022-03-14  3:25 UTC (permalink / raw)
  To: newlib

__builtin_mul_overflow showed up with gcc-5, so stub it out for older
versions.
---
 newlib/libc/include/sys/cdefs.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/newlib/libc/include/sys/cdefs.h b/newlib/libc/include/sys/cdefs.h
index da729092185c..9ab0fc1e16f4 100644
--- a/newlib/libc/include/sys/cdefs.h
+++ b/newlib/libc/include/sys/cdefs.h
@@ -416,6 +416,10 @@
 #define	__unreachable()	((void)0)
 #endif
 
+#if !__GNUC_PREREQ__(5, 0)
+#define __builtin_mul_overflow(a, b, size) 0
+#endif
+
 /* XXX: should use `#if __STDC_VERSION__ < 199901'. */
 #if !__GNUC_PREREQ__(2, 7) && !defined(__INTEL_COMPILER)
 #define	__func__	NULL
-- 
2.34.1


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] newlib: fix build with <gcc-5 versions
  2022-03-14  3:25 [PATCH] newlib: fix build with <gcc-5 versions Mike Frysinger
@ 2022-03-14  7:36 ` Sebastian Huber
  2022-03-14 16:58   ` Mike Frysinger
  2022-03-15  3:25 ` [PATCH v2] " Mike Frysinger
  2022-03-15 23:53 ` [PATCH v3] " Mike Frysinger
  2 siblings, 1 reply; 24+ messages in thread
From: Sebastian Huber @ 2022-03-14  7:36 UTC (permalink / raw)
  To: Mike Frysinger, newlib

On 14/03/2022 04:25, Mike Frysinger wrote:
> __builtin_mul_overflow showed up with gcc-5, so stub it out for older
> versions.

The __builtin_mul_overflow() performs a multiplication and an overflow 
check. Just replacing this with 0 is not the same.

-- 
embedded brains GmbH
Herr Sebastian HUBER
Dornierstr. 4
82178 Puchheim
Germany
email: sebastian.huber@embedded-brains.de
phone: +49-89-18 94 741 - 16
fax:   +49-89-18 94 741 - 08

Registergericht: Amtsgericht München
Registernummer: HRB 157899
Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
Unsere Datenschutzerklärung finden Sie hier:
https://embedded-brains.de/datenschutzerklaerung/

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] newlib: fix build with <gcc-5 versions
  2022-03-14  7:36 ` Sebastian Huber
@ 2022-03-14 16:58   ` Mike Frysinger
  0 siblings, 0 replies; 24+ messages in thread
From: Mike Frysinger @ 2022-03-14 16:58 UTC (permalink / raw)
  To: Sebastian Huber; +Cc: newlib

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

On 14 Mar 2022 08:36, Sebastian Huber wrote:
> On 14/03/2022 04:25, Mike Frysinger wrote:
> > __builtin_mul_overflow showed up with gcc-5, so stub it out for older
> > versions.
> 
> The __builtin_mul_overflow() performs a multiplication and an overflow 
> check. Just replacing this with 0 is not the same.

i'm aware it does the check.  i figured since <gcc-5 isn't that common, it
isn't worth the effort doing the ad-hoc ({...}) function definition trying
to maintain the same semantics.  although it should at least do the actual
multiplication & assignment.
-mike

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v2] newlib: fix build with <gcc-5 versions
  2022-03-14  3:25 [PATCH] newlib: fix build with <gcc-5 versions Mike Frysinger
  2022-03-14  7:36 ` Sebastian Huber
@ 2022-03-15  3:25 ` Mike Frysinger
  2022-03-15 12:41   ` Richard Earnshaw
  2022-03-15 23:53 ` [PATCH v3] " Mike Frysinger
  2 siblings, 1 reply; 24+ messages in thread
From: Mike Frysinger @ 2022-03-15  3:25 UTC (permalink / raw)
  To: newlib

__builtin_mul_overflow showed up with gcc-5, so stub it out for older
versions.
---
 newlib/libc/include/sys/cdefs.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/newlib/libc/include/sys/cdefs.h b/newlib/libc/include/sys/cdefs.h
index da729092185c..e51f7f4b873a 100644
--- a/newlib/libc/include/sys/cdefs.h
+++ b/newlib/libc/include/sys/cdefs.h
@@ -416,6 +416,10 @@
 #define	__unreachable()	((void)0)
 #endif
 
+#if !__GNUC_PREREQ__(5, 0)
+#define __builtin_mul_overflow(a, b, size) ({ *(size) = (a) * (b); 0; })
+#endif
+
 /* XXX: should use `#if __STDC_VERSION__ < 199901'. */
 #if !__GNUC_PREREQ__(2, 7) && !defined(__INTEL_COMPILER)
 #define	__func__	NULL
-- 
2.34.1


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] newlib: fix build with <gcc-5 versions
  2022-03-15  3:25 ` [PATCH v2] " Mike Frysinger
@ 2022-03-15 12:41   ` Richard Earnshaw
  2022-03-15 23:54     ` Mike Frysinger
  0 siblings, 1 reply; 24+ messages in thread
From: Richard Earnshaw @ 2022-03-15 12:41 UTC (permalink / raw)
  To: Mike Frysinger, newlib



On 15/03/2022 03:25, Mike Frysinger wrote:
> __builtin_mul_overflow showed up with gcc-5, so stub it out for older
> versions.
> ---
>   newlib/libc/include/sys/cdefs.h | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/newlib/libc/include/sys/cdefs.h b/newlib/libc/include/sys/cdefs.h
> index da729092185c..e51f7f4b873a 100644
> --- a/newlib/libc/include/sys/cdefs.h
> +++ b/newlib/libc/include/sys/cdefs.h
> @@ -416,6 +416,10 @@
>   #define	__unreachable()	((void)0)
>   #endif
>   
> +#if !__GNUC_PREREQ__(5, 0)
> +#define __builtin_mul_overflow(a, b, size) ({ *(size) = (a) * (b); 0; })

Wouldn't
(*(size) = (a) * (b), 0)

be more portable (avoiding the GNU statement expression extension)?

R.

> +#endif
> +
>   /* XXX: should use `#if __STDC_VERSION__ < 199901'. */
>   #if !__GNUC_PREREQ__(2, 7) && !defined(__INTEL_COMPILER)
>   #define	__func__	NULL

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v3] newlib: fix build with <gcc-5 versions
  2022-03-14  3:25 [PATCH] newlib: fix build with <gcc-5 versions Mike Frysinger
  2022-03-14  7:36 ` Sebastian Huber
  2022-03-15  3:25 ` [PATCH v2] " Mike Frysinger
@ 2022-03-15 23:53 ` Mike Frysinger
  2 siblings, 0 replies; 24+ messages in thread
From: Mike Frysinger @ 2022-03-15 23:53 UTC (permalink / raw)
  To: newlib

__builtin_mul_overflow showed up with gcc-5, so stub it out for older
versions.
---
v3
- use style from Richard Earnshaw to avoid gcc-specific syntax

 newlib/libc/include/sys/cdefs.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/newlib/libc/include/sys/cdefs.h b/newlib/libc/include/sys/cdefs.h
index da729092185c..79bce1d1d78d 100644
--- a/newlib/libc/include/sys/cdefs.h
+++ b/newlib/libc/include/sys/cdefs.h
@@ -416,6 +416,10 @@
 #define	__unreachable()	((void)0)
 #endif
 
+#if !__GNUC_PREREQ__(5, 0)
+#define __builtin_mul_overflow(a, b, size) (*(size) = (a) * (b), 0)
+#endif
+
 /* XXX: should use `#if __STDC_VERSION__ < 199901'. */
 #if !__GNUC_PREREQ__(2, 7) && !defined(__INTEL_COMPILER)
 #define	__func__	NULL
-- 
2.34.1


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] newlib: fix build with <gcc-5 versions
  2022-03-15 12:41   ` Richard Earnshaw
@ 2022-03-15 23:54     ` Mike Frysinger
  2022-03-16  7:12       ` R. Diez
  2022-03-20 12:52       ` Eric Bresie
  0 siblings, 2 replies; 24+ messages in thread
From: Mike Frysinger @ 2022-03-15 23:54 UTC (permalink / raw)
  To: Richard Earnshaw; +Cc: newlib

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

On 15 Mar 2022 12:41, Richard Earnshaw wrote:
> On 15/03/2022 03:25, Mike Frysinger wrote:
> > __builtin_mul_overflow showed up with gcc-5, so stub it out for older
> > versions.
> > ---
> >   newlib/libc/include/sys/cdefs.h | 4 ++++
> >   1 file changed, 4 insertions(+)
> > 
> > diff --git a/newlib/libc/include/sys/cdefs.h b/newlib/libc/include/sys/cdefs.h
> > index da729092185c..e51f7f4b873a 100644
> > --- a/newlib/libc/include/sys/cdefs.h
> > +++ b/newlib/libc/include/sys/cdefs.h
> > @@ -416,6 +416,10 @@
> >   #define	__unreachable()	((void)0)
> >   #endif
> >   
> > +#if !__GNUC_PREREQ__(5, 0)
> > +#define __builtin_mul_overflow(a, b, size) ({ *(size) = (a) * (b); 0; })
> 
> Wouldn't
> (*(size) = (a) * (b), 0)
> 
> be more portable (avoiding the GNU statement expression extension)?

sure, that works too, thanks
-mike

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] newlib: fix build with <gcc-5 versions
  2022-03-15 23:54     ` Mike Frysinger
@ 2022-03-16  7:12       ` R. Diez
  2022-03-16  8:30         ` Mike Frysinger
  2022-03-20 12:52       ` Eric Bresie
  1 sibling, 1 reply; 24+ messages in thread
From: R. Diez @ 2022-03-16  7:12 UTC (permalink / raw)
  To: Mike Frysinger; +Cc: Richard Earnshaw, newlib

> __builtin_mul_overflow showed up with gcc-5, so stub it out for older
> versions.
> [...]
> +#if !__GNUC_PREREQ__(5, 0)
> +#define __builtin_mul_overflow(a, b, size) ({ *(size) = (a) * (b); 0; })

I do not understand why Newlib needs to "stub it out" like this.

According to the GCC documentation, this kind of built-in routines allow the caller to check whether the operations overflowes. But the code above performs no overflow checking at all.

Therefore, compiling your code with GCC < 5 will silently break your application. After all, the only reason to use __builtin_mul_overflow() is that you need to check for overflow, is it?

Regards,
   rdiez

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] newlib: fix build with <gcc-5 versions
  2022-03-16  7:12       ` R. Diez
@ 2022-03-16  8:30         ` Mike Frysinger
  2022-03-16  9:17           ` R. Diez
  0 siblings, 1 reply; 24+ messages in thread
From: Mike Frysinger @ 2022-03-16  8:30 UTC (permalink / raw)
  To: R. Diez; +Cc: Richard Earnshaw, newlib

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

On 16 Mar 2022 08:12, R. Diez wrote:
> > __builtin_mul_overflow showed up with gcc-5, so stub it out for older
> > versions.
> > [...]
> > +#if !__GNUC_PREREQ__(5, 0)
> > +#define __builtin_mul_overflow(a, b, size) ({ *(size) = (a) * (b); 0; })
> 
> I do not understand why Newlib needs to "stub it out" like this.

because the builtin doesn't exist, and attempting to use it leads to
undefined functions, and the resulting libc.a can't link anything.

> According to the GCC documentation, this kind of built-in routines allow the caller to check whether the operations overflowes. But the code above performs no overflow checking at all.
> 
> Therefore, compiling your code with GCC < 5 will silently break your application. After all, the only reason to use __builtin_mul_overflow() is that you need to check for overflow, is it?

practically speaking, i don't think this is a big deal.  newlib gained these
checks only "recently" (<2 years ago).  newlib has been around for much much
longer, and the world didn't notice.  yes, if an app starts trying to allocate
huge amounts of memory such that it triggers 32-bit overflows when calculating,
the new size, it will probably internally allocate fewer bytes than requested,
and things will get corrupted.  but like, don't do that :p.  such applications
probably will have other problems already.
-mike

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] newlib: fix build with <gcc-5 versions
  2022-03-16  8:30         ` Mike Frysinger
@ 2022-03-16  9:17           ` R. Diez
  2022-03-17  2:41             ` Mike Frysinger
  0 siblings, 1 reply; 24+ messages in thread
From: R. Diez @ 2022-03-16  9:17 UTC (permalink / raw)
  To: Mike Frysinger; +Cc: newlib, Richard Earnshaw


>> Therefore, compiling your code with GCC < 5 will silently break your application.
>> After all, the only reason to use __builtin_mul_overflow() is
>> that you need to check for overflow, is it?
> 
> practically speaking, i don't think this is a big deal.  newlib gained these
> checks only "recently" (<2 years ago).  newlib has been around for much much
> longer, and the world didn't notice.

Such general justifications wouldn't pass quality assurance (if we had one).


> yes, if an app starts trying to allocate
> huge amounts of memory such that it triggers 32-bit overflows when calculating,
> the new size, it will probably internally allocate fewer bytes than requested,
> and things will get corrupted.  but like, don't do that :p.  such applications
> probably will have other problems already.

You are suggesting that this only affects memory allocation, but the patch is for libc/include/sys/cdefs.h , so those mine traps will be available for everybody.

People will tend to assume that anything in Newlib is correct, and code has a way to get copied around and re-used.

There are many ways to mitigate the risk:

- Require GCC 5.
- Provide a proper implementation of __builtin_mul_overflow().
- Patch all users of __builtin_mul_overflow() within Newlib, so that they do not use it if the compiler does not provide it.
- Issue a compilation warning for GCC < 5 that the "stub" __builtin_mul_overflow() is broken.
   Note that this is not actually a "stub" implementation in the common sense.
- Add an "assert( false ) // fix me" inside the implementation.
- Add a comment stating that the "stub" implementation is not actually correct.

Regards,
   rdiez

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] newlib: fix build with <gcc-5 versions
  2022-03-16  9:17           ` R. Diez
@ 2022-03-17  2:41             ` Mike Frysinger
  2022-03-17  9:49               ` Corinna Vinschen
  0 siblings, 1 reply; 24+ messages in thread
From: Mike Frysinger @ 2022-03-17  2:41 UTC (permalink / raw)
  To: R. Diez; +Cc: newlib, Richard Earnshaw

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

On 16 Mar 2022 10:17, R. Diez wrote:
> >> Therefore, compiling your code with GCC < 5 will silently break your application.
> >> After all, the only reason to use __builtin_mul_overflow() is
> >> that you need to check for overflow, is it?
> > 
> > practically speaking, i don't think this is a big deal.  newlib gained these
> > checks only "recently" (<2 years ago).  newlib has been around for much much
> > longer, and the world didn't notice.
> 
> Such general justifications wouldn't pass quality assurance (if we had one).

in your opinion.  software is not perfect, it's trade-offs.

> > yes, if an app starts trying to allocate
> > huge amounts of memory such that it triggers 32-bit overflows when calculating,
> > the new size, it will probably internally allocate fewer bytes than requested,
> > and things will get corrupted.  but like, don't do that :p.  such applications
> > probably will have other problems already.
> 
> You are suggesting that this only affects memory allocation, but the patch is for libc/include/sys/cdefs.h , so those mine traps will be available for everybody.
> 
> People will tend to assume that anything in Newlib is correct, and code has a way to get copied around and re-used.
> 
> There are many ways to mitigate the risk:
> 
> - Require GCC 5.
> - Provide a proper implementation of __builtin_mul_overflow().
> - Patch all users of __builtin_mul_overflow() within Newlib, so that they do not use it if the compiler does not provide it.
> - Issue a compilation warning for GCC < 5 that the "stub" __builtin_mul_overflow() is broken.
>    Note that this is not actually a "stub" implementation in the common sense.
> - Add an "assert( false ) // fix me" inside the implementation.
> - Add a comment stating that the "stub" implementation is not actually correct.

any option that prevents correct execution with gcc-4 is not an improvement.
if you care this much, feel free to contribute a patch.  or use gcc-5+ and
not worry about it.
-mike

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] newlib: fix build with <gcc-5 versions
  2022-03-17  2:41             ` Mike Frysinger
@ 2022-03-17  9:49               ` Corinna Vinschen
  2022-03-17 11:26                 ` Richard Earnshaw
  2022-03-20  1:21                 ` Mike Frysinger
  0 siblings, 2 replies; 24+ messages in thread
From: Corinna Vinschen @ 2022-03-17  9:49 UTC (permalink / raw)
  To: newlib

On Mar 16 22:41, Mike Frysinger wrote:
> On 16 Mar 2022 10:17, R. Diez wrote:
> > >> Therefore, compiling your code with GCC < 5 will silently break your application.
> > >> After all, the only reason to use __builtin_mul_overflow() is
> > >> that you need to check for overflow, is it?
> > > 
> > > practically speaking, i don't think this is a big deal.  newlib gained these
> > > checks only "recently" (<2 years ago).  newlib has been around for much much
> > > longer, and the world didn't notice.
> > 
> > Such general justifications wouldn't pass quality assurance (if we had one).
> 
> in your opinion.  software is not perfect, it's trade-offs.
> 
> > > yes, if an app starts trying to allocate
> > > huge amounts of memory such that it triggers 32-bit overflows when calculating,
> > > the new size, it will probably internally allocate fewer bytes than requested,
> > > and things will get corrupted.  but like, don't do that :p.  such applications
> > > probably will have other problems already.
> > 
> > You are suggesting that this only affects memory allocation, but the patch is for libc/include/sys/cdefs.h , so those mine traps will be available for everybody.
> > 
> > People will tend to assume that anything in Newlib is correct, and code has a way to get copied around and re-used.
> > 
> > There are many ways to mitigate the risk:
> > 
> > - Require GCC 5.
> > - Provide a proper implementation of __builtin_mul_overflow().
> > - Patch all users of __builtin_mul_overflow() within Newlib, so that they do not use it if the compiler does not provide it.
> > - Issue a compilation warning for GCC < 5 that the "stub" __builtin_mul_overflow() is broken.
> >    Note that this is not actually a "stub" implementation in the common sense.
> > - Add an "assert( false ) // fix me" inside the implementation.
> > - Add a comment stating that the "stub" implementation is not actually correct.
> 
> any option that prevents correct execution with gcc-4 is not an improvement.
> if you care this much, feel free to contribute a patch.  or use gcc-5+ and
> not worry about it.
> -mike

Does anybody actually care for building with gcc < 5?  If not, we
should just make gcc 5 a prerequisite.


Corinna


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] newlib: fix build with <gcc-5 versions
  2022-03-17  9:49               ` Corinna Vinschen
@ 2022-03-17 11:26                 ` Richard Earnshaw
  2022-03-18  7:24                   ` Corinna Vinschen
  2022-03-20  1:21                 ` Mike Frysinger
  1 sibling, 1 reply; 24+ messages in thread
From: Richard Earnshaw @ 2022-03-17 11:26 UTC (permalink / raw)
  To: newlib



On 17/03/2022 09:49, Corinna Vinschen wrote:
> On Mar 16 22:41, Mike Frysinger wrote:
>> On 16 Mar 2022 10:17, R. Diez wrote:
>>>>> Therefore, compiling your code with GCC < 5 will silently break your application.
>>>>> After all, the only reason to use __builtin_mul_overflow() is
>>>>> that you need to check for overflow, is it?
>>>>
>>>> practically speaking, i don't think this is a big deal.  newlib gained these
>>>> checks only "recently" (<2 years ago).  newlib has been around for much much
>>>> longer, and the world didn't notice.
>>>
>>> Such general justifications wouldn't pass quality assurance (if we had one).
>>
>> in your opinion.  software is not perfect, it's trade-offs.
>>
>>>> yes, if an app starts trying to allocate
>>>> huge amounts of memory such that it triggers 32-bit overflows when calculating,
>>>> the new size, it will probably internally allocate fewer bytes than requested,
>>>> and things will get corrupted.  but like, don't do that :p.  such applications
>>>> probably will have other problems already.
>>>
>>> You are suggesting that this only affects memory allocation, but the patch is for libc/include/sys/cdefs.h , so those mine traps will be available for everybody.
>>>
>>> People will tend to assume that anything in Newlib is correct, and code has a way to get copied around and re-used.
>>>
>>> There are many ways to mitigate the risk:
>>>
>>> - Require GCC 5.
>>> - Provide a proper implementation of __builtin_mul_overflow().
>>> - Patch all users of __builtin_mul_overflow() within Newlib, so that they do not use it if the compiler does not provide it.
>>> - Issue a compilation warning for GCC < 5 that the "stub" __builtin_mul_overflow() is broken.
>>>     Note that this is not actually a "stub" implementation in the common sense.
>>> - Add an "assert( false ) // fix me" inside the implementation.
>>> - Add a comment stating that the "stub" implementation is not actually correct.
>>
>> any option that prevents correct execution with gcc-4 is not an improvement.
>> if you care this much, feel free to contribute a patch.  or use gcc-5+ and
>> not worry about it.
>> -mike
> 
> Does anybody actually care for building with gcc < 5?  If not, we
> should just make gcc 5 a prerequisite.
> 
> 

It's not just about old GCC, it's about any C compiler that doesn't have 
that builtin.

R.
> Corinna
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] newlib: fix build with <gcc-5 versions
  2022-03-17 11:26                 ` Richard Earnshaw
@ 2022-03-18  7:24                   ` Corinna Vinschen
  2022-03-18  8:30                     ` R. Diez
  0 siblings, 1 reply; 24+ messages in thread
From: Corinna Vinschen @ 2022-03-18  7:24 UTC (permalink / raw)
  To: newlib

On Mar 17 11:26, Richard Earnshaw wrote:
> On 17/03/2022 09:49, Corinna Vinschen wrote:
> > On Mar 16 22:41, Mike Frysinger wrote:
> > > On 16 Mar 2022 10:17, R. Diez wrote:
> > > > There are many ways to mitigate the risk:
> > > > 
> > > > - Require GCC 5.
> > > > - Provide a proper implementation of __builtin_mul_overflow().
> > > > - Patch all users of __builtin_mul_overflow() within Newlib, so that they do not use it if the compiler does not provide it.
> > > > - Issue a compilation warning for GCC < 5 that the "stub" __builtin_mul_overflow() is broken.
> > > >     Note that this is not actually a "stub" implementation in the common sense.
> > > > - Add an "assert( false ) // fix me" inside the implementation.
> > > > - Add a comment stating that the "stub" implementation is not actually correct.
> > > 
> > > any option that prevents correct execution with gcc-4 is not an improvement.
> > > if you care this much, feel free to contribute a patch.  or use gcc-5+ and
> > > not worry about it.
> > > -mike
> > 
> > Does anybody actually care for building with gcc < 5?  If not, we
> > should just make gcc 5 a prerequisite.
> 
> It's not just about old GCC, it's about any C compiler that doesn't have
> that builtin.

Well, I guess, GTG then.


Thanks,
Corinna


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] newlib: fix build with <gcc-5 versions
  2022-03-18  7:24                   ` Corinna Vinschen
@ 2022-03-18  8:30                     ` R. Diez
  2022-03-18  9:26                       ` Corinna Vinschen
  0 siblings, 1 reply; 24+ messages in thread
From: R. Diez @ 2022-03-18  8:30 UTC (permalink / raw)
  To: Corinna Vinschen, Mike Frysinger, Richard Earnshaw
  Cc: newlib, sebastian.huber


>> It's not just about old GCC, it's about any C compiler that doesn't have
>> that builtin.
> 
> Well, I guess, GTG then.


Let's see if I understand the situation:

You are committing a replacement implementation of __builtin_mul_overflow() for older GCC versions and for any other compiler which does not have it.

The only significant extra feature about that function is the detection of integer overflow.

The implementation lives in libc/include/sys/cdefs.h , so it is accessible not just by some special malloc code which should never overflow because targets wouldn't have that much memory or whatever.

The replacement implementation is known to be broken and therefore poses a risk on anybody relying on the original, documented behaviour.

There are no mitigation measures. There is not even a comment next to the replacement implementation that states it is broken.

And you guys are fine with that.

Is that correct?

Regards,
   rdiez

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] newlib: fix build with <gcc-5 versions
  2022-03-18  8:30                     ` R. Diez
@ 2022-03-18  9:26                       ` Corinna Vinschen
  2022-03-18  9:45                         ` R. Diez
  0 siblings, 1 reply; 24+ messages in thread
From: Corinna Vinschen @ 2022-03-18  9:26 UTC (permalink / raw)
  To: newlib

On Mar 18 09:30, R. Diez via Newlib wrote:
> 
> > > It's not just about old GCC, it's about any C compiler that doesn't have
> > > that builtin.
> > 
> > Well, I guess, GTG then.
> 
> 
> Let's see if I understand the situation:
> 
> You are committing a replacement implementation of __builtin_mul_overflow() for older GCC versions and for any other compiler which does not have it.
> 
> The only significant extra feature about that function is the detection of integer overflow.
> 
> The implementation lives in libc/include/sys/cdefs.h , so it is
> accessible not just by some special malloc code which should never
> overflow because targets wouldn't have that much memory or whatever.
> 
> The replacement implementation is known to be broken and therefore poses a risk on anybody relying on the original, documented behaviour.
> 
> There are no mitigation measures. There is not even a comment next to the replacement implementation that states it is broken.
> 
> And you guys are fine with that.
> 
> Is that correct?

I'm open to a better or followup patch.


Corinna


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] newlib: fix build with <gcc-5 versions
  2022-03-18  9:26                       ` Corinna Vinschen
@ 2022-03-18  9:45                         ` R. Diez
  2022-03-20  1:22                           ` Mike Frysinger
  0 siblings, 1 reply; 24+ messages in thread
From: R. Diez @ 2022-03-18  9:45 UTC (permalink / raw)
  To: Corinna Vinschen; +Cc: Newlib


> I'm open to a better or followup patch.

Why are you willing to accept such a blatantly incorrect and dangerous patch?

Rejecting it seems like the obvious thing to do in the first place.

Regards,
   rdiez

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] newlib: fix build with <gcc-5 versions
  2022-03-17  9:49               ` Corinna Vinschen
  2022-03-17 11:26                 ` Richard Earnshaw
@ 2022-03-20  1:21                 ` Mike Frysinger
  2022-03-20 13:57                   ` Jordi Sanfeliu
  1 sibling, 1 reply; 24+ messages in thread
From: Mike Frysinger @ 2022-03-20  1:21 UTC (permalink / raw)
  To: newlib

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

On 17 Mar 2022 10:49, Corinna Vinschen wrote:
> On Mar 16 22:41, Mike Frysinger wrote:
> > On 16 Mar 2022 10:17, R. Diez wrote:
> > > >> Therefore, compiling your code with GCC < 5 will silently break your application.
> > > >> After all, the only reason to use __builtin_mul_overflow() is
> > > >> that you need to check for overflow, is it?
> > > > 
> > > > practically speaking, i don't think this is a big deal.  newlib gained these
> > > > checks only "recently" (<2 years ago).  newlib has been around for much much
> > > > longer, and the world didn't notice.
> > > 
> > > Such general justifications wouldn't pass quality assurance (if we had one).
> > 
> > in your opinion.  software is not perfect, it's trade-offs.
> > 
> > > > yes, if an app starts trying to allocate
> > > > huge amounts of memory such that it triggers 32-bit overflows when calculating,
> > > > the new size, it will probably internally allocate fewer bytes than requested,
> > > > and things will get corrupted.  but like, don't do that :p.  such applications
> > > > probably will have other problems already.
> > > 
> > > You are suggesting that this only affects memory allocation, but the patch is for libc/include/sys/cdefs.h , so those mine traps will be available for everybody.
> > > 
> > > People will tend to assume that anything in Newlib is correct, and code has a way to get copied around and re-used.
> > > 
> > > There are many ways to mitigate the risk:
> > > 
> > > - Require GCC 5.
> > > - Provide a proper implementation of __builtin_mul_overflow().
> > > - Patch all users of __builtin_mul_overflow() within Newlib, so that they do not use it if the compiler does not provide it.
> > > - Issue a compilation warning for GCC < 5 that the "stub" __builtin_mul_overflow() is broken.
> > >    Note that this is not actually a "stub" implementation in the common sense.
> > > - Add an "assert( false ) // fix me" inside the implementation.
> > > - Add a comment stating that the "stub" implementation is not actually correct.
> > 
> > any option that prevents correct execution with gcc-4 is not an improvement.
> > if you care this much, feel free to contribute a patch.  or use gcc-5+ and
> > not worry about it.
> > -mike
> 
> Does anybody actually care for building with gcc < 5?  If not, we
> should just make gcc 5 a prerequisite.

i'm using gcc 4.9 for one of my targets which is why i noticed :).
-mike

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] newlib: fix build with <gcc-5 versions
  2022-03-18  9:45                         ` R. Diez
@ 2022-03-20  1:22                           ` Mike Frysinger
  0 siblings, 0 replies; 24+ messages in thread
From: Mike Frysinger @ 2022-03-20  1:22 UTC (permalink / raw)
  To: R. Diez; +Cc: Newlib

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

On 18 Mar 2022 10:45, R. Diez via Newlib wrote:
> > I'm open to a better or followup patch.
> 
> Why are you willing to accept such a blatantly incorrect and dangerous patch?

i think you're blatantly overstating the actual impact here.
-mike

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: Re: [PATCH v2] newlib: fix build with <gcc-5 versions
@ 2022-03-20 12:52       ` Eric Bresie
  2022-03-20 14:16         ` Mike Frysinger
  0 siblings, 1 reply; 24+ messages in thread
From: Eric Bresie @ 2022-03-20 12:52 UTC (permalink / raw)
  To: Newlib

My C is a little rusty so forgive me up front if I’m not reading something quite right…

Trying to understand the basic logic, the macro is expected to return Boolean but the expression is assigning the multiplication results to the size and then always returning 0 (false). Is that flow correct? Should there be some form of “==“ involved and/or ever return non-zero number?

Eric Bresie
Ebresie@gmail.com (mailto:Ebresie@gmail.com)

> On March 15, 2022 at 6:54:00 PM CDT, Mike Frysinger <vapier@gentoo.org (mailto:vapier@gentoo.org)> wrote:
> On 15 Mar 2022 12:41, Richard Earnshaw wrote:
> > On 15/03/2022 03:25, Mike Frysinger wrote:
> > > __builtin_mul_overflow showed up with gcc-5, so stub it out for older
> > > versions.
> > > ---
> > > newlib/libc/include/sys/cdefs.h | 4 ++++
> > > 1 file changed, 4 insertions(+)
> > >
> > > diff --git a/newlib/libc/include/sys/cdefs.h b/newlib/libc/include/sys/cdefs.h
> > > index da729092185c..e51f7f4b873a 100644
> > > --- a/newlib/libc/include/sys/cdefs.h
> > > +++ b/newlib/libc/include/sys/cdefs.h
> > > @@ -416,6 +416,10 @@
> > > #define __unreachable() ((void)0)
> > > #endif
> > >
> > > +#if !__GNUC_PREREQ__(5, 0)
> > > +#define __builtin_mul_overflow(a, b, size) ({ *(size) = (a) * (b); 0; })
> >
> > Wouldn't
> > (*(size) = (a) * (b), 0)
> >
> > be more portable (avoiding the GNU statement expression extension)?
>
> sure, that works too, thanks
> -mike
> signature.asc849 bytes (#attachment-1)

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] newlib: fix build with <gcc-5 versions
  2022-03-20  1:21                 ` Mike Frysinger
@ 2022-03-20 13:57                   ` Jordi Sanfeliu
  0 siblings, 0 replies; 24+ messages in thread
From: Jordi Sanfeliu @ 2022-03-20 13:57 UTC (permalink / raw)
  To: newlib

For what it’s worth, I'm using GCC 4.x in my humble hobby OS (fiwix.org) 
focused on old i386 machines. So for me, will be fine to continue be 
able to build Newlib with GCC 4.x.

Also, I noticed that GCC 4.x needs less memory footprint than GCC 5.x, 
so it makes it more appreciated in old machines.

Thanks.



On 3/20/22 02:21, Mike Frysinger wrote:
> On 17 Mar 2022 10:49, Corinna Vinschen wrote:
>> On Mar 16 22:41, Mike Frysinger wrote:
>>> On 16 Mar 2022 10:17, R. Diez wrote:
>>>>>> Therefore, compiling your code with GCC < 5 will silently break your application.
>>>>>> After all, the only reason to use __builtin_mul_overflow() is
>>>>>> that you need to check for overflow, is it?
>>>>>
>>>>> practically speaking, i don't think this is a big deal.  newlib gained these
>>>>> checks only "recently" (<2 years ago).  newlib has been around for much much
>>>>> longer, and the world didn't notice.
>>>>
>>>> Such general justifications wouldn't pass quality assurance (if we had one).
>>>
>>> in your opinion.  software is not perfect, it's trade-offs.
>>>
>>>>> yes, if an app starts trying to allocate
>>>>> huge amounts of memory such that it triggers 32-bit overflows when calculating,
>>>>> the new size, it will probably internally allocate fewer bytes than requested,
>>>>> and things will get corrupted.  but like, don't do that :p.  such applications
>>>>> probably will have other problems already.
>>>>
>>>> You are suggesting that this only affects memory allocation, but the patch is for libc/include/sys/cdefs.h , so those mine traps will be available for everybody.
>>>>
>>>> People will tend to assume that anything in Newlib is correct, and code has a way to get copied around and re-used.
>>>>
>>>> There are many ways to mitigate the risk:
>>>>
>>>> - Require GCC 5.
>>>> - Provide a proper implementation of __builtin_mul_overflow().
>>>> - Patch all users of __builtin_mul_overflow() within Newlib, so that they do not use it if the compiler does not provide it.
>>>> - Issue a compilation warning for GCC < 5 that the "stub" __builtin_mul_overflow() is broken.
>>>>     Note that this is not actually a "stub" implementation in the common sense.
>>>> - Add an "assert( false ) // fix me" inside the implementation.
>>>> - Add a comment stating that the "stub" implementation is not actually correct.
>>>
>>> any option that prevents correct execution with gcc-4 is not an improvement.
>>> if you care this much, feel free to contribute a patch.  or use gcc-5+ and
>>> not worry about it.
>>> -mike
>>
>> Does anybody actually care for building with gcc < 5?  If not, we
>> should just make gcc 5 a prerequisite.
> 
> i'm using gcc 4.9 for one of my targets which is why i noticed :).
> -mike

-- 
Jordi Sanfeliu
FIBRANET Network Services Provider
https://www.fibranet.cat

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] newlib: fix build with <gcc-5 versions
  2022-03-20 12:52       ` Eric Bresie
@ 2022-03-20 14:16         ` Mike Frysinger
  0 siblings, 0 replies; 24+ messages in thread
From: Mike Frysinger @ 2022-03-20 14:16 UTC (permalink / raw)
  To: Eric Bresie; +Cc: Newlib

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

On 20 Mar 2022 07:52, Eric Bresie wrote:
> My C is a little rusty so forgive me up front if I’m not reading something quite right…
> 
> Trying to understand the basic logic, the macro is expected to return Boolean but the expression is assigning the multiplication results to the size and then always returning 0 (false). Is that flow correct? Should there be some form of “==“ involved and/or ever return non-zero number?

it returns a bool to indicate whether there was overflow, but the result of the
actual multiplication of the first two operands is stored in the 3rd arg.

a return value of true means "the value overflowed", not "the multiplication
was successful".  hence returning false is what the stub should do.

https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html
Built-in Function: bool __builtin_mul_overflow (type1 a, type2 b, type3 *res)
These built-in functions promote the first two operands into infinite precision
signed type and perform multiplication on those promoted operands. The result is
then cast to the type the third pointer argument points to and stored there. If
the stored result is equal to the infinite precision result, the built-in
functions return false, otherwise they return true. As the multiplication is
performed in infinite signed precision, these built-in functions have fully
defined behavior for all argument values.
-mike

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v2] newlib: fix build with <gcc-5 versions
       [not found] <1647807849.2524.1.ref@smtp.mail.att.net>
@ 2022-03-20 20:24 ` Steven J Abner
  0 siblings, 0 replies; 24+ messages in thread
From: Steven J Abner @ 2022-03-20 20:24 UTC (permalink / raw)
  To: newlib

Solution was quick attempt at original i386 problem, I am aware one 
would need to break down for actual 64 or 128 bit returns and could use 
96,160 bit math depending
on what (a) and (b) enter as, but question of why 0 if is to be newlib 
supplied.
Curiousity
Steve
 >On 20 Mar 2022 12:13, I wrote:
 >I'm probably out of my depth, but along the same question:
 >Why 0 return and not ((bool)(((unsigned long long)(a) * (b)) >> 32))
 >or prepend with '!!'?




^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v2] newlib: fix build with <gcc-5 versions
       [not found] <1647792834.2524.0.ref@smtp.mail.att.net>
@ 2022-03-20 16:13 ` Steven J Abner
  0 siblings, 0 replies; 24+ messages in thread
From: Steven J Abner @ 2022-03-20 16:13 UTC (permalink / raw)
  To: newlib

I'm probably out of my depth, but along the same question:
Why 0 return and not ((bool)(((unsigned long long)(a) * (b)) >> 32))
or prepend with '!!'?
Steve

 >On 20 Mar 2022 07:52, Eric Bresie wrote:
 >> My C is a little rusty so forgive me up front if I’m not reading 
something quite right…
 >>
 >> Trying to understand the basic logic, the macro is expected to 
return Boolean but the expression is assigning the multiplication 
results to the size and then always returning 0 (false). Is that flow 
correct? Should there be some form of “==“ involved and/or ever 
return non-zero number?

 >it returns a bool to indicate whether there was overflow, but the 
result of the
actual multiplication of the first two operands is stored in the 3rd 
arg.

 >a return value of true means "the value overflowed", not "the 
multiplication
was successful". hence returning false is what the stub should do.




^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2022-03-20 20:24 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-14  3:25 [PATCH] newlib: fix build with <gcc-5 versions Mike Frysinger
2022-03-14  7:36 ` Sebastian Huber
2022-03-14 16:58   ` Mike Frysinger
2022-03-15  3:25 ` [PATCH v2] " Mike Frysinger
2022-03-15 12:41   ` Richard Earnshaw
2022-03-15 23:54     ` Mike Frysinger
2022-03-16  7:12       ` R. Diez
2022-03-16  8:30         ` Mike Frysinger
2022-03-16  9:17           ` R. Diez
2022-03-17  2:41             ` Mike Frysinger
2022-03-17  9:49               ` Corinna Vinschen
2022-03-17 11:26                 ` Richard Earnshaw
2022-03-18  7:24                   ` Corinna Vinschen
2022-03-18  8:30                     ` R. Diez
2022-03-18  9:26                       ` Corinna Vinschen
2022-03-18  9:45                         ` R. Diez
2022-03-20  1:22                           ` Mike Frysinger
2022-03-20  1:21                 ` Mike Frysinger
2022-03-20 13:57                   ` Jordi Sanfeliu
2022-03-20 12:52       ` Eric Bresie
2022-03-20 14:16         ` Mike Frysinger
2022-03-15 23:53 ` [PATCH v3] " Mike Frysinger
     [not found] <1647792834.2524.0.ref@smtp.mail.att.net>
2022-03-20 16:13 ` [PATCH v2] " Steven J Abner
     [not found] <1647807849.2524.1.ref@smtp.mail.att.net>
2022-03-20 20:24 ` Steven J Abner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).