[PATCH] time64.inc: Simplify GLIBC_64BIT_TIME_FLAGS usage

[PATCH] time64.inc: Simplify GLIBC_64BIT_TIME_FLAGS usage

[Tom Hochstein]

The implementation uses the append operator to include
GLIBC_64BIT_TIME_FLAGS in TARGET_CC_ARCH, but it places the space in
the GLIBC_64BIT_TIME_FLAGS assignment in order to avoid a 'spurious
space' when the value is empty.

68b50d3 time64: Remove leading whitespace from GLIBC_64BIT_TIME_FLAGS

The problem with this is it requires anyone wishing to assign a value
to GLIBC_64BIT_TIME_FLAGS to add the leading space, otherwise this is
the error:

cc1: error: '-Werror=format-security-D_TIME_BITS=64': no option '-Wformat-security-D_TIME_BITS=64'

Remove the non-standard usage requirement with a different design that
uses the += operator and moves the arch override.

Signed-off-by: Tom Hochstein <tom.hochstein@oss.nxp.com>
---
 meta/conf/distro/include/time64.inc | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/meta/conf/distro/include/time64.inc b/meta/conf/distro/include/time64.inc
index 510da11039..a459013a92 100644
--- a/meta/conf/distro/include/time64.inc
+++ b/meta/conf/distro/include/time64.inc
@@ -8,15 +8,18 @@
 #
 # Working to address those (before Y2038 rolls in) will be appreciated.
 
-GLIBC_64BIT_TIME_FLAGS = " -D_TIME_BITS=64 -D_FILE_OFFSET_BITS=64"
+TARGET_CC_ARCH += "${GLIBC_64BIT_TIME_FLAGS}"
 
 # Only needed for some 32-bit architectures, some relatively newer
 # architectures do not need it ( e.g. riscv32 )
-TARGET_CC_ARCH:append:arm = "${GLIBC_64BIT_TIME_FLAGS}"
-TARGET_CC_ARCH:append:armeb = "${GLIBC_64BIT_TIME_FLAGS}"
-TARGET_CC_ARCH:append:mipsarcho32 = "${GLIBC_64BIT_TIME_FLAGS}"
-TARGET_CC_ARCH:append:powerpc = "${@bb.utils.contains('TUNE_FEATURES', 'm32', '${GLIBC_64BIT_TIME_FLAGS}', '', d)}"
-TARGET_CC_ARCH:append:x86 = "${@bb.utils.contains('TUNE_FEATURES', 'm32', '${GLIBC_64BIT_TIME_FLAGS}', '', d)}"
+GLIBC_64BIT_TIME_FLAGS             = ""
+GLIBC_64BIT_TIME_FLAGS:arm         = "-D_TIME_BITS=64 -D_FILE_OFFSET_BITS=64"
+GLIBC_64BIT_TIME_FLAGS:armeb       = "-D_TIME_BITS=64 -D_FILE_OFFSET_BITS=64"
+GLIBC_64BIT_TIME_FLAGS:mipsarcho32 = "-D_TIME_BITS=64 -D_FILE_OFFSET_BITS=64"
+GLIBC_64BIT_TIME_FLAGS:powerpc     = \
+    "${@bb.utils.contains('TUNE_FEATURES', 'm32', '-D_TIME_BITS=64 -D_FILE_OFFSET_BITS=64', '', d)}"
+GLIBC_64BIT_TIME_FLAGS:x86         = \
+    "${@bb.utils.contains('TUNE_FEATURES', 'm32', '-D_TIME_BITS=64 -D_FILE_OFFSET_BITS=64', '', d)}"
 
 GLIBC_64BIT_TIME_FLAGS:pn-glibc = ""
 GLIBC_64BIT_TIME_FLAGS:pn-glibc-y2038-tests = ""
-- 
2.25.1

Re: [OE-core] [PATCH] time64.inc: Simplify GLIBC_64BIT_TIME_FLAGS usage

[Alexander Kanavin]

