| Message ID | 20240724170829.3456577-1-tom.hochstein@oss.nxp.com |
|---|---|
| State | Accepted, archived |
| Commit | e8177827f92e71c80c5b63453d8bbd1defbe1fbc |
| Headers | show |
| Series | time64.inc: Simplify GLIBC_64BIT_TIME_FLAGS usage | expand |
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
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
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
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
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?
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 = ""
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(-)