Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

POSIX sysconf breaks builds for many STM32 targets #74623

Closed
besmarsh opened this issue Jun 20, 2024 · 3 comments
Closed

POSIX sysconf breaks builds for many STM32 targets #74623

besmarsh opened this issue Jun 20, 2024 · 3 comments
Assignees
Labels
area: POSIX POSIX API Library bug The issue is a bug, or the PR is fixing a bug platform: STM32 ST Micro STM32

Comments

@besmarsh
Copy link
Contributor

Describe the bug

Zephyr v3.6 added a compile-time sysconf() implementation to the POSIX API, enabled with the Kconfig option CONFIG_POSIX_SYSCONF. One of the options that can be queried is PAGESIZE/PAGE_SIZE (_SC_PAGESIZE/_SC_PAGE_SIZE). Enabling CONFIG_POSIX_SYSCONF and calling sysconf(_SC_PAGESIZE) fails to compile for many STM32 targets. This is obviously an issue for applications wishing to call sysconf(_SC_PAGESIZE) on an affected target.

Zephyr v3.7 added a runtime sysconf() implementation, deprecated CONFIG_POSIX_SYSCONF, and added CONFIG_POSIX_SYSCONF_IMPL_MACRO and CONFIG_POSIX_SYSCONF_IMPL_FULL to select between the compile-time and runtime sysconf() implementations. When CONFIG_POSIX_SYSCONF_IMPL_MACRO=y, the behaviour is the same as in v3.6. When CONFIG_POSIX_SYSCONF_IMPL_FULL=y, the problem is worse as applications will fail to compile even if they don't call sysconf() at all. Even worse, CONFIG_POSIX_SYSCONF_IMPL_FULL is the default when CONFIG_POSIX_API=y and CONFIG_CPP=y, meaning that even applications not using sysconf at all will fail to compile if they enable the POSIX API (now needed for POSIX net socket names after the deprecation of CONFIG_NET_SOCKETS_POSIX_NAMES) and C++ support.

The bug is caused by:

  • Sysconf options are identified by __z_posix_sysconf* macros (defined in unistd.h in v3.6, and sysconf.h in v3.7)
  • __z_posix_sysconf_SC_PAGE_SIZE is defined as PAGE_SIZE, which comes from CONFIG_POSIX_PAGE_SIZE_BITS in v3.6 and CONFIG_POSIX_PAGE_SIZE in v3.7
  • __z_posix_sysconf_SC_PAGESIZE is defined as PAGESIZE, which is defined as PAGE_SIZE (as above), IF PAGESIZE is not already defined
  • PAGESIZE is defined in the STM32 HAL file stm32_hal_legacy.h, which eventually gets included from unistd.h, as FLASH_PAGE_SIZE
  • FLASH_PAGE_SIZE is defined in the STM32 HAL file stm32**xx_hal_flash.h for some but not all STM32 families

Applications targeting STM32 families that have FLASH_PAGE_SIZE defined in their stm32**xx_hal_flash.h will compile, whereas applications targeting STM32 families without FLASH_PAGE_SIZE defined will not.

This raises another issue - for those STM32 families with FLASH_PAGE_SIZE defined where applications using sysconf() do compile, the results of sysconf(_SC_PAGESIZE) and sysconf(_SC_PAGE_SIZE) may be different, despite the documentation for sysconf describing PAGE_SIZE/_SC_PAGE_SIZE as "a synonym for PAGESIZE/_SC_PAGESIZE.

Please also mention any information which could help others to understand
the problem you're facing:

  • What target platform are you using?
    • Will not compile for stm32h735g_disco, among other targets
  • What have you tried to diagnose or workaround this issue?
    • Diagnosis described above
  • Is this a regression? If yes, have you been able to "git bisect" it to a
    specific commit?
    • Issue is not present prior to v3.6 as sysconf() was not implemented
    • Issue is present in v3.6 but only affects applications using sysconf()
    • Issue is present in v3.7 and affects both applications using sysconf(), and applications not using sysconf() but using POSIX API and C++

To Reproduce
Steps to reproduce the behavior:

  1. cd zephyrproject
  2. cmake -b stm32h735g_disco -d build_sysconf_bug zephyr/samples/hello_world -- -DCONFIG_POSIX_API=y -DCONFIG_CPP=y
  3. See error

Expected behavior
A clear and concise description of what you expected to happen.

  • Applications not even using sysconf should compile
  • Applications using sysconf should compile
  • Calling sysconf(_SC_PAGESIZE) and sysconf(_SC_PAGE_SIZE) should yield the same result