On Wed, 24 Jul 2024 at 19:10, Tom Hochstein via lists.openembedded.org
<tom.hochstein=oss.nxp.com@lists.openembedded.org> wrote:
> -GLIBC_64BIT_TIME_FLAGS = " -D_TIME_BITS=64 -D_FILE_OFFSET_BITS=64"
> +TARGET_CC_ARCH += "${GLIBC_64BIT_TIME_FLAGS}"
>
>  # Only needed for some 32-bit architectures, some relatively newer
>  # architectures do not need it ( e.g. riscv32 )
> -TARGET_CC_ARCH:append:arm = "${GLIBC_64BIT_TIME_FLAGS}"
> -TARGET_CC_ARCH:append:armeb = "${GLIBC_64BIT_TIME_FLAGS}"
> -TARGET_CC_ARCH:append:mipsarcho32 = "${GLIBC_64BIT_TIME_FLAGS}"
> -TARGET_CC_ARCH:append:powerpc = "${@bb.utils.contains('TUNE_FEATURES', 'm32', '${GLIBC_64BIT_TIME_FLAGS}', '', d)}"
> -TARGET_CC_ARCH:append:x86 = "${@bb.utils.contains('TUNE_FEATURES', 'm32', '${GLIBC_64BIT_TIME_FLAGS}', '', d)}"
> +GLIBC_64BIT_TIME_FLAGS             = ""
> +GLIBC_64BIT_TIME_FLAGS:arm         = "-D_TIME_BITS=64 -D_FILE_OFFSET_BITS=64"
> +GLIBC_64BIT_TIME_FLAGS:armeb       = "-D_TIME_BITS=64 -D_FILE_OFFSET_BITS=64"
> +GLIBC_64BIT_TIME_FLAGS:mipsarcho32 = "-D_TIME_BITS=64 -D_FILE_OFFSET_BITS=64"
> +GLIBC_64BIT_TIME_FLAGS:powerpc     = \
> +    "${@bb.utils.contains('TUNE_FEATURES', 'm32', '-D_TIME_BITS=64 -D_FILE_OFFSET_BITS=64', '', d)}"
> +GLIBC_64BIT_TIME_FLAGS:x86         = \
> +    "${@bb.utils.contains('TUNE_FEATURES', 'm32', '-D_TIME_BITS=64 -D_FILE_OFFSET_BITS=64', '', d)}"

Apologies, but this is not simpler or better or more readable.
Repeating "-D_TIME_BITS=64 -D_FILE_OFFSET_BITS=64" multiple times
should really be avoided.

What prompted this patch specifically?

Alex

Re: [OE-core] [PATCH] time64.inc: Simplify GLIBC_64BIT_TIME_FLAGS usage

[Tom Hochstein]

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


On 7/24/2024 12:20 PM, Alexander Kanavin wrote:
> On Wed, 24 Jul 2024 at 19:10, Tom Hochstein via lists.openembedded.org
> <tom.hochstein=oss.nxp.com@lists.openembedded.org>  wrote:
>> -GLIBC_64BIT_TIME_FLAGS = " -D_TIME_BITS=64 -D_FILE_OFFSET_BITS=64"
>> +TARGET_CC_ARCH += "${GLIBC_64BIT_TIME_FLAGS}"
>>
>>   # Only needed for some 32-bit architectures, some relatively newer
>>   # architectures do not need it ( e.g. riscv32 )
>> -TARGET_CC_ARCH:append:arm = "${GLIBC_64BIT_TIME_FLAGS}"
>> -TARGET_CC_ARCH:append:armeb = "${GLIBC_64BIT_TIME_FLAGS}"
>> -TARGET_CC_ARCH:append:mipsarcho32 = "${GLIBC_64BIT_TIME_FLAGS}"
>> -TARGET_CC_ARCH:append:powerpc ="${@bb.utils.contains('TUNE_FEATURES', 'm32', 
>> '${GLIBC_64BIT_TIME_FLAGS}', '', d)}"
>> -TARGET_CC_ARCH:append:x86 ="${@bb.utils.contains('TUNE_FEATURES', 'm32', 
>> '${GLIBC_64BIT_TIME_FLAGS}', '', d)}"
>> +GLIBC_64BIT_TIME_FLAGS             = ""
>> +GLIBC_64BIT_TIME_FLAGS:arm         = "-D_TIME_BITS=64 -D_FILE_OFFSET_BITS=64"
>> +GLIBC_64BIT_TIME_FLAGS:armeb       = "-D_TIME_BITS=64 -D_FILE_OFFSET_BITS=64"
>> +GLIBC_64BIT_TIME_FLAGS:mipsarcho32 = "-D_TIME_BITS=64 -D_FILE_OFFSET_BITS=64"
>> +GLIBC_64BIT_TIME_FLAGS:powerpc     = \
>> +"${@bb.utils.contains('TUNE_FEATURES', 'm32', '-D_TIME_BITS=64 
>> -D_FILE_OFFSET_BITS=64', '', d)}"
>> +GLIBC_64BIT_TIME_FLAGS:x86         = \
>> +"${@bb.utils.contains('TUNE_FEATURES', 'm32', '-D_TIME_BITS=64 
>> -D_FILE_OFFSET_BITS=64', '', d)}"
> Apologies, but this is not simpler or better or more readable.
> Repeating "-D_TIME_BITS=64 -D_FILE_OFFSET_BITS=64" multiple times
> should really be avoided.
>
> What prompted this patch specifically?
Thanks, Alex.

