You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "tmedicci (via GitHub)" <gi...@apache.org> on 2023/04/28 18:45:50 UTC
[GitHub] [nuttx] tmedicci opened a new pull request, #9138: Draft: libc/string: select arch's libc for kernel/userspace optionally
tmedicci opened a new pull request, #9138:
URL: https://github.com/apache/nuttx/pull/9138
## Summary
Please, check the discussion on https://github.com/apache/nuttx/pull/9022#discussion_r1179518496
This is an alternative implementation of https://github.com/apache/nuttx/pull/9022
Add the `LIBC_ARCH_KERNEL_ONLY` and `LIBC_ARCH_USER_ONLY` choices to enable arch-specific code (`LIBC_ARCH_xxx`) to be used only by the kernel or by the userspace when building the protected mode. These options are dependent on the `LIBC_ARCH_ALLOW_KERNEL_ONLY` and `LIBC_ARCH_ALLOW_USER_ONLY` that are meant to be selected by the arch's chip if applicable. If none of these options are selected, none of the choices are available and the arch's libc implementations will be used by the kernel and the userspace. If only one of the `LIBC_ARCH_ALLOW_xxx_ONLY` is selected, only one of the `LIBC_ARCH_xxx_ONLY` choices will be available and selected. Finally, if both `LIBC_ARCH_ALLOW_xxx_ONLY` are selected by the chip, the user is able to choose if arch's libc will be used only by the kernel, only by the userspace, or both.
## Impact
## Testing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1186730948
##########
libs/libc/machine/arm/armv7-a/gnu/arch_memchr.S:
##########
@@ -66,6 +66,10 @@
*
****************************************************************************/
+#include "libc.h"
+
+#if (LIBC_BUILD_FUNCTION == 0)
Review Comment:
But, the flags already exist: LIBC_ARCH_xxx just indicate the target support the optimized implementation which may come from assembly version under libc/machine or ROM version from chip. we need set e.g. CONFIG_ARMV8M_MEMCHR=y To really compile and link libc/machine's version.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1186730948
##########
libs/libc/machine/arm/armv7-a/gnu/arch_memchr.S:
##########
@@ -66,6 +66,10 @@
*
****************************************************************************/
+#include "libc.h"
+
+#if (LIBC_BUILD_FUNCTION == 0)
Review Comment:
But, the flags already exist: LIBC_ARCH_xxx just indicate the target support the optimized implementation may come from assembly version under libc/machine or ROM version from chip. we need set e.g. CONFIG_ARMV8M_MEMCHR=y To really compile and link libc/machine's version.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] gustavonihei commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "gustavonihei (via GitHub)" <gi...@apache.org>.
gustavonihei commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1182992872
##########
libs/libc/machine/Kconfig:
##########
@@ -137,6 +137,54 @@ config LIBC_ARCH_ELF_64BIT
default n
depends on LIBC_ARCH_ELF
+config LIBC_ARCH_PREVENT_KERNEL
+ bool
+ default n
+ ---help---
+ Prevent the architecture-specific libc from being used in the kernel.
+
+config LIBC_ARCH_PREVENT_USER
+ bool
+ default n
+ ---help---
+ Prevent the architecture-specific libc from being used in the
+ userspace.
+
+if BUILD_PROTECTED || BUILD_KERNEL
+
+choice LIBC_ARCH_USAGE
Review Comment:
Sorry, that's not what I meant. Instead I mean removing the `choice` completely and `LIBC_ARCH_PREVENT_[KERNEL/USER]` directly.
```c
#if (defined(CONFIG_LIBC_ARCH_PREVENT_KERNEL) && defined(__KERNEL__)) || \
(defined(CONFIG_LIBC_ARCH_PREVENT_USER) && !defined(__KERNEL__))
```
> Furthermore, I think it's desirable to provide users a way to select whether to use arch's libc only in the kernel/userspace (if not "prevented" by the chip) depending on their applications.
I disagree. Protected mode has been there for ages and nobody had this necessity, so most probably it will never be used.
Again, this will only increase the maintenance burden.
##########
libs/libc/machine/Kconfig:
##########
@@ -137,6 +137,54 @@ config LIBC_ARCH_ELF_64BIT
default n
depends on LIBC_ARCH_ELF
+config LIBC_ARCH_PREVENT_KERNEL
+ bool
+ default n
+ ---help---
+ Prevent the architecture-specific libc from being used in the kernel.
+
+config LIBC_ARCH_PREVENT_USER
+ bool
+ default n
+ ---help---
+ Prevent the architecture-specific libc from being used in the
+ userspace.
+
+if BUILD_PROTECTED || BUILD_KERNEL
+
+choice LIBC_ARCH_USAGE
Review Comment:
Sorry, that's not what I meant. Instead I mean removing the `choice` completely and use `LIBC_ARCH_PREVENT_[KERNEL/USER]` directly.
```c
#if (defined(CONFIG_LIBC_ARCH_PREVENT_KERNEL) && defined(__KERNEL__)) || \
(defined(CONFIG_LIBC_ARCH_PREVENT_USER) && !defined(__KERNEL__))
```
> Furthermore, I think it's desirable to provide users a way to select whether to use arch's libc only in the kernel/userspace (if not "prevented" by the chip) depending on their applications.
I disagree. Protected mode has been there for ages and nobody had this necessity, so most probably it will never be used.
Again, this will only increase the maintenance burden.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] gustavonihei commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "gustavonihei (via GitHub)" <gi...@apache.org>.
gustavonihei commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1182992872
##########
libs/libc/machine/Kconfig:
##########
@@ -137,6 +137,54 @@ config LIBC_ARCH_ELF_64BIT
default n
depends on LIBC_ARCH_ELF
+config LIBC_ARCH_PREVENT_KERNEL
+ bool
+ default n
+ ---help---
+ Prevent the architecture-specific libc from being used in the kernel.
+
+config LIBC_ARCH_PREVENT_USER
+ bool
+ default n
+ ---help---
+ Prevent the architecture-specific libc from being used in the
+ userspace.
+
+if BUILD_PROTECTED || BUILD_KERNEL
+
+choice LIBC_ARCH_USAGE
Review Comment:
Sorry, that's not what I meant. Instead I mean removing the `choice` completely.
```c
#if (defined(CONFIG_LIBC_ARCH_PREVENT_KERNEL) && defined(__KERNEL__)) || \
(defined(CONFIG_LIBC_ARCH_PREVENT_USER) && !defined(__KERNEL__))
```
> Furthermore, I think it's desirable to provide users a way to select whether to use arch's libc only in the kernel/userspace (if not "prevented" by the chip) depending on their applications.
I disagree. Protected mode has been there for ages and nobody had this necessity, so most probably it will never be used.
Again, this will only increase the maintenance burden.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] tmedicci commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "tmedicci (via GitHub)" <gi...@apache.org>.
tmedicci commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1182957086
##########
libs/libc/string/lib_memcpy.c:
##########
@@ -34,7 +34,9 @@
* Name: memcpy
****************************************************************************/
-#ifndef CONFIG_LIBC_ARCH_MEMCPY
+#if !defined(CONFIG_LIBC_ARCH_MEMCPY) || \
+ (defined(CONFIG_LIBC_ARCH_USER_ONLY) && defined(__KERNEL__)) || \
+ (defined(CONFIG_LIBC_ARCH_KERNEL_ONLY) && !defined(__KERNEL__))
Review Comment:
I added a guard for `nuttx/libs/libc/machine/<arch>` code (basically, the opposite of this) to avoid both the software version and the arch's being compiled. Could you please take a look at it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] gustavonihei commented on pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "gustavonihei (via GitHub)" <gi...@apache.org>.
gustavonihei commented on PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#issuecomment-1536239638
@tmedicci Please, rebase to resolve new conflicts.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] mu578 commented on pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "mu578 (via GitHub)" <gi...@apache.org>.
mu578 commented on PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#issuecomment-1551976783
@xiaoxiang781216 yes exactly but make all combinations explicit and tagged ; no `else maybe figure out`, that's my experience on maintaining large build systems ; be explicitly explicit everywhere: do not assume any enduser understanding.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] tmedicci commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "tmedicci (via GitHub)" <gi...@apache.org>.
tmedicci commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1181085999
##########
libs/libc/machine/Kconfig:
##########
@@ -137,6 +137,50 @@ config LIBC_ARCH_ELF_64BIT
default n
depends on LIBC_ARCH_ELF
+config LIBC_ARCH_ALLOW_KERNEL_ONLY
+ bool
+ default n
+
+config LIBC_ARCH_ALLOW_USER_ONLY
+ bool
+ default n
+
+if BUILD_PROTECTED || BUILD_KERNEL
+
+choice LIBC_ARCH_USAGE
+ prompt "libc architecture-specific usage"
+ default LIBC_ARCH_BOTH
+ depends on LIBC_ARCH_ALLOW_KERNEL_ONLY || LIBC_ARCH_ALLOW_USER_ONLY
+
+config LIBC_ARCH_BOTH
+ bool "Use architecture-specific on kernel and userspace"
+ depends on LIBC_ARCH_ALLOW_KERNEL_ONLY
+ depends on LIBC_ARCH_ALLOW_USER_ONLY
Review Comment:
Well, first of all, I tried to avoid the need of fixing other architectures and make this change very isolated. If none of the `LIBC_ARCH_ALLOW_KERNEL_ONLY` and `LIBC_ARCH_ALLOW_USER_ONLY` options are selected, the build system will use the arch-specific for both the kernel and the userspace (as it works currently). These two options, however, if selected by the chip, state that the chip allows using arch-specific's libc on kernel/userspace only (and that's why I used `_ALLOW_xxx_ONLY`). However, it doesn't select it. This is done by the `LIBC_ARCH_USAGE` choice.
That being said, if only one of the `LIBC_ARCH_ALLOW_xxx_ONLY` is selected, the choice will be selected automatically to use the arch-specific implementation on kernel/userspace only. There might exist, however, a situation that - for a specific chip - both options are valid: the chip allows arch-specific to be used only in the kernel, only in the userspace, or both in the kernel and the userspace (same effect of the current implementation). So, if the chip selects both the `LIBC_ARCH_ALLOW_KERNEL_ONLY` and `LIBC_ARCH_ALLOW_USER_ONLY` options, it's up to the user/`defconfig` to select which combination (using the `LIBC_ARCH_USAGE` choice) will be used and `LIBC_ARCH_BOTH` is necessary to be able to not select one of the `LIBC_ARCH_xxx_ONLY` choices automatically. You can think as a non-restrictive option.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#issuecomment-1550784058
@mu578 but all possible combination(4) are valid and useful.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] gustavonihei commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "gustavonihei (via GitHub)" <gi...@apache.org>.
gustavonihei commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1183024007
##########
libs/libc/machine/Kconfig:
##########
@@ -137,6 +137,54 @@ config LIBC_ARCH_ELF_64BIT
default n
depends on LIBC_ARCH_ELF
+config LIBC_ARCH_PREVENT_KERNEL
+ bool
+ default n
+ ---help---
+ Prevent the architecture-specific libc from being used in the kernel.
+
+config LIBC_ARCH_PREVENT_USER
+ bool
+ default n
+ ---help---
+ Prevent the architecture-specific libc from being used in the
+ userspace.
+
+if BUILD_PROTECTED || BUILD_KERNEL
+
+choice LIBC_ARCH_USAGE
Review Comment:
> Well, Espressif "has" this necessity.
> we only need this behavior when the Wi-Fi driver is being used.
This is not accurate.
The Wi-Fi driver is the only one that **so far** requires it because of the way it uses the libc functions. BTW, I faced the same issue with `memcpy` when implementing Protected Mode support for ESP32-S3.
https://github.com/apache/nuttx/blob/ea276b20ef9edede2c90a4e7fd708b45b4bc01d4/arch/xtensa/src/esp32s3/esp32s3_userspace.c#L548-L559
So it is desirable to avoid creating pitfalls for the unaware user.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1186610846
##########
libs/libc/machine/arm/armv7-a/gnu/arch_memchr.S:
##########
@@ -66,6 +66,10 @@
*
****************************************************************************/
+#include "libc.h"
+
+#if (LIBC_BUILD_FUNCTION == 0)
Review Comment:
ROM optimized version may have some limitation, but the assembly optimized version doesn't. So, it is not good to forbid the user select the assembly version. For example, esp32s3 could avoid lose the userspace performance lose in protected build by linking with: https://github.com/apache/nuttx/tree/master/libs/libc/machine/xtensa
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 merged pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 merged PR #9138:
URL: https://github.com/apache/nuttx/pull/9138
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] gustavonihei commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "gustavonihei (via GitHub)" <gi...@apache.org>.
gustavonihei commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1181138307
##########
libs/libc/machine/Kconfig:
##########
@@ -137,6 +137,50 @@ config LIBC_ARCH_ELF_64BIT
default n
depends on LIBC_ARCH_ELF
+config LIBC_ARCH_ALLOW_KERNEL_ONLY
+ bool
+ default n
+
+config LIBC_ARCH_ALLOW_USER_ONLY
+ bool
+ default n
+
+if BUILD_PROTECTED || BUILD_KERNEL
+
+choice LIBC_ARCH_USAGE
+ prompt "libc architecture-specific usage"
+ default LIBC_ARCH_BOTH
+ depends on LIBC_ARCH_ALLOW_KERNEL_ONLY || LIBC_ARCH_ALLOW_USER_ONLY
+
+config LIBC_ARCH_BOTH
+ bool "Use architecture-specific on kernel and userspace"
+ depends on LIBC_ARCH_ALLOW_KERNEL_ONLY
+ depends on LIBC_ARCH_ALLOW_USER_ONLY
Review Comment:
Another possibility is to invert the logic and instead of "allowing" the config could "prevent" the enabling on kernel/userspace.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] gustavonihei commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "gustavonihei (via GitHub)" <gi...@apache.org>.
gustavonihei commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1182968280
##########
libs/libc/machine/Kconfig:
##########
@@ -137,6 +137,54 @@ config LIBC_ARCH_ELF_64BIT
default n
depends on LIBC_ARCH_ELF
+config LIBC_ARCH_PREVENT_KERNEL
+ bool
+ default n
+ ---help---
+ Prevent the architecture-specific libc from being used in the kernel.
+
+config LIBC_ARCH_PREVENT_USER
+ bool
+ default n
+ ---help---
+ Prevent the architecture-specific libc from being used in the
+ userspace.
+
+if BUILD_PROTECTED || BUILD_KERNEL
+
+choice LIBC_ARCH_USAGE
Review Comment:
What's the point of exposing this as a user-selectable option?
Isn't the motivation for the proposed change to overcome a limitation of some chips? If so, this will only increase the maintenance burden.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] gustavonihei commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "gustavonihei (via GitHub)" <gi...@apache.org>.
gustavonihei commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1182971541
##########
libs/libc/string/lib_memcmp.c:
##########
@@ -30,7 +30,9 @@
* Public Functions
****************************************************************************/
-#ifndef CONFIG_LIBC_ARCH_MEMCMP
+#if !defined(CONFIG_LIBC_ARCH_MEMCMP) || \
Review Comment:
I agree, we should have something like:
```c
#if (!defined(CONFIG_LIBC_ARCH_USER_ONLY) && defined(__KERNEL__)) || \
(!defined(CONFIG_LIBC_ARCH_KERNEL_ONLY) && !defined(__KERNEL__)
# define LIBC_SHOULD_BUILD_FUNCTION 1
#endif
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] tmedicci commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "tmedicci (via GitHub)" <gi...@apache.org>.
tmedicci commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1195243859
##########
libs/libc/libc.h:
##########
@@ -53,6 +55,12 @@
#define LIB_BUFLEN_UNKNOWN INT_MAX
+#if defined(CONFIG_BUILD_FLAT) || \
+ ((!defined(CONFIG_LIBC_PREVENT_STRING_USER) && !defined(__KERNEL__)) || \
+ (!defined(CONFIG_LIBC_PREVENT_STRING_KERNEL) && defined(__KERNEL__)))
+# define LIBC_BUILD_FUNCTION
Review Comment:
done!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] tmedicci commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "tmedicci (via GitHub)" <gi...@apache.org>.
tmedicci commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1181087221
##########
libs/libc/machine/Kconfig:
##########
@@ -137,6 +137,50 @@ config LIBC_ARCH_ELF_64BIT
default n
depends on LIBC_ARCH_ELF
+config LIBC_ARCH_ALLOW_KERNEL_ONLY
+ bool
+ default n
+
+config LIBC_ARCH_ALLOW_USER_ONLY
+ bool
+ default n
Review Comment:
Please refer to https://github.com/apache/nuttx/pull/9138#discussion_r1181085999
Using `LIBC_ARCH_ALLOW_xxx` without the `ONLY` suffix reminds me of something that should be selected by a chip to state where the arch-specific implementations would be allowed. This would require 1) the default value being y or 2) these options being selected by all the chips (currently, all the chips "allow" arch's libc in both the kernel and the userspace).
The suffix `_ONLY` reminds me a different meaning, something like "ow, this arch has a different behavior than the usual. It allows the arch's libc to be used only by other kernel or by the userspace".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] tmedicci commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "tmedicci (via GitHub)" <gi...@apache.org>.
tmedicci commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1186727529
##########
libs/libc/machine/arm/armv7-a/gnu/arch_memchr.S:
##########
@@ -66,6 +66,10 @@
*
****************************************************************************/
+#include "libc.h"
+
+#if (LIBC_BUILD_FUNCTION == 0)
Review Comment:
I understand. Currently, we don't differentiate the ROM-defined implementation from the arch-specific (assembly version): both select `LIBC_ARCH_xxx` and this PR only prevent them to be used in the kernel or in the userspace. Specifically for the ESP32-S3, we need to use the ROM-defined version in the kernel (the assembly version won't work), but the ROM-defined can't be used in the userspace. So, we select `LIBC_ARCH_xxx` and `LIBC_ARCH_PREVENT_USER` to ensure that it won't build the software version in the kernel, but it'd be built for the userspace. If `<ARCH>_xxx` is selected, it will build the assembly version for the kernel and the software version for the userspace and it still wouldn't work for ESP32-S3. The opposite logic (`LIBC_ARCH_PREVENT_KERNEL`) would build the software version for the kernel.
I'd need to add a new flag to ensure that we won't build anything (software version or arch-specific assembly) to enable using the ROM defined (by the linker script) as expected.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1190262866
##########
libs/libc/machine/arm/armv7-a/gnu/arch_memchr.S:
##########
@@ -66,6 +66,10 @@
*
****************************************************************************/
+#include "libc.h"
+
+#if (LIBC_BUILD_FUNCTION == 0)
Review Comment:
> I'm sorry for the late response (I was working on something else).
>
> Well, I understand that `<ARCH>_xxx` needs to be selected to compile and link libc/machine's version. However, the opposite is not true: there is no Kconfig for selecting using the ROM-defined functions. ROM-defined functions are set on linker and they are used when there are no other symbols that define the same function.
> That being said, please note that this PR does not solve our problem. Consider the following scenarios:
>
> * Considering your suggestions about inverting the logic on libc/machine code (`#ifdef LIBC_BUILD_FUNCTION`):
>
> 1. No code would be built on flat build (software version or libc/machine);
> 2. On protected build, at least one of the `LIBC_ARCH_PREVENT_xxx` needs to be selected for the software version or the libc/machine being built;
> 3. If libc/machine code is selected (`CONFIG_ARMV8M_MEMCHR=y`, for instance), both the software version and libc/machine would be built for the other side of `LIBC_ARCH_PREVENT_xxx` (on protected mode);
>
> * Even after fixing the above issues, it still doesn't solve our problems: ESP32-S3 requires the ROM-defined `memcpy` to be used on flat build or in the kernel (on protected build) when Wi-Fi is enabled.
>
> 1. If we want the optimized assembly version to be available for the userspace, we'd need to ensure it's prevented in the kernel, as well as the software version;
>
> That being said, I propose that we create a new set of rules like `LIBC_ARCH_ROM_PREVENT_xxx` and `LIBC_ARCH_MACHINE_PREVENT_xxx` to differentiate the usage. Considering the ESP32-S3, for instance, we'd select `LIBC_ARCH_ROM_PREVENT_USER` and `LIBC_ARCH_MACHINE_PREVENT_KERNEL` to make sure ROM-defined is not used in the userspace AND the libc/machine version is not being used in the kernel (at least when Wi-Fi is enabled).
>
> What do you think about it, @xiaoxiang781216 and @gustavonihei ?
look like workable, but it's enough to add something like LIBC_PREVENT_KERNEL and LIBC_PREVENT_USER which mean disable libc version(either c or assemble). Here is some possible combination:
1. default use libc version for both user and kernel
2. enable assemble version(e.g. CONFIG_ARMV8M_MEMCHR=y) for user and kernel
3. CONFIG_LIBC_PREVENT_KERNEL c version for user and rom for kernel
4. CONFIG_ARMV8M_MEMCHR and CONFIG_LIBC_PREVENT_KERNEL, assemble version for user and rom for kernel
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#issuecomment-1553051586
I am fine with both approach:).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] gustavonihei commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "gustavonihei (via GitHub)" <gi...@apache.org>.
gustavonihei commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1183024007
##########
libs/libc/machine/Kconfig:
##########
@@ -137,6 +137,54 @@ config LIBC_ARCH_ELF_64BIT
default n
depends on LIBC_ARCH_ELF
+config LIBC_ARCH_PREVENT_KERNEL
+ bool
+ default n
+ ---help---
+ Prevent the architecture-specific libc from being used in the kernel.
+
+config LIBC_ARCH_PREVENT_USER
+ bool
+ default n
+ ---help---
+ Prevent the architecture-specific libc from being used in the
+ userspace.
+
+if BUILD_PROTECTED || BUILD_KERNEL
+
+choice LIBC_ARCH_USAGE
Review Comment:
> Well, Espressif "has" this necessity.
> we only need this behavior when the Wi-Fi driver is being used.
This is not accurate.
The Wi-Fi driver is the only one that **so far** requires it because of the way it uses the libc functions. BTW, I faced the same issue with `memcpy` when implementing Protected Mode support for ESP32-S3.
https://github.com/apache/nuttx/blob/ea276b20ef9edede2c90a4e7fd708b45b4bc01d4/arch/xtensa/src/esp32s3/esp32s3_userspace.c#L548-L559
Using the NuttX `memcpy` would result in an exception while the `memcpy` from the Internal ROM would execute as expected.
So it is desirable to avoid creating pitfalls for the unaware user.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] gustavonihei commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "gustavonihei (via GitHub)" <gi...@apache.org>.
gustavonihei commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1180788621
##########
libs/libc/machine/Kconfig:
##########
@@ -137,6 +137,50 @@ config LIBC_ARCH_ELF_64BIT
default n
depends on LIBC_ARCH_ELF
+config LIBC_ARCH_ALLOW_KERNEL_ONLY
+ bool
+ default n
+
+config LIBC_ARCH_ALLOW_USER_ONLY
+ bool
+ default n
+
+if BUILD_PROTECTED || BUILD_KERNEL
+
+choice LIBC_ARCH_USAGE
+ prompt "libc architecture-specific usage"
+ default LIBC_ARCH_BOTH
+ depends on LIBC_ARCH_ALLOW_KERNEL_ONLY || LIBC_ARCH_ALLOW_USER_ONLY
+
+config LIBC_ARCH_BOTH
+ bool "Use architecture-specific on kernel and userspace"
+ depends on LIBC_ARCH_ALLOW_KERNEL_ONLY
+ depends on LIBC_ARCH_ALLOW_USER_ONLY
Review Comment:
Unused option, what's the intended use for it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] tmedicci commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "tmedicci (via GitHub)" <gi...@apache.org>.
tmedicci commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1193996785
##########
libs/libc/machine/arm/armv7-a/gnu/arch_memchr.S:
##########
@@ -66,6 +66,10 @@
*
****************************************************************************/
+#include "libc.h"
+
+#if (LIBC_BUILD_FUNCTION == 0)
Review Comment:
It's ready, @xiaoxiang781216 ! The solution you proposed fits perfectly. Could you please take a look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] tmedicci commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "tmedicci (via GitHub)" <gi...@apache.org>.
tmedicci commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1181138092
##########
libs/libc/machine/Kconfig:
##########
@@ -137,6 +137,50 @@ config LIBC_ARCH_ELF_64BIT
default n
depends on LIBC_ARCH_ELF
+config LIBC_ARCH_ALLOW_KERNEL_ONLY
+ bool
+ default n
+
+config LIBC_ARCH_ALLOW_USER_ONLY
+ bool
+ default n
+
+if BUILD_PROTECTED || BUILD_KERNEL
+
+choice LIBC_ARCH_USAGE
+ prompt "libc architecture-specific usage"
+ default LIBC_ARCH_BOTH
+ depends on LIBC_ARCH_ALLOW_KERNEL_ONLY || LIBC_ARCH_ALLOW_USER_ONLY
+
+config LIBC_ARCH_BOTH
+ bool "Use architecture-specific on kernel and userspace"
+ depends on LIBC_ARCH_ALLOW_KERNEL_ONLY
+ depends on LIBC_ARCH_ALLOW_USER_ONLY
Review Comment:
> If the PR instead implemented the idea proposed in https://github.com/apache/nuttx/pull/9138#discussion_r1181087221, it would result in a much cleaner and more straightforward implementation:
I didn't get which proposed idea should be implemented. 1) or 2)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] tmedicci commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "tmedicci (via GitHub)" <gi...@apache.org>.
tmedicci commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1186461126
##########
libs/libc/machine/arm/armv7-a/gnu/arch_memchr.S:
##########
@@ -66,6 +66,10 @@
*
****************************************************************************/
+#include "libc.h"
+
+#if (LIBC_BUILD_FUNCTION == 0)
Review Comment:
> Note: LIBC_ARCH_PREVENT_xxx should prevent only the rom function, but not assembler optimized version.
I didn't get it. Currently, it prevents any libc implementation other than NuttX's to be used. So, if `LIBC_ARCH_PREVENT_xxx` is selected, NuttX's C software version will be used. Please note that `<ARCH>_MEMCHR` selects `LIBC_ARCH_MEMCHR` automatically, so the guards in `libs/libc/machine/<arch>` files avoid both being built.
That is the intended behavior: ROM-defined and optimized versions are treated "equally" on this PR. The idea is to prevent them to be used in kernel/userspace (and build NuttX's version, instead).
Do you mean to change this behavior?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] tmedicci commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "tmedicci (via GitHub)" <gi...@apache.org>.
tmedicci commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1181085999
##########
libs/libc/machine/Kconfig:
##########
@@ -137,6 +137,50 @@ config LIBC_ARCH_ELF_64BIT
default n
depends on LIBC_ARCH_ELF
+config LIBC_ARCH_ALLOW_KERNEL_ONLY
+ bool
+ default n
+
+config LIBC_ARCH_ALLOW_USER_ONLY
+ bool
+ default n
+
+if BUILD_PROTECTED || BUILD_KERNEL
+
+choice LIBC_ARCH_USAGE
+ prompt "libc architecture-specific usage"
+ default LIBC_ARCH_BOTH
+ depends on LIBC_ARCH_ALLOW_KERNEL_ONLY || LIBC_ARCH_ALLOW_USER_ONLY
+
+config LIBC_ARCH_BOTH
+ bool "Use architecture-specific on kernel and userspace"
+ depends on LIBC_ARCH_ALLOW_KERNEL_ONLY
+ depends on LIBC_ARCH_ALLOW_USER_ONLY
Review Comment:
Well, the considerations of this comment are the same of the next one's (https://github.com/apache/nuttx/pull/9138#pullrequestreview-1406556561), so I'll answer here, alright?
Well, first of all, I tried to avoid the need of fixing other architectures and make this change very isolated. If none of the `LIBC_ARCH_ALLOW_KERNEL_ONLY` and `LIBC_ARCH_ALLOW_USER_ONLY` options are selected, the build system will use the arch-specific for both the kernel and the userspace (as it works currently). These two options, however, if selected by the chip, state that the chip allows using arch-specific's libc on kernel/userspace only (and that's why I used `_ALLOW_xxx_ONLY`). However, it doesn't select it. This is done by the `LIBC_ARCH_USAGE` choice.
That being said, if only one of the `LIBC_ARCH_ALLOW_xxx_ONLY` is selected, the choice will be selected automatically to use the arch-specific implementation on kernel/userspace only. There might exist, however, a situation that - for a specific chip - both options are valid: the chip allows arch-specific to be used only in the kernel, only in the userspace, or both in the kernel and the userspace (same effect of the current implementation). So, if the chip selects both the `LIBC_ARCH_ALLOW_KERNEL_ONLY` and `LIBC_ARCH_ALLOW_USER_ONLY` options, it's up to the user/`defconfig` to select which combination (using the `LIBC_ARCH_USAGE` choice) will be used and `LIBC_ARCH_BOTH` is necessary to be able to not select one of the `LIBC_ARCH_xxx_ONLY` choices automatically. You can think as a non-restrictive option.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] tmedicci commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "tmedicci (via GitHub)" <gi...@apache.org>.
tmedicci commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1181138779
##########
libs/libc/machine/Kconfig:
##########
@@ -137,6 +137,50 @@ config LIBC_ARCH_ELF_64BIT
default n
depends on LIBC_ARCH_ELF
+config LIBC_ARCH_ALLOW_KERNEL_ONLY
+ bool
+ default n
+
+config LIBC_ARCH_ALLOW_USER_ONLY
+ bool
+ default n
+
+if BUILD_PROTECTED || BUILD_KERNEL
+
+choice LIBC_ARCH_USAGE
+ prompt "libc architecture-specific usage"
+ default LIBC_ARCH_BOTH
+ depends on LIBC_ARCH_ALLOW_KERNEL_ONLY || LIBC_ARCH_ALLOW_USER_ONLY
+
+config LIBC_ARCH_BOTH
+ bool "Use architecture-specific on kernel and userspace"
+ depends on LIBC_ARCH_ALLOW_KERNEL_ONLY
+ depends on LIBC_ARCH_ALLOW_USER_ONLY
Review Comment:
That could work! It avoids needing to update all the chips: something like `LIBC_ARCH_FORBID_xxx` would fit... I'll try to update the proposal on Tuesday (after the holidays)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] tmedicci commented on pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "tmedicci (via GitHub)" <gi...@apache.org>.
tmedicci commented on PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#issuecomment-1536237327
@xiaoxiang781216 and @pkarashchenko , could you please take a look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] tmedicci commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "tmedicci (via GitHub)" <gi...@apache.org>.
tmedicci commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1186461126
##########
libs/libc/machine/arm/armv7-a/gnu/arch_memchr.S:
##########
@@ -66,6 +66,10 @@
*
****************************************************************************/
+#include "libc.h"
+
+#if (LIBC_BUILD_FUNCTION == 0)
Review Comment:
> Note: LIBC_ARCH_PREVENT_xxx should prevent only the rom function, but not assembler optimized version.
I didn't get it. Currently, it prevents any libc implementation other than NuttX's to be used. So, if `LIBC_ARCH_PREVENT_xxx` is selected, NuttX's C software version will be used. Please note that `<ARCH>_MEMCHR` selects `LIBC_ARCH_MEMCHR` automatically, so the guards in `libs/libc/machine/<arch>` files avoid both being built.
That is the intended behavior: ROM-defined and optimized versions are treated "equally" on this PR.
Do you mean to change this behavior?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1186422857
##########
libs/libc/libc.h:
##########
@@ -53,6 +55,14 @@
#define LIB_BUFLEN_UNKNOWN INT_MAX
+#if defined(CONFIG_BUILD_PROTECTED) && \
+ ((defined(CONFIG_LIBC_ARCH_PREVENT_USER) && !defined(__KERNEL__)) || \
+ (defined(CONFIG_LIBC_ARCH_PREVENT_KERNEL) && defined(__KERNEL__)))
+# define LIBC_BUILD_FUNCTION 1
+#else
+# define LIBC_BUILD_FUNCTION 0
Review Comment:
remvove, let's check if defined(LIBC_BUILD_FUNCTION)?
##########
libs/libc/machine/arm/armv7-a/gnu/arch_memchr.S:
##########
@@ -66,6 +66,10 @@
*
****************************************************************************/
+#include "libc.h"
+
+#if (LIBC_BUILD_FUNCTION == 0)
Review Comment:
#ifdef LIBC_BUILD_FUNCTION
and other similar place.
Note: LIBC_ARCH_PREVENT_xxx should prevent only the rom function, but not assembler optimized version.
##########
libs/libc/libc.h:
##########
@@ -53,6 +55,14 @@
#define LIB_BUFLEN_UNKNOWN INT_MAX
+#if defined(CONFIG_BUILD_PROTECTED) && \
Review Comment:
#if !defined(CONFIG_BUILD_FLAT)
##########
libs/libc/string/lib_memcpy.c:
##########
@@ -34,7 +36,7 @@
* Name: memcpy
****************************************************************************/
-#ifndef CONFIG_LIBC_ARCH_MEMCPY
+#if !defined(CONFIG_LIBC_ARCH_MEMCPY) || (LIBC_BUILD_FUNCTION)
Review Comment:
#if defined(LIBC_BUILD_FUNCTION) && !defined(CONFIG_LIBC_ARCH_MEMCPY)
and other similar place
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1186610846
##########
libs/libc/machine/arm/armv7-a/gnu/arch_memchr.S:
##########
@@ -66,6 +66,10 @@
*
****************************************************************************/
+#include "libc.h"
+
+#if (LIBC_BUILD_FUNCTION == 0)
Review Comment:
ROM optimized version may have some limitation, but the assembly optimized version doesn't. So, it is not good to forbid the user select the assembly version. For example, esp32s3 could avoid lose the userspace performance lose in protected build by linking with: https://github.com/apache/nuttx/tree/master/libs/libc/machine/xtensa.
These function are very critical for graphics/media application.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1194126157
##########
libs/libc/machine/Kconfig:
##########
@@ -137,6 +137,30 @@ config LIBC_ARCH_ELF_64BIT
default n
depends on LIBC_ARCH_ELF
+config LIBC_PREVENT_KERNEL
Review Comment:
should we change to LIBC_PREVENT_STRING_xxx to limit the scope of exclusion?
##########
libs/libc/machine/arm/armv7-a/gnu/arch_memchr.S:
##########
@@ -66,6 +66,10 @@
*
****************************************************************************/
+#include "libc.h"
+
+#ifdef LIBC_BUILD_FUNCTION
Review Comment:
```suggestion
#ifdef LIBC_BUILD_STRING_FUNCTION
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] tmedicci commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "tmedicci (via GitHub)" <gi...@apache.org>.
tmedicci commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1186461126
##########
libs/libc/machine/arm/armv7-a/gnu/arch_memchr.S:
##########
@@ -66,6 +66,10 @@
*
****************************************************************************/
+#include "libc.h"
+
+#if (LIBC_BUILD_FUNCTION == 0)
Review Comment:
> Note: LIBC_ARCH_PREVENT_xxx should prevent only the rom function, but not assembler optimized version.
I didn't get it. Currently, it prevents any libc implementation other than NuttX's to be used. So, if `LIBC_ARCH_PREVENT_xxx` is selected, NuttX's C software version will be used. Please note that `<ARCH>_MEMCHR` selects `LIBC_ARCH_MEMCHR` automatically, so the guards in `libs/libc/machine/<arch>` files avoid both being built if `<ARCH>_MEMCHR` is selected.
That is the intended behavior: ROM-defined and optimized versions are treated "equally" on this PR. The idea is to prevent them to be used in kernel/userspace (and build NuttX's version, instead).
Do you mean to change this behavior?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] gustavonihei commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "gustavonihei (via GitHub)" <gi...@apache.org>.
gustavonihei commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1180789566
##########
libs/libc/machine/Kconfig:
##########
@@ -137,6 +137,50 @@ config LIBC_ARCH_ELF_64BIT
default n
depends on LIBC_ARCH_ELF
+config LIBC_ARCH_ALLOW_KERNEL_ONLY
+ bool
+ default n
+
+config LIBC_ARCH_ALLOW_USER_ONLY
+ bool
+ default n
Review Comment:
```suggestion
config LIBC_ARCH_ALLOW_KERNEL
bool
default n
config LIBC_ARCH_ALLOW_USER
bool
default n
```
The existence of a `LIBC_ARCH_BOTH` option contradicts the suffix `_ONLY` for the above configs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] tmedicci commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "tmedicci (via GitHub)" <gi...@apache.org>.
tmedicci commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1183008392
##########
libs/libc/machine/Kconfig:
##########
@@ -137,6 +137,54 @@ config LIBC_ARCH_ELF_64BIT
default n
depends on LIBC_ARCH_ELF
+config LIBC_ARCH_PREVENT_KERNEL
+ bool
+ default n
+ ---help---
+ Prevent the architecture-specific libc from being used in the kernel.
+
+config LIBC_ARCH_PREVENT_USER
+ bool
+ default n
+ ---help---
+ Prevent the architecture-specific libc from being used in the
+ userspace.
+
+if BUILD_PROTECTED || BUILD_KERNEL
+
+choice LIBC_ARCH_USAGE
Review Comment:
> Sorry, that's not what I meant. Instead I mean removing the `choice` completely and use `LIBC_ARCH_PREVENT_[KERNEL/USER]` directly.
>
> ```c
> #if (defined(CONFIG_LIBC_ARCH_PREVENT_KERNEL) && defined(__KERNEL__)) || \
> (defined(CONFIG_LIBC_ARCH_PREVENT_USER) && !defined(__KERNEL__))
> ```
>
> > Furthermore, I think it's desirable to provide users a way to select whether to use arch's libc only in the kernel/userspace (if not "prevented" by the chip) depending on their applications.
>
> I disagree. Protected mode has been there for ages and nobody had this necessity, so most probably it will never be used. Again, this will only increase the maintenance burden.
Well, Espressif "has" this necessity. I chose to select `CONFIG_LIBC_ARCH_PREVENT_USER` at chip's config, but strictly speaking, we only need this behavior when the Wi-Fi driver is being used. Thus, alternatively, `CONFIG_LIBC_ARCH_PREVENT_USER` could be only selected by the Wi-Fi driver or, otherwise, the choice be defined by a `defconfig` (and the user could tweak this choice accordingly).
Particularly, I disagree with the maintenance burden: I think it brings flexibility and I prefer to keep it.
@xiaoxiang781216 suggested a similar approach. What do you think about it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] tmedicci commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "tmedicci (via GitHub)" <gi...@apache.org>.
tmedicci commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1183009766
##########
libs/libc/string/lib_memcmp.c:
##########
@@ -30,7 +30,9 @@
* Public Functions
****************************************************************************/
-#ifndef CONFIG_LIBC_ARCH_MEMCMP
+#if !defined(CONFIG_LIBC_ARCH_MEMCMP) || \
Review Comment:
> I agree, we should have something like:
>
> ```c
> #if (!defined(CONFIG_LIBC_ARCH_USER_ONLY) && defined(__KERNEL__)) || \
> (!defined(CONFIG_LIBC_ARCH_KERNEL_ONLY) && !defined(__KERNEL__)
> # define LIBC_SHOULD_BUILD_FUNCTION 1
> #endif
> ```
That is the result for building arch-specific libc. Do you think we'd need a macro for both the software version and the arch's? As a drawback, we'd need to include libc.h on both sources (software and arch), too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] tmedicci commented on pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "tmedicci (via GitHub)" <gi...@apache.org>.
tmedicci commented on PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#issuecomment-1533498348
> LGTM. @tmedicci This PR could also extend to ESP32, basically the same changes applied to ESP32-S3. You may validate it via a successful `ostest` with `esp32-devkitc:knsh` defconfig.
Alright. I'll update it with a new commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] tmedicci commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "tmedicci (via GitHub)" <gi...@apache.org>.
tmedicci commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1189109905
##########
libs/libc/machine/arm/armv7-a/gnu/arch_memchr.S:
##########
@@ -66,6 +66,10 @@
*
****************************************************************************/
+#include "libc.h"
+
+#if (LIBC_BUILD_FUNCTION == 0)
Review Comment:
I'm sorry for the late response (I was working on something else).
Well, I understand that `<ARCH>_xxx` needs to be selected to compile and link libc/machine's version. However, the opposite is not true: there is no Kconfig for selecting using the ROM-defined functions. ROM-defined functions are set on linker and they are used when there are no other symbols that define the same function. That being said, please note that this PR does not solve our problem. Consider the following scenarios:
- Considering your suggestions about inverting the logic on libc/machine code (`#ifdef LIBC_BUILD_FUNCTION`):
1. No code would be built on flat build (software version or libc/machine);
2. On protected build, at least one of the `LIBC_ARCH_PREVENT_xxx` needs to be selected for the software version or the libc/machine being built;
3. If libc/machine code is selected (`CONFIG_ARMV8M_MEMCHR=y`, for instance), both the software version and libc/machine would be built for the other side of `LIBC_ARCH_PREVENT_xxx` (on protected mode);
- Even after fixing the above issues, it still doesn't solve our problems: ESP32-S3 requires the ROM-defined `memcpy` to be used on flat build or in the kernel (on protected build) when Wi-Fi is enabled.
1. If we want the optimized assembly version to be available for the userspace, we'd need to ensure it's prevented in the kernel, as well as the software version;
That being said, I propose that we create a new set of rules like `LIBC_ARCH_ROM_PREVENT_xxx` and `LIBC_ARCH_MACHINE_PREVENT_xxx` to differentiate the usage. Considering the ESP32-S3, for instance, we'd select `LIBC_ARCH_ROM_PREVENT_USER` and `LIBC_ARCH_MACHINE_PREVENT_KERNEL` to make sure ROM-defined is not used in the userspace AND the libc/machine version is not being used in the kernel (at least when Wi-Fi is enabled).
What do you think about it, @xiaoxiang781216 and @gustavonihei ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] gustavonihei commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "gustavonihei (via GitHub)" <gi...@apache.org>.
gustavonihei commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1181138235
##########
libs/libc/machine/Kconfig:
##########
@@ -137,6 +137,50 @@ config LIBC_ARCH_ELF_64BIT
default n
depends on LIBC_ARCH_ELF
+config LIBC_ARCH_ALLOW_KERNEL_ONLY
+ bool
+ default n
+
+config LIBC_ARCH_ALLOW_USER_ONLY
+ bool
+ default n
+
+if BUILD_PROTECTED || BUILD_KERNEL
+
+choice LIBC_ARCH_USAGE
+ prompt "libc architecture-specific usage"
+ default LIBC_ARCH_BOTH
+ depends on LIBC_ARCH_ALLOW_KERNEL_ONLY || LIBC_ARCH_ALLOW_USER_ONLY
+
+config LIBC_ARCH_BOTH
+ bool "Use architecture-specific on kernel and userspace"
+ depends on LIBC_ARCH_ALLOW_KERNEL_ONLY
+ depends on LIBC_ARCH_ALLOW_USER_ONLY
Review Comment:
> > If the PR instead implemented the idea proposed in https://github.com/apache/nuttx/pull/9138#discussion_r1181087221, it would result in a much cleaner and more straightforward implementation:
>
>
>
> I didn't get which proposed idea should be implemented. 1) or 2)?
Sorry, I meant the option 2.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] tmedicci commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "tmedicci (via GitHub)" <gi...@apache.org>.
tmedicci commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1181138305
##########
libs/libc/machine/Kconfig:
##########
@@ -137,6 +137,50 @@ config LIBC_ARCH_ELF_64BIT
default n
depends on LIBC_ARCH_ELF
+config LIBC_ARCH_ALLOW_KERNEL_ONLY
+ bool
+ default n
+
+config LIBC_ARCH_ALLOW_USER_ONLY
+ bool
+ default n
Review Comment:
I agree that `_ONLY` implies XOR logic. But there is a level between `_ALLOW_` and the choice itself: you can allow something that is XOR-ed when you have multiple choices.
However, if It isn't that clear, let's find a more intuitive way of doing it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] gustavonihei commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "gustavonihei (via GitHub)" <gi...@apache.org>.
gustavonihei commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1181136955
##########
libs/libc/machine/Kconfig:
##########
@@ -137,6 +137,50 @@ config LIBC_ARCH_ELF_64BIT
default n
depends on LIBC_ARCH_ELF
+config LIBC_ARCH_ALLOW_KERNEL_ONLY
+ bool
+ default n
+
+config LIBC_ARCH_ALLOW_USER_ONLY
+ bool
+ default n
Review Comment:
> The suffix _ONLY reminds me a different meaning, something like "ow, this arch has a different behavior than the usual. It allows the arch's libc to be used only by other kernel or by the userspace".
It is still contradictory. Having a `_ONLY` prefix implies a XOR logic.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1194514372
##########
libs/libc/libc.h:
##########
@@ -53,6 +55,12 @@
#define LIB_BUFLEN_UNKNOWN INT_MAX
+#if defined(CONFIG_BUILD_FLAT) || \
+ ((!defined(CONFIG_LIBC_PREVENT_STRING_USER) && !defined(__KERNEL__)) || \
+ (!defined(CONFIG_LIBC_PREVENT_STRING_KERNEL) && defined(__KERNEL__)))
+# define LIBC_BUILD_FUNCTION
Review Comment:
change to LIBC_BUILD_STRING?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] tmedicci commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "tmedicci (via GitHub)" <gi...@apache.org>.
tmedicci commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1182952968
##########
libs/libc/machine/Kconfig:
##########
@@ -137,6 +137,50 @@ config LIBC_ARCH_ELF_64BIT
default n
depends on LIBC_ARCH_ELF
+config LIBC_ARCH_ALLOW_KERNEL_ONLY
+ bool
+ default n
+
+config LIBC_ARCH_ALLOW_USER_ONLY
+ bool
+ default n
+
+if BUILD_PROTECTED || BUILD_KERNEL
+
+choice LIBC_ARCH_USAGE
+ prompt "libc architecture-specific usage"
+ default LIBC_ARCH_BOTH
+ depends on LIBC_ARCH_ALLOW_KERNEL_ONLY || LIBC_ARCH_ALLOW_USER_ONLY
+
+config LIBC_ARCH_BOTH
+ bool "Use architecture-specific on kernel and userspace"
+ depends on LIBC_ARCH_ALLOW_KERNEL_ONLY
+ depends on LIBC_ARCH_ALLOW_USER_ONLY
Review Comment:
I've updated the MR with your suggestions. Could you please take a look at it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] tmedicci commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "tmedicci (via GitHub)" <gi...@apache.org>.
tmedicci commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1184077894
##########
libs/libc/machine/Kconfig:
##########
@@ -137,6 +137,54 @@ config LIBC_ARCH_ELF_64BIT
default n
depends on LIBC_ARCH_ELF
+config LIBC_ARCH_PREVENT_KERNEL
+ bool
+ default n
+ ---help---
+ Prevent the architecture-specific libc from being used in the kernel.
+
+config LIBC_ARCH_PREVENT_USER
+ bool
+ default n
+ ---help---
+ Prevent the architecture-specific libc from being used in the
+ userspace.
+
+if BUILD_PROTECTED || BUILD_KERNEL
+
+choice LIBC_ARCH_USAGE
Review Comment:
Done!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] tmedicci commented on pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "tmedicci (via GitHub)" <gi...@apache.org>.
tmedicci commented on PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#issuecomment-1533535429
> > LGTM. @tmedicci This PR could also extend to ESP32, basically the same changes applied to ESP32-S3. You may validate it via a successful `ostest` with `esp32-devkitc:knsh` defconfig.
>
> Alright. I'll update it with a new commit.
Done! I split the commit to apply the changes for ESP32 and ESP32-S3 in a separate commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] tmedicci commented on pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "tmedicci (via GitHub)" <gi...@apache.org>.
tmedicci commented on PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#issuecomment-1536254585
> @tmedicci Please, rebase to resolve new conflicts.
done!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] gustavonihei commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "gustavonihei (via GitHub)" <gi...@apache.org>.
gustavonihei commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1182992872
##########
libs/libc/machine/Kconfig:
##########
@@ -137,6 +137,54 @@ config LIBC_ARCH_ELF_64BIT
default n
depends on LIBC_ARCH_ELF
+config LIBC_ARCH_PREVENT_KERNEL
+ bool
+ default n
+ ---help---
+ Prevent the architecture-specific libc from being used in the kernel.
+
+config LIBC_ARCH_PREVENT_USER
+ bool
+ default n
+ ---help---
+ Prevent the architecture-specific libc from being used in the
+ userspace.
+
+if BUILD_PROTECTED || BUILD_KERNEL
+
+choice LIBC_ARCH_USAGE
Review Comment:
Sorry, that's not what I meant. Instead I mean removing the choice completely.
```c
#if (defined(CONFIG_LIBC_ARCH_PREVENT_KERNEL) && defined(__KERNEL__)) || \
(defined(CONFIG_LIBC_ARCH_PREVENT_USER) && !defined(__KERNEL__))
```
> Furthermore, I think it's desirable to provide users a way to select whether to use arch's libc only in the kernel/userspace (if not "prevented" by the chip) depending on their applications.
I disagree. Protected mode has been there for ages and nobody had this necessity, so most probably it will never be used.
Again, this will only increase the maintenance burden.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] tmedicci commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "tmedicci (via GitHub)" <gi...@apache.org>.
tmedicci commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1182982889
##########
libs/libc/machine/Kconfig:
##########
@@ -137,6 +137,54 @@ config LIBC_ARCH_ELF_64BIT
default n
depends on LIBC_ARCH_ELF
+config LIBC_ARCH_PREVENT_KERNEL
+ bool
+ default n
+ ---help---
+ Prevent the architecture-specific libc from being used in the kernel.
+
+config LIBC_ARCH_PREVENT_USER
+ bool
+ default n
+ ---help---
+ Prevent the architecture-specific libc from being used in the
+ userspace.
+
+if BUILD_PROTECTED || BUILD_KERNEL
+
+choice LIBC_ARCH_USAGE
Review Comment:
Choices can't be hidden. If we try to:
```
warning: <choice LIBC_ARCH_USAGE> (defined at libs/libc/machine/Kconfig:155) defined without a prompt
```
Furthermore, I think it's desirable to provide users a way to select whether to use arch's libc only in the kernel/userspace (if not "prevented" by the chip) depending on their applications.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] tmedicci commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "tmedicci (via GitHub)" <gi...@apache.org>.
tmedicci commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1184077178
##########
libs/libc/string/lib_memcmp.c:
##########
@@ -30,7 +30,9 @@
* Public Functions
****************************************************************************/
-#ifndef CONFIG_LIBC_ARCH_MEMCMP
+#if !defined(CONFIG_LIBC_ARCH_MEMCMP) || \
Review Comment:
Done!
##########
libs/libc/string/lib_memcpy.c:
##########
@@ -34,7 +34,9 @@
* Name: memcpy
****************************************************************************/
-#ifndef CONFIG_LIBC_ARCH_MEMCPY
+#if !defined(CONFIG_LIBC_ARCH_MEMCPY) || \
+ (defined(CONFIG_LIBC_ARCH_USER_ONLY) && defined(__KERNEL__)) || \
+ (defined(CONFIG_LIBC_ARCH_KERNEL_ONLY) && !defined(__KERNEL__))
Review Comment:
Done!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1181157853
##########
libs/libc/string/lib_memcmp.c:
##########
@@ -30,7 +30,9 @@
* Public Functions
****************************************************************************/
-#ifndef CONFIG_LIBC_ARCH_MEMCMP
+#if !defined(CONFIG_LIBC_ARCH_MEMCMP) || \
Review Comment:
can we add a macro in libc/libc.h to avoid the dupliated?
##########
libs/libc/string/lib_memcpy.c:
##########
@@ -34,7 +34,9 @@
* Name: memcpy
****************************************************************************/
-#ifndef CONFIG_LIBC_ARCH_MEMCPY
+#if !defined(CONFIG_LIBC_ARCH_MEMCPY) || \
+ (defined(CONFIG_LIBC_ARCH_USER_ONLY) && defined(__KERNEL__)) || \
+ (defined(CONFIG_LIBC_ARCH_KERNEL_ONLY) && !defined(__KERNEL__))
Review Comment:
should we add the similar guard to the optimization version under libc/machince?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] gustavonihei commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "gustavonihei (via GitHub)" <gi...@apache.org>.
gustavonihei commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1181136813
##########
libs/libc/machine/Kconfig:
##########
@@ -137,6 +137,50 @@ config LIBC_ARCH_ELF_64BIT
default n
depends on LIBC_ARCH_ELF
+config LIBC_ARCH_ALLOW_KERNEL_ONLY
+ bool
+ default n
+
+config LIBC_ARCH_ALLOW_USER_ONLY
+ bool
+ default n
+
+if BUILD_PROTECTED || BUILD_KERNEL
+
+choice LIBC_ARCH_USAGE
+ prompt "libc architecture-specific usage"
+ default LIBC_ARCH_BOTH
+ depends on LIBC_ARCH_ALLOW_KERNEL_ONLY || LIBC_ARCH_ALLOW_USER_ONLY
+
+config LIBC_ARCH_BOTH
+ bool "Use architecture-specific on kernel and userspace"
+ depends on LIBC_ARCH_ALLOW_KERNEL_ONLY
+ depends on LIBC_ARCH_ALLOW_USER_ONLY
Review Comment:
> Well, first of all, I tried to avoid the need of fixing other architectures and make this change very isolated.
This fear of changing common code is compromising the quality of the proposed change by making it counterintuitive.
If the PR instead implemented the idea proposed in https://github.com/apache/nuttx/pull/9138#discussion_r1181087221, it would result in a much cleaner and more straightforward implementation:
> 1) the default value being y or 2) these options being selected by all the chips (currently, all the chips "allow" arch's libc in both the kernel and the userspace).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] gustavonihei commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "gustavonihei (via GitHub)" <gi...@apache.org>.
gustavonihei commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1181136955
##########
libs/libc/machine/Kconfig:
##########
@@ -137,6 +137,50 @@ config LIBC_ARCH_ELF_64BIT
default n
depends on LIBC_ARCH_ELF
+config LIBC_ARCH_ALLOW_KERNEL_ONLY
+ bool
+ default n
+
+config LIBC_ARCH_ALLOW_USER_ONLY
+ bool
+ default n
Review Comment:
> The suffix _ONLY reminds me a different meaning, something like "ow, this arch has a different behavior than the usual. It allows the arch's libc to be used only by other kernel or by the userspace".
It is still contradictory. Having a `_ONLY` suffix implies a XOR logic.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] tmedicci commented on a diff in pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "tmedicci (via GitHub)" <gi...@apache.org>.
tmedicci commented on code in PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#discussion_r1194304668
##########
libs/libc/machine/Kconfig:
##########
@@ -137,6 +137,30 @@ config LIBC_ARCH_ELF_64BIT
default n
depends on LIBC_ARCH_ELF
+config LIBC_PREVENT_KERNEL
Review Comment:
done!
##########
libs/libc/machine/arm/armv7-a/gnu/arch_memchr.S:
##########
@@ -66,6 +66,10 @@
*
****************************************************************************/
+#include "libc.h"
+
+#ifdef LIBC_BUILD_FUNCTION
Review Comment:
done!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] mu578 commented on pull request #9138: libc/string: select arch's libc for kernel/userspace optionally
Posted by "mu578 (via GitHub)" <gi...@apache.org>.
mu578 commented on PR #9138:
URL: https://github.com/apache/nuttx/pull/9138#issuecomment-1549817140
Maybe you should enforce an explicit LIBC_ARCH_BUILD_SHARED instead of defaulting.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org