Several fixes around POSIX_SOURCE, remove global definitions of POSIX_SOURCE #67052

aescolar · 2023-12-28T15:54:33Z

Some cleanup around the selection of the API provided by the C library.
The main idea is to limit where the SOURCE selection is applied to much fewer files than before.

This PR includes some commits from #67040 so as to provide a complete solution. But it only includes those commits that are inline with the overall direction of this PR of removing all global definitions of these macros.

Background on the PR strategy differences:
Currently #67040 cleans up only the global definitions done by picolibc, but sets newlibc to define those globally.
The current assumption in 67040 is that all functions listed in the coding guidelines rules A.4 and A.5 should be globally available, without needing to set any macro by the user file, even that some of those functions are not part of the C std library API but extensions. Unfortunately, this sets a requirement on the libC integration to ensure they are exposed. See #67040 (comment).

Instead, this PR solves the global use of POSIX_SOURCE issue, while postponing deciding on and solving the libC integration topic ( #67040 (comment) )

Notes about the checkpatch warnings:

For the native port specific driver "bottoms", coding guidelines rules A.4/5 do not really apply as they intend to limit the C library use in the embedded code, while this is host code. So I would think those warnings should just be ignored. A PR to clarify the rule has been submitted ( doc coding_guidelines: Exclude host tooling from rule A.5 #68004 )
The warnings in subsys/shell/modules/devmem_service.c & subsys/net/lib/lwm2m/lwm2m_rw_cbor.c are violations by the original code, which are uncovered now.
- subsys/shell/modules/devmem_service.c violates coding guidelines rule A.5 #68002
- subsys/net/lib/lwm2m/lwm2m_rw_cbor.c violates coding guidelines rule A.5 #68003
The warning in tests/bluetooth/uuid/src/test_bt_uuid_to_str.c & tests/benchmarks/footprints/src/libc.c are due to strnlen which is an exception to rule A.4, and therefore I think the checkpatch warning in this PR should be waived.

Sidenote: We can avoid all those check-patch warnings by defining these macros in cmake, but it does not feel nice to do so, just because of checkpatch.

Fixes #66921

drivers/can/can_native_linux_adapt.c

keith-packard

The ssize_t -> int changes are unrelated and also seem wrong? I'd also remove the #undef bits; they shouldn't be necessary once we get rid of the global definitions

keith-packard · 2023-12-28T18:18:14Z

drivers/eeprom/eeprom_simulator.c

@@ -7,6 +7,20 @@

 #define DT_DRV_COMPAT zephyr_sim_eeprom

+#ifdef CONFIG_ARCH_POSIX
+#undef _POSIX_C_SOURCE


Why the undef here?

As a harmless safety.
A user may define this globally, another library may due to legacy or by mistake.
It is safe to undef macros even if they were never defined.
Given that we are here due to conflicts in this area, and that some feel strongly about defining this kind of macro globally, and that Zephyr as an OS is built quite monotonically with the app it seemed reasonable to play safe.
Wouldn't you want to add them on your PR? :)

bjarki-andreasen

I agree with the changes :)

cfriedt

Temporary nak until the issue described in #67181 can be brought to the AWG.

zephyrbot · 2024-01-24T11:33:12Z

The following west manifest projects have been modified in this Pull Request:

Name	Old Revision	New Revision	Diff

Note: This message is automatically posted and updated by the Manifest GitHub Action.

strnlen() and strtok_r() are tested in this files which are extensions to the the std C library. Let's explicity select one of the extensions also when building with the host C library, instead of relaying on somebody having set it for this file somewhere else. Also, let's be nice and undefine them first in case they had been defined somewhere else in the build scripts. Signed-off-by: Alberto Escolar Piedras <[email protected]>

This test uses functions which are extensions to the C library. Let's explicity select one of the extensions which includes it instead of relaying on somebody having set it for this file somewhere else. (Note this test is exclusive to native targets) Signed-off-by: Alberto Escolar Piedras <[email protected]>

strnlen is not a C standard API, but an extension. Instead of relaying that the SOURCE macro was set somewhere else of that the C library provides a prototype anyhow let's explicitly define it for this library. Signed-off-by: Alberto Escolar Piedras <[email protected]>

This test uses functions and types which are extensions to the C library. Let's explicity select one of the extensions which includes it instead of relaying on somebody having set it for this file somewhere else. Signed-off-by: Alberto Escolar Piedras <[email protected]>

Instead of relaying on those macros having been defined somewhere else let's define them for this file. Signed-off-by: Alberto Escolar Piedras <[email protected]>

This was necessary to get Picolibc to expose the whole Zephyr C library API, but current versions of the SDK use a version of Picolibc with built-in Zephyr support. Signed-off-by: Keith Packard <[email protected]>

Instead of relaying on those macros having been defined somewhere else let's define them for this library. Signed-off-by: Alberto Escolar Piedras <[email protected]>

henrikbrixandersen · 2024-01-25T15:56:56Z

The compliance check failures are false positives.

The srandom function is not available unless _XOPEN_SOURCE is set > 500 or an equivalent declaration is done. Let's set it. Signed-off-by: Alberto Escolar Piedras <[email protected]>

Do not define these macros globally, but instead define them only for this library and when needed. Signed-off-by: Alberto Escolar Piedras <[email protected]>

Both zcbor_encode.c & zcbor_decode.c use strnlen() In general these 2 functions are only exposed by the C library if _POSIX_C_SOURCE is set 200809L. But neither of these files (or their build scripts), are setting this macro, causing build warnings Implicit declaration of function ‘strnlen’ which turn into failures in CI with some libCs. Let's set this macro to avoid this issue. Signed-off-by: Alberto Escolar Piedras <[email protected]>

de-nordic

Looks OK. Thanks!

rlubos

Net part looks ok

cfriedt

LGTM - thanks @aescolar 👍

aescolar force-pushed the POSIX_SOURCE branch 3 times, most recently from b8c9623 to efaaef3 Compare December 28, 2023 17:14

aescolar requested a review from keith-packard December 28, 2023 17:21

keith-packard reviewed Dec 28, 2023

View reviewed changes

drivers/can/can_native_linux_adapt.c Outdated Show resolved Hide resolved

keith-packard reviewed Dec 28, 2023

View reviewed changes

aescolar mentioned this pull request Dec 28, 2023

Remove explicit -D_POSIX_C_SOURCE global compiler options when building with picolibc #67040

Closed

aescolar added the DNM This PR should not be merged (Do Not Merge) label Dec 29, 2023

aescolar requested review from henrikbrixandersen, jukkar, stephanosio, bjarki-andreasen, nordic-krch and rlubos December 29, 2023 09:28

bjarki-andreasen previously approved these changes Dec 29, 2023

View reviewed changes

bjarki-andreasen self-requested a review December 29, 2023 10:31

bjarki-andreasen reviewed Dec 29, 2023

View reviewed changes

cfriedt requested changes Jan 7, 2024

View reviewed changes

aescolar mentioned this pull request Jan 22, 2024

posix: add Kconfig and cmake for application conformance #67132

Draft

aescolar dismissed bjarki-andreasen’s stale review via 3ee105f January 23, 2024 13:03

aescolar force-pushed the POSIX_SOURCE branch 2 times, most recently from 3ee105f to 5eb2b29 Compare January 24, 2024 11:32

zephyrbot added manifest manifest-picolibc and removed DNM This PR should not be merged (Do Not Merge) labels Jan 24, 2024

aescolar changed the title ~~Several fixes around POSIX_SOURCE~~ Several fixes around POSIX_SOURCE, remove global definitions of POSIX_SOURCE Jan 24, 2024

aescolar force-pushed the POSIX_SOURCE branch from 5eb2b29 to c6a5377 Compare January 24, 2024 12:11

aescolar marked this pull request as ready for review January 24, 2024 12:32

zephyrbot requested a review from dcpleung January 24, 2024 12:33

zephyrbot removed the manifest-picolibc label Jan 25, 2024

aescolar and others added 8 commits January 25, 2024 16:29

subsys/net/lib/lwm2m: Define required source standard macros

39537c3

Instead of relaying on those macros having been defined somewhere else let's define them for this file. Signed-off-by: Alberto Escolar Piedras <[email protected]>

libc/picolibc: Remove the global definition of _POSIX_C_SOURCE

97015e1

This was necessary to get Picolibc to expose the whole Zephyr C library API, but current versions of the SDK use a version of Picolibc with built-in Zephyr support. Signed-off-by: Keith Packard <[email protected]>

board native_posix: Define required source standard macros

1d7d129

Instead of relaying on those macros having been defined somewhere else let's define them for this library. Signed-off-by: Alberto Escolar Piedras <[email protected]>

aescolar force-pushed the POSIX_SOURCE branch from 07fa0b4 to 0bc1883 Compare January 25, 2024 15:30

henrikbrixandersen previously approved these changes Jan 25, 2024

View reviewed changes

aescolar dismissed henrikbrixandersen’s stale review via 36b70b7 January 25, 2024 16:45

henrikbrixandersen previously approved these changes Jan 25, 2024

View reviewed changes

carlescufi requested a review from bjarki-andreasen January 25, 2024 17:48

aescolar added 3 commits January 25, 2024 20:08

drivers/entropy/native: Define required SOURCE macro

2443573

The srandom function is not available unless _XOPEN_SOURCE is set > 500 or an equivalent declaration is done. Let's set it. Signed-off-by: Alberto Escolar Piedras <[email protected]>

arch posix: Do not define standard SOURCE macros globally

500492c

Do not define these macros globally, but instead define them only for this library and when needed. Signed-off-by: Alberto Escolar Piedras <[email protected]>

aescolar dismissed henrikbrixandersen’s stale review via d4b85c6 January 25, 2024 19:09

aescolar force-pushed the POSIX_SOURCE branch from 36b70b7 to d4b85c6 Compare January 25, 2024 19:09

jukkar approved these changes Jan 26, 2024

View reviewed changes

henrikbrixandersen approved these changes Jan 26, 2024

View reviewed changes

bjarki-andreasen approved these changes Jan 26, 2024

View reviewed changes

de-nordic approved these changes Jan 26, 2024

View reviewed changes

rlubos approved these changes Jan 26, 2024

View reviewed changes

cfriedt approved these changes Jan 26, 2024

View reviewed changes

nashif merged commit 1b90d29 into zephyrproject-rtos:main Jan 26, 2024
45 of 46 checks passed

aescolar deleted the POSIX_SOURCE branch January 26, 2024 12:49

This was referenced Jan 29, 2024

Define _POSIX_C_SOURCE in a few files which use strnlen() #68233

Closed

[RFC] On the way C libraries (don't) expose all functions defined in coding guideline rules A.4 and A.5 #68278

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Several fixes around POSIX_SOURCE, remove global definitions of POSIX_SOURCE #67052

Several fixes around POSIX_SOURCE, remove global definitions of POSIX_SOURCE #67052

aescolar commented Dec 28, 2023 •

edited

Loading

keith-packard left a comment

keith-packard Dec 28, 2023

aescolar Dec 28, 2023

bjarki-andreasen left a comment •

edited

Loading

cfriedt left a comment

zephyrbot commented Jan 24, 2024 •

edited

Loading

henrikbrixandersen commented Jan 25, 2024 •

edited

Loading

de-nordic left a comment

rlubos left a comment

cfriedt left a comment

Several fixes around POSIX_SOURCE, remove global definitions of POSIX_SOURCE #67052

Several fixes around POSIX_SOURCE, remove global definitions of POSIX_SOURCE #67052

Conversation

aescolar commented Dec 28, 2023 • edited Loading

keith-packard left a comment

Choose a reason for hiding this comment

keith-packard Dec 28, 2023

Choose a reason for hiding this comment

aescolar Dec 28, 2023

Choose a reason for hiding this comment

bjarki-andreasen left a comment • edited Loading

Choose a reason for hiding this comment

cfriedt left a comment

Choose a reason for hiding this comment

zephyrbot commented Jan 24, 2024 • edited Loading

henrikbrixandersen commented Jan 25, 2024 • edited Loading

de-nordic left a comment

Choose a reason for hiding this comment

rlubos left a comment

Choose a reason for hiding this comment

cfriedt left a comment

Choose a reason for hiding this comment

aescolar commented Dec 28, 2023 •

edited

Loading

bjarki-andreasen left a comment •

edited

Loading

zephyrbot commented Jan 24, 2024 •

edited

Loading

henrikbrixandersen commented Jan 25, 2024 •

edited

Loading