We are working to configure the builds of certain recipes so the 
non-Y2038-compliant code is avoided, e.g, by disabling oss-output in 
pulseaudio. That leads to needing to restore GLIBC_64BIT_TIME_FLAGS, 
which for pulseaudio is cleared here in time64.inc (on scarthgap, not on 
master):

GLIBC_64BIT_TIME_FLAGS:pn-pulseaudio = ""

When you do that override from another layer as one would normally 
expect, i.e., without the leading space, you get the error:

cc1: error: '-Werror=format-security-D_TIME_BITS=64': no option 
'-Wformat-security-D_TIME_BITS=64'

The problem is the design in time64.inc does impose an extra requirement 
for an external assignment to include a leading space. The redesign is 
meant to remove that requirement on the leading space, i.e., to simplify 
the usage of the variable by external users.

Tom


[-- Attachment #2: Type: text/html, Size: 3954 bytes --]

Re: [OE-core] [PATCH] time64.inc: Simplify GLIBC_64BIT_TIME_FLAGS usage

[Tom Hochstein]

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


On 7/24/2024 12:20 PM, Alexander Kanavin wrote:
> On Wed, 24 Jul 2024 at 19:10, Tom Hochstein via lists.openembedded.org
> <tom.hochstein=oss.nxp.com@lists.openembedded.org>  wrote:
>> -GLIBC_64BIT_TIME_FLAGS = " -D_TIME_BITS=64 -D_FILE_OFFSET_BITS=64"
>> +TARGET_CC_ARCH += "${GLIBC_64BIT_TIME_FLAGS}"
>>
>>   # Only needed for some 32-bit architectures, some relatively newer
>>   # architectures do not need it ( e.g. riscv32 )
>> -TARGET_CC_ARCH:append:arm = "${GLIBC_64BIT_TIME_FLAGS}"
>> -TARGET_CC_ARCH:append:armeb = "${GLIBC_64BIT_TIME_FLAGS}"
>> -TARGET_CC_ARCH:append:mipsarcho32 = "${GLIBC_64BIT_TIME_FLAGS}"
>> -TARGET_CC_ARCH:append:powerpc ="${@bb.utils.contains('TUNE_FEATURES', 'm32', 
>> '${GLIBC_64BIT_TIME_FLAGS}', '', d)}"
>> -TARGET_CC_ARCH:append:x86 ="${@bb.utils.contains('TUNE_FEATURES', 'm32', 
>> '${GLIBC_64BIT_TIME_FLAGS}', '', d)}"
>> +GLIBC_64BIT_TIME_FLAGS             = ""
>> +GLIBC_64BIT_TIME_FLAGS:arm         = "-D_TIME_BITS=64 -D_FILE_OFFSET_BITS=64"
>> +GLIBC_64BIT_TIME_FLAGS:armeb       = "-D_TIME_BITS=64 -D_FILE_OFFSET_BITS=64"
>> +GLIBC_64BIT_TIME_FLAGS:mipsarcho32 = "-D_TIME_BITS=64 -D_FILE_OFFSET_BITS=64"
>> +GLIBC_64BIT_TIME_FLAGS:powerpc     = \
>> +"${@bb.utils.contains('TUNE_FEATURES', 'm32', '-D_TIME_BITS=64 
>> -D_FILE_OFFSET_BITS=64', '', d)}"
>> +GLIBC_64BIT_TIME_FLAGS:x86         = \
>> +"${@bb.utils.contains('TUNE_FEATURES', 'm32', '-D_TIME_BITS=64 
>> -D_FILE_OFFSET_BITS=64', '', d)}"
> Apologies, but this is not simpler or better or more readable.
> Repeating "-D_TIME_BITS=64 -D_FILE_OFFSET_BITS=64" multiple times
> should really be avoided.
>
> What prompted this patch specifically?