Impact
What impact does this issue have on your progress (e.g., annoyance, showstopper)

For affected targets:

  • Applications not using sysconf: Not a showstopper as they can set CONFIG_POSIX_SYSCONF_IMPL_MACRO=y to avoid the issue, but it takes some investigation to discover this workaround
  • Appliations using compile-time sysconf: Annoyance as they can use _SC_PAGE_SIZE rather than _SC_PAGESIZE
  • Application using runtime sysconf: Showstopper

Logs and console output

In file included from .../zephyrproject/modules/hal/stm32/stm32cube/stm32h7xx/drivers/include/stm32h7xx_hal_def.h:30,
                 from .../zephyrproject/modules/hal/stm32/stm32cube/stm32h7xx/drivers/include/stm32h7xx_hal_rcc.h:27,
                 from .../zephyrproject/modules/hal/stm32/stm32cube/stm32h7xx/drivers/include/stm32h7xx_hal_conf.h:246,
                 from .../zephyrproject/modules/hal/stm32/stm32cube/stm32h7xx/drivers/include/stm32h7xx_hal.h:29,
                 from .../zephyrproject/modules/hal/stm32/stm32cube/stm32h7xx/soc/stm32h7xx.h:282,
                 from c:\users\benma\zephyrproject\zephyr\soc\st\stm32\stm32h7x\soc.h:12,
                 from c:\users\benma\zephyrproject\zephyr\modules\cmsis\cmsis_core_m.h:24,
                 from c:\users\benma\zephyrproject\zephyr\modules\cmsis\cmsis_core.h:10,
                 from .../zephyrproject/zephyr/include/zephyr/arch/arm/asm_inline_gcc.h:24,
                 from .../zephyrproject/zephyr/include/zephyr/arch/arm/asm_inline.h:18,
                 from .../zephyrproject/zephyr/include/zephyr/arch/arm/arch.h:33,
                 from .../zephyrproject/zephyr/include/zephyr/arch/cpu.h:19,
                 from .../zephyrproject/zephyr/include/zephyr/kernel_includes.h:36,
                 from .../zephyrproject/zephyr/include/zephyr/kernel.h:17,
                 from .../zephyrproject/zephyr/include/zephyr/posix/pthread.h:13,
                 from .../zephyrproject/zephyr/lib/posix/options/sysconf.c:7:
.../zephyrproject/zephyr/lib/posix/options/sysconf.c: In function 'sysconf':
.../zephyrproject/modules/hal/stm32/stm32cube/stm32h7xx/drivers/include/Legacy/stm32_hal_legacy.h:475:39: error: 'FLASH_PAGE_SIZE' undeclared (first use in this function); did you mean 'FLASH_BANK_SIZE'?
  475 | #define PAGESIZE                      FLASH_PAGE_SIZE
      |                                       ^~~~~~~~~~~~~~~
.../zephyrproject/zephyr/include/zephyr/posix/sys/sysconf.h:281:59: note: in expansion of macro 'PAGESIZE'
  281 | #define __z_posix_sysconf_SC_PAGESIZE                     PAGESIZE
      |                                                           ^~~~~~~~
.../zephyrproject/zephyr/include/zephyr/sys/util_internal.h:105:36: note: in expansion of macro '__z_posix_sysconf_SC_PAGESIZE'
  105 | #define UTIL_PRIMITIVE_CAT(a, ...) a##__VA_ARGS__
      |                                    ^
.../zephyrproject/zephyr/include/zephyr/sys/util_internal.h:104:26: note: in expansion of macro 'UTIL_PRIMITIVE_CAT'
  104 | #define UTIL_CAT(a, ...) UTIL_PRIMITIVE_CAT(a, __VA_ARGS__)
      |                          ^~~~~~~~~~~~~~~~~~
.../zephyrproject/zephyr/include/zephyr/sys/util_internal.h:120:29: note: in expansion of macro 'UTIL_CAT'
  120 | #define _CONCAT_1(arg, ...) UTIL_CAT(arg, _CONCAT_0(__VA_ARGS__))
      |                             ^~~~~~~~
.../zephyrproject/zephyr/include/zephyr/sys/util_internal.h:105:36: note: in expansion of macro '_CONCAT_1'
  105 | #define UTIL_PRIMITIVE_CAT(a, ...) a##__VA_ARGS__
      |                                    ^
