* [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; 19+ 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] 19+ messages in thread
[parent not found: <1647807849.2524.1.ref@smtp.mail.att.net>]
* [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; 19+ 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] 19+ messages in thread
* [PATCH] newlib: fix build with <gcc-5 versions @ 2022-03-14 3:25 Mike Frysinger 2022-03-15 3:25 ` [PATCH v2] " Mike Frysinger 0 siblings, 1 reply; 19+ 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] 19+ messages in thread
* [PATCH v2] newlib: fix build with <gcc-5 versions 2022-03-14 3:25 [PATCH] " Mike Frysinger @ 2022-03-15 3:25 ` Mike Frysinger 2022-03-15 12:41 ` Richard Earnshaw 0 siblings, 1 reply; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ messages in thread
end of thread, other threads:[~2022-03-20 20:24 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <1647792834.2524.0.ref@smtp.mail.att.net> 2022-03-20 16:13 ` [PATCH v2] newlib: fix build with <gcc-5 versions Steven J Abner [not found] <1647807849.2524.1.ref@smtp.mail.att.net> 2022-03-20 20:24 ` Steven J Abner 2022-03-14 3:25 [PATCH] " 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
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).