[PATCH] newlib: fix build with <gcc-5 versions

[PATCH] newlib: fix build with <gcc-5 versions

[Mike Frysinger]

__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

Re: [PATCH] newlib: fix build with <gcc-5 versions

[Sebastian Huber]

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/

Re: [PATCH] newlib: fix build with <gcc-5 versions

[Mike Frysinger]

[-- 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 --]

[PATCH v2] newlib: fix build with <gcc-5 versions

[Mike Frysinger]

__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

Re: [PATCH v2] newlib: fix build with <gcc-5 versions

[Richard Earnshaw]

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

Re: [PATCH v2] newlib: fix build with <gcc-5 versions

[Mike Frysinger]

[-- 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 --]

Re: [PATCH v2] newlib: fix build with <gcc-5 versions

[R. Diez]

> __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

Re: [PATCH v2] newlib: fix build with <gcc-5 versions

[Mike Frysinger]

[-- 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 --]

Re: [PATCH v2] newlib: fix build with <gcc-5 versions

[R. Diez]

>> 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

Re: [PATCH v2] newlib: fix build with <gcc-5 versions

[Mike Frysinger]

[-- 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 --]

Re: [PATCH v2] newlib: fix build with <gcc-5 versions

[Corinna Vinschen]

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

Re: [PATCH v2] newlib: fix build with <gcc-5 versions

[Richard Earnshaw]

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
> 

Re: [PATCH v2] newlib: fix build with <gcc-5 versions

[Corinna Vinschen]

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

Re: [PATCH v2] newlib: fix build with <gcc-5 versions

[R. Diez]

>> 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

Re: [PATCH v2] newlib: fix build with <gcc-5 versions

[Corinna Vinschen]

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

Re: [PATCH v2] newlib: fix build with <gcc-5 versions

[R. Diez]

> 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

Re: [PATCH v2] newlib: fix build with <gcc-5 versions

[Mike Frysinger]

[-- 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 --]

Re: [PATCH v2] newlib: fix build with <gcc-5 versions

[Mike Frysinger]

[-- 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 --]

Re: [PATCH v2] newlib: fix build with <gcc-5 versions

[Jordi Sanfeliu]

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

Re: Re: [PATCH v2] newlib: fix build with <gcc-5 versions

[Eric Bresie]

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)

Re: [PATCH v2] newlib: fix build with <gcc-5 versions

[Mike Frysinger]

[-- 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 --]

[PATCH v3] newlib: fix build with <gcc-5 versions

[Mike Frysinger]

__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

Re: Re: [PATCH v2] newlib: fix build with <gcc-5 versions

[Eric Bresie]

Interesting was not familiar with either of these
https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html
https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html

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)