Thanks, Alex.

We are working to configure the builds of certain recipes so the 
non-Y2038-compliant code is avoided, e.g, by disabling oss-output in 
pulseaudio. That leads to needing to restore GLIBC_64BIT_TIME_FLAGS, 
which for pulseaudio is cleared in this file (on scarthgap, not on master):

GLIBC_64BIT_TIME_FLAGS:pn-pulseaudio = ""

When you do that override as one would normally expect, i.e., without 
the leading space, you get the error:

cc1: error: '-Werror=format-security-D_TIME_BITS=64': no option 
'-Wformat-security-D_TIME_BITS=64'

The problem is the design in time64.inc does impose an extra requirement 
for an external assignment to include a leading space. The redesign is 
meant to remove that requirement on the leading space, i.e., to simplify 
the usage of the variable by external users.

Tom

[-- Attachment #2: Type: text/html, Size: 4116 bytes --]

Re: [OE-core] [PATCH] time64.inc: Simplify GLIBC_64BIT_TIME_FLAGS usage

[Alexander Kanavin]

On Wed, 24 Jul 2024 at 21:33, Tom Hochstein <tom.hochstein@oss.nxp.com> wrote:

> Thanks, Alex.
>
> We are working to configure the builds of certain recipes so the non-Y2038-compliant code is avoided, e.g, by disabling oss-output in pulseaudio. That leads to needing to restore GLIBC_64BIT_TIME_FLAGS, which for pulseaudio is cleared in this file (on scarthgap, not on master):
>
> GLIBC_64BIT_TIME_FLAGS:pn-pulseaudio = ""
>
> When you do that override as one would normally expect, i.e., without the leading space, you get the error:
>
> cc1: error: '-Werror=format-security-D_TIME_BITS=64': no option '-Wformat-security-D_TIME_BITS=64'
>
> The problem is the design in time64.inc does impose an extra requirement for an external assignment to include a leading space. The redesign is meant to remove that requirement on the leading space, i.e., to simplify the usage of the variable by external users.

Thanks for the background. I guess the only real objection I have is
about repeating the flags multiple times. They should be defined once,
so we probably need an extra intermediate variable that would be set
with target overrides.

Alex

Re: [OE-core] [PATCH] time64.inc: Simplify GLIBC_64BIT_TIME_FLAGS usage

[Alexandre Belloni]

On 24/07/2024 22:29:45+0200, Alexander Kanavin wrote:
> On Wed, 24 Jul 2024 at 21:33, Tom Hochstein <tom.hochstein@oss.nxp.com> wrote:
> 
> > Thanks, Alex.
> >
> > We are working to configure the builds of certain recipes so the non-Y2038-compliant code is avoided, e.g, by disabling oss-output in pulseaudio. That leads to needing to restore GLIBC_64BIT_TIME_FLAGS, which for pulseaudio is cleared in this file (on scarthgap, not on master):
> >
> > GLIBC_64BIT_TIME_FLAGS:pn-pulseaudio = ""
> >
> > When you do that override as one would normally expect, i.e., without the leading space, you get the error:
> >
> > cc1: error: '-Werror=format-security-D_TIME_BITS=64': no option '-Wformat-security-D_TIME_BITS=64'
> >
> > The problem is the design in time64.inc does impose an extra requirement for an external assignment to include a leading space. The redesign is meant to remove that requirement on the leading space, i.e., to simplify the usage of the variable by external users.
> 
> Thanks for the background. I guess the only real objection I have is
> about repeating the flags multiple times. They should be defined once,
> so we probably need an extra intermediate variable that would be set
> with target overrides.

Then why not simply have this intermediate variable contain the initial
GLIBC_64BIT_TIME_FLAGS value with the leading space so recipe can simply
use it to restore the value?

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com