.../zephyrproject/zephyr/lib/posix/options/sysconf.c:13:28: note: in expansion of macro 'CONCAT'
   13 | #define z_sysconf(x) (long)CONCAT(__z_posix_sysconf, x)
      |                            ^~~~~~
.../zephyrproject/zephyr/lib/posix/options/sysconf.c:241:24: note: in expansion of macro 'z_sysconf'
  241 |                 return z_sysconf(_SC_PAGESIZE);
      |                        ^~~~~~~~~
.../zephyrproject/modules/hal/stm32/stm32cube/stm32h7xx/drivers/include/Legacy/stm32_hal_legacy.h:475:39: note: each undeclared identifier is reported only once for each function it appears in
  475 | #define PAGESIZE                      FLASH_PAGE_SIZE
      |                                       ^~~~~~~~~~~~~~~
.../zephyrproject/zephyr/include/zephyr/posix/sys/sysconf.h:281:59: note: in expansion of macro 'PAGESIZE'
  281 | #define __z_posix_sysconf_SC_PAGESIZE                     PAGESIZE
      |                                                           ^~~~~~~~
.../zephyrproject/zephyr/include/zephyr/sys/util_internal.h:105:36: note: in expansion of macro '__z_posix_sysconf_SC_PAGESIZE'
  105 | #define UTIL_PRIMITIVE_CAT(a, ...) a##__VA_ARGS__
      |                                    ^
.../zephyrproject/zephyr/include/zephyr/sys/util_internal.h:104:26: note: in expansion of macro 'UTIL_PRIMITIVE_CAT'
  104 | #define UTIL_CAT(a, ...) UTIL_PRIMITIVE_CAT(a, __VA_ARGS__)
      |                          ^~~~~~~~~~~~~~~~~~
.../zephyrproject/zephyr/include/zephyr/sys/util_internal.h:120:29: note: in expansion of macro 'UTIL_CAT'
  120 | #define _CONCAT_1(arg, ...) UTIL_CAT(arg, _CONCAT_0(__VA_ARGS__))
      |                             ^~~~~~~~
.../zephyrproject/zephyr/include/zephyr/sys/util_internal.h:105:36: note: in expansion of macro '_CONCAT_1'
  105 | #define UTIL_PRIMITIVE_CAT(a, ...) a##__VA_ARGS__
      |                                    ^
.../zephyrproject/zephyr/lib/posix/options/sysconf.c:13:28: note: in expansion of macro 'CONCAT'
   13 | #define z_sysconf(x) (long)CONCAT(__z_posix_sysconf, x)
      |                            ^~~~~~
.../zephyrproject/zephyr/lib/posix/options/sysconf.c:241:24: note: in expansion of macro 'z_sysconf'
  241 |                 return z_sysconf(_SC_PAGESIZE);
      |                        ^~~~~~~~~
[115/173] Building C object zephyr/lib/posix/options/CMakeFiles/lib__posix__options.dir/pthread.c.obj
ninja: build stopped: subcommand failed.

Environment (please complete the following information):

  • OS: Windows
  • Toolchain: Zephyr SDK (arm-zephyr-eabi)
  • Commit SHA or Version used: v3.6, main at time of investigation (7f0f0a4)
@besmarsh besmarsh added the bug The issue is a bug, or the PR is fixing a bug label Jun 20, 2024
besmarsh added a commit to besmarsh/zephyr that referenced this issue Jun 20, 2024
The POSIX sysconf() option _SC_PAGESIZE was defined as PAGESIZE, which
was defined as PAGE_SIZE (coming from CONFIG_POSIX_PAGE_SIZE) IF not
already defined. The STM32 HAL defines PAGESIZE for flash pages as
FLASH_PAGE_SIZE, which is only defined in the HAL for some STM32
families. This caused build failures for many STM32 targets.
See issue zephyrproject-rtos#74623 for full description.

In addition, the POSIX sysconf() options _SC_PAGE_SIZE and _SC_PAGESIZE
should be synonymous, but the existing definition could result in them
differing when PAGESIZE is already defined elsewhere.

This commit removes the conditional definition of PAGESIZE and uses
PAGE_SIZE (set from CONFIG_POSIX_PAGE_SIZE) for both _SC_PAGE_SIZE and
_SC_PAGESIZE.

Signed-off-by: Ben Marsh <ben.marsh@helvar.com>
@besmarsh
Copy link
Contributor Author

I've just seen #70136, #73363, and #74428 regarding this same issue.

