-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Unconditionally define POSIX sysconf macros PAGE_SIZE and PAGESIZE #74624
Conversation
#74428 should fix this |
This is fixed by #74428, but this change ensures that no other rogue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the conditions around defining PAGESIZE
should be all that is needed here, but PAGESIZE
also needs to be defined according to the spec.
https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/limits.h.html
#ifndef PAGESIZE | ||
#define PAGESIZE PAGE_SIZE | ||
#endif | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#ifndef PAGESIZE | |
#define PAGESIZE PAGE_SIZE | |
#endif | |
#define PAGESIZE PAGE_SIZE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the conditions around the definitions of PAGE_SIZE
and ATEXIT_MAX
just above also be removed to ensure they are defined correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please
include/zephyr/posix/sys/sysconf.h
Outdated
#define __z_posix_sysconf_SC_PAGESIZE PAGESIZE | ||
#define __z_posix_sysconf_SC_PAGESIZE PAGE_SIZE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be preferable to keep the original definition here. Otherwise, it would be an unnecessary special case for scripts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping the original definition is fine if PAGESIZE
and PAGE_SIZE
are unconditionally defined equal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
PR updated, including change of title and description after scope change. |
|
@besmarsh - if you prefer, you can go back to the way it was before. There will be a "Great Reckoning" not far down the road where we make internal Zephyr headers as consistent as possible with both Picolibc and Newlib. |
Removing the |
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>
I've removed the |
The POSIXsysconf()
option_SC_PAGESIZE
was defined asPAGESIZE
, which was defined asPAGE_SIZE
(coming fromCONFIG_POSIX_PAGE_SIZE
) IF not already defined. The STM32 HAL definesPAGESIZE
for flash pages asFLASH_PAGE_SIZE
, which is only defined in the HAL for some STM32 families. This caused build failures for many STM32 targets. See issue #74623 for full description.In addition, the POSIXsysconf()
options_SC_PAGE_SIZE
and_SC_PAGESIZE
should be synonymous, but the existing definition could result in them differing whenPAGESIZE
is already defined elsewhere.This issue was present in v3.6 but is more of an issue for v3.7 as it causes build failures for applications not even using sysconf at all, if they target an affected STM32 family and enableCONFIG_POSIX_API
andCONFIG_CPP
.This PR removes the conditional definition ofPAGESIZE
and usesPAGE_SIZE
(set fromCONFIG_POSIX_PAGE_SIZE
) for both_SC_PAGE_SIZE
and_SC_PAGESIZE
.The POSIX macros
PAGE_SIZE
andPAGESIZE
(queriable throughsysconf()
) 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 PR defines
PAGE_SIZE
andPAGESIZE
unconditionally.