@nashif nashif added the area: POSIX POSIX API Library label Jun 20, 2024
@cfriedt cfriedt assigned erwango and unassigned cfriedt Jun 20, 2024
@cfriedt cfriedt added the platform: STM32 ST Micro STM32 label Jun 20, 2024
@erwango
Copy link
Member

erwango commented Jun 24, 2024

@cfriedt Should we keep this open now that #74428 was merged.
At least maybe the STM32 label can be removed if #74624 is still considered (as this one is generic).

@cfriedt
Copy link
Member

cfriedt commented Jun 24, 2024

@erwango - I think we can close this. The other pr is less of a bug fix and more of a stability improvement, I would say.

@cfriedt cfriedt closed this as completed Jun 24, 2024
besmarsh added a commit to besmarsh/zephyr that referenced this issue Jun 25, 2024
The POSIX macros ATEXIT_MAX, PAGE_SIZE, and PAGESIZE (queriable through
sysconf()) were conditionally defined only if an existing definition
did not already exist. These should be defined unconditionally in their
header to ensure they get the correct values.

If these macros are defined elsewhere with a different meaning, that's a
problem. There was an issue where PAGESIZE was already defined with a
different meaning. See zephyrproject-rtos#74623 and zephyrproject-rtos#74428.

This commit defines ATEXIT_MAX, PAGE_SIZE, and PAGESIZE unconditionally.

Signed-off-by: Ben Marsh <ben.marsh@helvar.com>
besmarsh added a commit to besmarsh/zephyr that referenced this issue Jul 8, 2024
The POSIX macros PAGE_SIZE and PAGESIZE (queriable through
sysconf()) were conditionally defined only if an existing definition
did not already exist. These should be defined unconditionally in their
header to ensure they get the correct values.

If these macros are defined elsewhere with a different meaning, that's a
problem. There was an issue where PAGESIZE was already defined with a
different meaning. See zephyrproject-rtos#74623 and zephyrproject-rtos#74428.

The POSIX macro ATEXIT_MAX is also conditionally defined and should be
unconditionally defined, but there is currently a definition in picolibc
(picolibc/newlib/libc/include/stdlib.h) so this change will be done
separately.

This commit defines PAGE_SIZE, and PAGESIZE unconditionally.

Signed-off-by: Ben Marsh <ben.marsh@helvar.com>
besmarsh added a commit to besmarsh/zephyr that referenced this issue Jul 8, 2024
The POSIX macros PAGE_SIZE and PAGESIZE (queriable through
sysconf()) were conditionally defined only if an existing definition
did not already exist. These should be defined unconditionally in their
header to ensure they get the correct values.

If these macros are defined elsewhere with a different meaning, that's a
problem. There was an issue where PAGESIZE was already defined with a
different meaning. See zephyrproject-rtos#74623 and zephyrproject-rtos#74428.

The POSIX macro ATEXIT_MAX is also conditionally defined and should be
unconditionally defined, but there is currently a definition in picolibc
(picolibc/newlib/libc/include/stdlib.h) so this change will be done
separately.

This commit defines PAGE_SIZE and PAGESIZE unconditionally.

Signed-off-by: Ben Marsh <ben.marsh@helvar.com>
aescolar pushed a commit that referenced this issue Jul 10, 2024
The POSIX macros PAGE_SIZE and PAGESIZE (queriable through
sysconf()) were conditionally defined only if an existing definition
did not already exist. These should be defined unconditionally in their
header to ensure they get the correct values.

If these macros are defined elsewhere with a different meaning, that's a
problem. There was an issue where PAGESIZE was already defined with a
different meaning. See #74623 and #74428.

The POSIX macro ATEXIT_MAX is also conditionally defined and should be
unconditionally defined, but there is currently a definition in picolibc
(picolibc/newlib/libc/include/stdlib.h) so this change will be done
separately.

This commit defines PAGE_SIZE and PAGESIZE unconditionally.

Signed-off-by: Ben Marsh <ben.marsh@helvar.com>
AlienSarlak pushed a commit to AlienSarlak/zephyr that referenced this issue Jul 13, 2024
The POSIX macros PAGE_SIZE and PAGESIZE (queriable through
sysconf()) were conditionally defined only if an existing definition
did not already exist. These should be defined unconditionally in their
header to ensure they get the correct values.

If these macros are defined elsewhere with a different meaning, that's a
problem. There was an issue where PAGESIZE was already defined with a
different meaning. See zephyrproject-rtos#74623 and zephyrproject-rtos#74428.

The POSIX macro ATEXIT_MAX is also conditionally defined and should be
unconditionally defined, but there is currently a definition in picolibc
(picolibc/newlib/libc/include/stdlib.h) so this change will be done
separately.

This commit defines PAGE_SIZE and PAGESIZE unconditionally.

Signed-off-by: Ben Marsh <ben.marsh@helvar.com>
CZKikin pushed a commit to nxp-upstream/zephyr that referenced this issue Jul 18, 2024
The POSIX macros PAGE_SIZE and PAGESIZE (queriable through
sysconf()) were conditionally defined only if an existing definition
did not already exist. These should be defined unconditionally in their
header to ensure they get the correct values.

If these macros are defined elsewhere with a different meaning, that's a
problem. There was an issue where PAGESIZE was already defined with a
different meaning. See zephyrproject-rtos#74623 and zephyrproject-rtos#74428.

The POSIX macro ATEXIT_MAX is also conditionally defined and should be
unconditionally defined, but there is currently a definition in picolibc
(picolibc/newlib/libc/include/stdlib.h) so this change will be done
separately.

This commit defines PAGE_SIZE and PAGESIZE unconditionally.

Signed-off-by: Ben Marsh <ben.marsh@helvar.com>
Devansh0210 pushed a commit to Devansh0210/zephyr that referenced this issue Jul 20, 2024
The POSIX macros PAGE_SIZE and PAGESIZE (queriable through
sysconf()) were conditionally defined only if an existing definition
did not already exist. These should be defined unconditionally in their
header to ensure they get the correct values.

If these macros are defined elsewhere with a different meaning, that's a
problem. There was an issue where PAGESIZE was already defined with a
different meaning. See zephyrproject-rtos#74623 and zephyrproject-rtos#74428.

The POSIX macro ATEXIT_MAX is also conditionally defined and should be
unconditionally defined, but there is currently a definition in picolibc
(picolibc/newlib/libc/include/stdlib.h) so this change will be done
separately.

This commit defines PAGE_SIZE and PAGESIZE unconditionally.

Signed-off-by: Ben Marsh <ben.marsh@helvar.com>
Thalley pushed a commit to Thalley/zephyr that referenced this issue Aug 1, 2024
The POSIX macros PAGE_SIZE and PAGESIZE (queriable through
sysconf()) were conditionally defined only if an existing definition
did not already exist. These should be defined unconditionally in their
header to ensure they get the correct values.

If these macros are defined elsewhere with a different meaning, that's a
problem. There was an issue where PAGESIZE was already defined with a
different meaning. See zephyrproject-rtos#74623 and zephyrproject-rtos#74428.

The POSIX macro ATEXIT_MAX is also conditionally defined and should be
unconditionally defined, but there is currently a definition in picolibc
(picolibc/newlib/libc/include/stdlib.h) so this change will be done
separately.

This commit defines PAGE_SIZE and PAGESIZE unconditionally.

Signed-off-by: Ben Marsh <ben.marsh@helvar.com>
Chenhongren pushed a commit to Chenhongren/zephyr that referenced this issue Aug 26, 2024
The POSIX macros PAGE_SIZE and PAGESIZE (queriable through
sysconf()) were conditionally defined only if an existing definition
did not already exist. These should be defined unconditionally in their
header to ensure they get the correct values.

If these macros are defined elsewhere with a different meaning, that's a
problem. There was an issue where PAGESIZE was already defined with a
different meaning. See zephyrproject-rtos#74623 and zephyrproject-rtos#74428.

The POSIX macro ATEXIT_MAX is also conditionally defined and should be
unconditionally defined, but there is currently a definition in picolibc
(picolibc/newlib/libc/include/stdlib.h) so this change will be done
separately.

This commit defines PAGE_SIZE and PAGESIZE unconditionally.

(cherry picked from commit 8c0d3de)

Original-Signed-off-by: Ben Marsh <ben.marsh@helvar.com>
GitOrigin-RevId: 8c0d3de
Change-Id: Icb9ff7549525906d976333b9327e3b7dd42975b7
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/zephyr/+/5692888
Tested-by: ChromeOS Prod (Robot) <chromeos-ci-prod@chromeos-bot.iam.gserviceaccount.com>
Tested-by: Keith Short <keithshort@chromium.org>
Reviewed-by: Keith Short <keithshort@chromium.org>
Commit-Queue: Keith Short <keithshort@chromium.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: POSIX POSIX API Library bug The issue is a bug, or the PR is fixing a bug platform: STM32 ST Micro STM32
Projects
None yet
Development

No branches or pull requests

4 participants