You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "pkarashchenko (via GitHub)" <gi...@apache.org> on 2023/08/07 22:47:08 UTC
[GitHub] [nuttx] pkarashchenko opened a new pull request, #10110: Use lib_free for memory deallocation after strdup or asprintf
pkarashchenko opened a new pull request, #10110:
URL: https://github.com/apache/nuttx/pull/10110
## Summary
This PR contains next set of the changes:
- use lib_free for memory de-allocation after `strdup` or `asprintf`
- use kernel internal API for libc stubs of ESP32
- Style fixes
- Adding missing `FAR` qualifier
- use `_SCHED_GETPID` in `getpgrp()` implementation
## Impact
Should fix assert "Free memory from the wrong heap" with flat mode and user separated heap enabled mode
## Testing
Pass CI
--
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 #10110: Use lib_free for memory deallocation after strdup or asprintf
Posted by "gustavonihei (via GitHub)" <gi...@apache.org>.
gustavonihei commented on code in PR #10110:
URL: https://github.com/apache/nuttx/pull/10110#discussion_r1286801053
##########
arch/risc-v/src/esp32c3/esp32c3_libc_stubs.c:
##########
@@ -59,12 +60,12 @@ struct _reent;
int _close_r(struct _reent *r, int fd)
{
- return close(fd);
+ return nx_close(fd);
Review Comment:
> @gustavonihei do we expect that newlib stubs are called from the application layer?
I think so. Some crypto-related algorithms implemented in the chip's Internal ROM make use of newlib functions which depend on the OS support. One example I can think of is the HMAC support, which motivated this PR some time ago: https://github.com/apache/nuttx/pull/7661
--
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] pkarashchenko commented on a diff in pull request #10110: Use lib_free for memory deallocation after strdup or asprintf
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #10110:
URL: https://github.com/apache/nuttx/pull/10110#discussion_r1287058192
##########
arch/risc-v/src/esp32c3/esp32c3_libc_stubs.c:
##########
@@ -59,12 +60,12 @@ struct _reent;
int _close_r(struct _reent *r, int fd)
{
- return close(fd);
+ return nx_close(fd);
Review Comment:
Ok. Let's move forward with this patch. Only one thing that I would like to highlight that for example `esp32_rom.ld` provides symbols `PROVIDE ( asprintf = 0x40056d9c );` and `PROVIDE ( strdup = 0x4000143c );` so if kernel code will link with it the memory allocation will most probably be done by newlib via `_malloc_r` implemented in this file and will lead to kernel memory allocation. On the other hand this patch is replacing `kmm_free` with `lib_free` calls to match memory allocation, so we are going to get mismatch here again. Maybe I will use `lib_malloc` in `_malloc_r`, so it will match `lib_free` explicit calls to release memory allocated by `strdup`
--
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 #10110: Use lib_free for memory deallocation after strdup or asprintf
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #10110:
URL: https://github.com/apache/nuttx/pull/10110#discussion_r1286773821
##########
arch/risc-v/src/esp32c3/esp32c3_libc_stubs.c:
##########
@@ -59,12 +60,12 @@ struct _reent;
int _close_r(struct _reent *r, int fd)
{
- return close(fd);
+ return nx_close(fd);
Review Comment:
the major difference between close and nx_close is how to handle the error code:
1. nx_close return error code to caller
2. close set error code to errno
--
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 merged pull request #10110: Use lib_free for memory deallocation after strdup or asprintf
Posted by "tmedicci (via GitHub)" <gi...@apache.org>.
tmedicci merged PR #10110:
URL: https://github.com/apache/nuttx/pull/10110
--
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] pkarashchenko commented on a diff in pull request #10110: Use lib_free for memory deallocation after strdup or asprintf
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #10110:
URL: https://github.com/apache/nuttx/pull/10110#discussion_r1287011089
##########
arch/risc-v/src/esp32c3/esp32c3_libc_stubs.c:
##########
@@ -59,12 +60,12 @@ struct _reent;
int _close_r(struct _reent *r, int fd)
{
- return close(fd);
+ return nx_close(fd);
Review Comment:
If we are having separate kernel and apps then we need to have two variants of stubs as we are not able to determine on runtime. To use `_NX_CLOSE`, `lib_malloc`, `lib_free` etc. we will need to have 2 stubs structures and app vs kernel to be liked with different variants (like we are having two variants of libc: one for kernel and one for apps) and this will require the stubs code to be compiled two times with different options. For now let me revert the changes related to stubs until we have a better solution as this seems to be a grey zone for me right now.
--
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 #10110: Use lib_free for memory deallocation after strdup or asprintf
Posted by "tmedicci (via GitHub)" <gi...@apache.org>.
tmedicci commented on PR #10110:
URL: https://github.com/apache/nuttx/pull/10110#issuecomment-1669316539
Re-running our internal CI...
--
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 #10110: Use lib_free for memory deallocation after strdup or asprintf
Posted by "tmedicci (via GitHub)" <gi...@apache.org>.
tmedicci commented on PR #10110:
URL: https://github.com/apache/nuttx/pull/10110#issuecomment-1668680928
I'll let our internal CI running and tomorrow morning I'll review the PR ;)
--
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 #10110: Use lib_free for memory deallocation after strdup or asprintf
Posted by "tmedicci (via GitHub)" <gi...@apache.org>.
tmedicci commented on code in PR #10110:
URL: https://github.com/apache/nuttx/pull/10110#discussion_r1286914801
##########
arch/risc-v/src/esp32c3/esp32c3_libc_stubs.c:
##########
@@ -59,12 +60,12 @@ struct _reent;
int _close_r(struct _reent *r, int fd)
{
- return close(fd);
+ return nx_close(fd);
Review Comment:
Hi!
> Is kernel linked with Internal ROM APIs that newlib functions?
Yes, there are internal ROM functions that require newlib functions to be defined, including crypto functions. Most of them run on the kernel side (e.g. Wireless), so theoretically, it should have no problem using kernel functions on the defining the stubs.
> Are apps linked with Internal ROM APIs that newlib functions?
I can't recall an app that uses them directly, but it'd be safer if we consider that. `hmac_md5` is an example: no app uses it directly, but we can't say for sure that clients don't.
I'd say that is safer to use `_NX_CLOSE`, `lib_malloc`, `lib_free` etc for now.
--
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] pkarashchenko commented on a diff in pull request #10110: Use lib_free for memory deallocation after strdup or asprintf
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #10110:
URL: https://github.com/apache/nuttx/pull/10110#discussion_r1286853707
##########
arch/risc-v/src/esp32c3/esp32c3_libc_stubs.c:
##########
@@ -59,12 +60,12 @@ struct _reent;
int _close_r(struct _reent *r, int fd)
{
- return close(fd);
+ return nx_close(fd);
Review Comment:
Regarding `struct reent` and `_errno`, I think it will not help much here as `__getreent` always return `NULL`, so there is no storage to reentrant data.
--
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 #10110: Use lib_free for memory deallocation after strdup or asprintf
Posted by "mu578 (via GitHub)" <gi...@apache.org>.
mu578 commented on PR #10110:
URL: https://github.com/apache/nuttx/pull/10110#issuecomment-1668809804
conflicting target on RISC
```c
Error: chip/esp_libc_stubs.c:97:5: error: conflicting types for '_lseek_r'
[469](https://github.com/apache/nuttx/actions/runs/5791110743/job/15695470612?pr=10110#step:7:470)
97 | int _lseek_r(struct _reent *r, int fd, int offset, int whence)
[470](https://github.com/apache/nuttx/actions/runs/5791110743/job/15695470612?pr=10110#step:7:471)
| ^~~~~~~~
[471](https://github.com/apache/nuttx/actions/runs/5791110743/job/15695470612?pr=10110#step:7:472)
In file included from /github/workspace/sources/nuttx/arch/risc-v/src/chip/esp-hal-3rdparty/components/esp_rom/include/esp32c3/rom/libc_stubs.h:14,
[472](https://github.com/apache/nuttx/actions/runs/5791110743/job/15695470612?pr=10110#step:7:473)
from chip/esp_libc_stubs.c:41:
[473](https://github.com/apache/nuttx/actions/runs/5791110743/job/15695470612?pr=10110#step:7:474)
/tools/riscv64-unknown-elf-gcc/riscv64-unknown-elf/include/reent.h:148:15: note: previous declaration of '_lseek_r' was here
[474](https://github.com/apache/nuttx/actions/runs/5791110743/job/15695470612?pr=10110#step:7:475)
148 | extern _off_t _lseek_r (struct _reent *, int, _off_t, int);
```
--
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 #10110: Use lib_free for memory deallocation after strdup or asprintf
Posted by "gustavonihei (via GitHub)" <gi...@apache.org>.
gustavonihei commented on code in PR #10110:
URL: https://github.com/apache/nuttx/pull/10110#discussion_r1287069682
##########
arch/risc-v/src/esp32c3/esp32c3_libc_stubs.c:
##########
@@ -59,12 +60,12 @@ struct _reent;
int _close_r(struct _reent *r, int fd)
{
- return close(fd);
+ return nx_close(fd);
Review Comment:
> Only one thing that I would like to highlight that for example `esp32_rom.ld` provides symbols `PROVIDE ( asprintf = 0x40056d9c );` and `PROVIDE ( strdup = 0x4000143c );` so if kernel code will link with it the memory allocation will most probably be done by newlib via `_malloc_r` implemented in this file and will lead to kernel memory allocation.
The symbols included via `PROVIDE` keyword behave as weak references, so `asprintf` and `strdup` from NuttX `libkc.a` will prevail in the Kernel image.
--
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 #10110: Use lib_free for memory deallocation after strdup or asprintf
Posted by "tmedicci (via GitHub)" <gi...@apache.org>.
tmedicci commented on code in PR #10110:
URL: https://github.com/apache/nuttx/pull/10110#discussion_r1286914801
##########
arch/risc-v/src/esp32c3/esp32c3_libc_stubs.c:
##########
@@ -59,12 +60,12 @@ struct _reent;
int _close_r(struct _reent *r, int fd)
{
- return close(fd);
+ return nx_close(fd);
Review Comment:
Hi!
> Is kernel linked with Internal ROM APIs that newlib functions?
Yes, there are internal ROM functions that require newlib functions to be defined, including crypto functions. Most of them run on the kernel side (e.g. Wireless), so theoretically, it should have no problem using kernel functions on the defining the stubs.
> Are apps linked with Internal ROM APIs that newlib functions?
I can't recall an app that uses them directly, but it'd be safer if we consider that. `hmac_md5` is an example: no app uses it directly, but we can't say for sure that clients don't.
I'd say that is safer to use `_NX_CLOSE`, `lib_malloc`, `lib_free` etc for now.
--
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] pkarashchenko commented on a diff in pull request #10110: Use lib_free for memory deallocation after strdup or asprintf
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #10110:
URL: https://github.com/apache/nuttx/pull/10110#discussion_r1287011089
##########
arch/risc-v/src/esp32c3/esp32c3_libc_stubs.c:
##########
@@ -59,12 +60,12 @@ struct _reent;
int _close_r(struct _reent *r, int fd)
{
- return close(fd);
+ return nx_close(fd);
Review Comment:
If we are having separate kernel and apps then we need to have two variants of stubs as we are not able to determine on runtime. To use `_NX_CLOSE`, `lib_malloc`, `lib_free` etc. we will need to have 2 stubs structures and app vs kernel to be liked with different variants (like we are having two variants of libc: one for kernel and one for apps). For now let me revert the changes related to stubs until we have a better solution as this seems to be a grey zone for me right now.
--
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 #10110: Use lib_free for memory deallocation after strdup or asprintf
Posted by "gustavonihei (via GitHub)" <gi...@apache.org>.
gustavonihei commented on code in PR #10110:
URL: https://github.com/apache/nuttx/pull/10110#discussion_r1287069682
##########
arch/risc-v/src/esp32c3/esp32c3_libc_stubs.c:
##########
@@ -59,12 +60,12 @@ struct _reent;
int _close_r(struct _reent *r, int fd)
{
- return close(fd);
+ return nx_close(fd);
Review Comment:
> Only one thing that I would like to highlight that for example `esp32_rom.ld` provides symbols `PROVIDE ( asprintf = 0x40056d9c );` and `PROVIDE ( strdup = 0x4000143c );` so if kernel code will link with it the memory allocation will most probably be done by newlib via `_malloc_r` implemented in this file and will lead to kernel memory allocation.
The symbols included via `PROVIDE` keyword behave as weak references, so `asprintf` and `strdup` from NuttX `libkc.a` will prevail in the Kernel image.
https://sourceware.org/binutils/docs/ld/PROVIDE.html
--
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 #10110: Use lib_free for memory deallocation after strdup or asprintf
Posted by "tmedicci (via GitHub)" <gi...@apache.org>.
tmedicci commented on code in PR #10110:
URL: https://github.com/apache/nuttx/pull/10110#discussion_r1287084421
##########
arch/risc-v/src/esp32c3/esp32c3_libc_stubs.c:
##########
@@ -59,12 +60,12 @@ struct _reent;
int _close_r(struct _reent *r, int fd)
{
- return close(fd);
+ return nx_close(fd);
Review Comment:
Thanks @pkarashchenko !
(thanks @gustavonihei for the observations ;) )
I'll re-run our internal CI.
--
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 #10110: Use lib_free for memory deallocation after strdup or asprintf
Posted by "mu578 (via GitHub)" <gi...@apache.org>.
mu578 commented on PR #10110:
URL: https://github.com/apache/nuttx/pull/10110#issuecomment-1668809308
```
Error: chip/esp_libc_stubs.c:97:5: error: conflicting types for '_lseek_r'
[469](https://github.com/apache/nuttx/actions/runs/5791110743/job/15695470612?pr=10110#step:7:470)
97 | int _lseek_r(struct _reent *r, int fd, int offset, int whence)
[470](https://github.com/apache/nuttx/actions/runs/5791110743/job/15695470612?pr=10110#step:7:471)
| ^~~~~~~~
[471](https://github.com/apache/nuttx/actions/runs/5791110743/job/15695470612?pr=10110#step:7:472)
In file included from /github/workspace/sources/nuttx/arch/risc-v/src/chip/esp-hal-3rdparty/components/esp_rom/include/esp32c3/rom/libc_stubs.h:14,
[472](https://github.com/apache/nuttx/actions/runs/5791110743/job/15695470612?pr=10110#step:7:473)
from chip/esp_libc_stubs.c:41:
[473](https://github.com/apache/nuttx/actions/runs/5791110743/job/15695470612?pr=10110#step:7:474)
/tools/riscv64-unknown-elf-gcc/riscv64-unknown-elf/include/reent.h:148:15: note: previous declaration of '_lseek_r' was here
[474](https://github.com/apache/nuttx/actions/runs/5791110743/job/15695470612?pr=10110#step:7:475)
148 | extern _off_t _lseek_r (struct _reent *, int, _off_t, int);
```
--
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] pkarashchenko commented on a diff in pull request #10110: Use lib_free for memory deallocation after strdup or asprintf
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #10110:
URL: https://github.com/apache/nuttx/pull/10110#discussion_r1286830967
##########
arch/risc-v/src/esp32c3/esp32c3_libc_stubs.c:
##########
@@ -59,12 +60,12 @@ struct _reent;
int _close_r(struct _reent *r, int fd)
{
- return close(fd);
+ return nx_close(fd);
Review Comment:
> of newlib functions which depend on the OS support. One example I can think of is the HMAC support, which motivated this PR some time ago: #7661
Ok. Just let me understand the use case. Till now I thought that libc stubs are called from ESP-IDF layer that runs in kernel, but if the stubs are needed for application usage only, then I will revert changes for libc stubs so they will be mapped to application layer APIs that will lead to a system calls in case of non-flat build mode
--
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] pkarashchenko commented on a diff in pull request #10110: Use lib_free for memory deallocation after strdup or asprintf
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #10110:
URL: https://github.com/apache/nuttx/pull/10110#discussion_r1287072105
##########
arch/risc-v/src/esp32c3/esp32c3_libc_stubs.c:
##########
@@ -59,12 +60,12 @@ struct _reent;
int _close_r(struct _reent *r, int fd)
{
- return close(fd);
+ return nx_close(fd);
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] gustavonihei commented on a diff in pull request #10110: Use lib_free for memory deallocation after strdup or asprintf
Posted by "gustavonihei (via GitHub)" <gi...@apache.org>.
gustavonihei commented on code in PR #10110:
URL: https://github.com/apache/nuttx/pull/10110#discussion_r1286865772
##########
arch/risc-v/src/esp32c3/esp32c3_libc_stubs.c:
##########
@@ -59,12 +60,12 @@ struct _reent;
int _close_r(struct _reent *r, int fd)
{
- return close(fd);
+ return nx_close(fd);
Review Comment:
As much as I would like to, unfortunately I cannot dedicate much time to investigate and give you the appropriate answer.
@tmedicci and @lucasssvaz, could you please help on this?
--
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 #10110: Use lib_free for memory deallocation after strdup or asprintf
Posted by "gustavonihei (via GitHub)" <gi...@apache.org>.
gustavonihei commented on code in PR #10110:
URL: https://github.com/apache/nuttx/pull/10110#discussion_r1286508225
##########
arch/risc-v/src/esp32c3/esp32c3_libc_stubs.c:
##########
@@ -59,12 +60,12 @@ struct _reent;
int _close_r(struct _reent *r, int fd)
{
- return close(fd);
+ return nx_close(fd);
Review Comment:
I am not sure about this change. These stubs are called by the newlib implementation located in the chip's internal ROM. In FLAT build these newlib functions may be called by userspace code, e.g. wireless protocol implementation.
@tmedicci
--
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] pkarashchenko commented on a diff in pull request #10110: Use lib_free for memory deallocation after strdup or asprintf
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #10110:
URL: https://github.com/apache/nuttx/pull/10110#discussion_r1286861005
##########
arch/risc-v/src/esp32c3/esp32c3_libc_stubs.c:
##########
@@ -59,12 +60,12 @@ struct _reent;
int _close_r(struct _reent *r, int fd)
{
- return close(fd);
+ return nx_close(fd);
Review Comment:
So the main questions are:
1. Is kernel linked with Internal ROM APIs that newlib functions?
2. Are apps linked with Internal ROM APIs that newlib functions?
Those two answers will give us a direction to move forward. If both are "yes", then we will need to use `_NX_CLOSE`, `lib_malloc`, `lib_free`, etc. basically following up how the rest of the things are done in NuttX `libc` implementation.
--
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 #10110: Use lib_free for memory deallocation after strdup or asprintf
Posted by "tmedicci (via GitHub)" <gi...@apache.org>.
tmedicci commented on code in PR #10110:
URL: https://github.com/apache/nuttx/pull/10110#discussion_r1287036977
##########
arch/risc-v/src/esp32c3/esp32c3_libc_stubs.c:
##########
@@ -59,12 +60,12 @@ struct _reent;
int _close_r(struct _reent *r, int fd)
{
- return close(fd);
+ return nx_close(fd);
Review Comment:
> To use _NX_CLOSE, lib_malloc, lib_free etc. we will need to have 2 stubs structures and app vs kernel to be liked with different variants (like we are having two variants of libc: one for kernel and one for apps) and this will require the stubs code to be compiled two times with different options
I thought the same, but after a quick review, I think we can continue on this approach: please note that 1) on flat build, the preprocessor will select the `close` and `malloc`-like functions and they can be used for applications without problems (as it was prior this patch). Considering the protected mode, it will use the kernel functions (as expected). Finally, we won't be able to use the functions from the userspace when protected mode is enabled, but this is expected (we should integrate them into the associated interfaces).
--
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] royfengsss commented on pull request #10110: Use lib_free for memory deallocation after strdup or asprintf
Posted by "royfengsss (via GitHub)" <gi...@apache.org>.
royfengsss commented on PR #10110:
URL: https://github.com/apache/nuttx/pull/10110#issuecomment-1668821520
As far as I tested on ESP32, the build passed and the crash former seen has been fixed.
--
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] pkarashchenko commented on a diff in pull request #10110: Use lib_free for memory deallocation after strdup or asprintf
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #10110:
URL: https://github.com/apache/nuttx/pull/10110#discussion_r1286830967
##########
arch/risc-v/src/esp32c3/esp32c3_libc_stubs.c:
##########
@@ -59,12 +60,12 @@ struct _reent;
int _close_r(struct _reent *r, int fd)
{
- return close(fd);
+ return nx_close(fd);
Review Comment:
> of newlib functions which depend on the OS support. One example I can think of is the HMAC support, which motivated this PR some time ago: #7661
Ok. Just let me understand the use case. Till now I thought that libc stubs are called from ESP-IDF layer that runs in kernel, but if the stubs are needed for application usage only, then I will revert changes for libc stubs so they will be mapped to application layer APIs that will lead to a system calls in case of non-flat build mode.
I mean that `close` is a system call while `nx_close` is not, so that is important case if stubs are called by application layer.
--
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] pkarashchenko commented on a diff in pull request #10110: Use lib_free for memory deallocation after strdup or asprintf
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #10110:
URL: https://github.com/apache/nuttx/pull/10110#discussion_r1286779099
##########
arch/risc-v/src/esp32c3/esp32c3_libc_stubs.c:
##########
@@ -59,12 +60,12 @@ struct _reent;
int _close_r(struct _reent *r, int fd)
{
- return close(fd);
+ return nx_close(fd);
Review Comment:
The main difference is setting `errno` that should not be done when newlib is called from kernel.
@gustavonihei do we expect that newlib stubs are called from the application layer?
For the return codes we can go with
```
int _close_r(struct _reent *r, int fd)
{
if (nx_close(fd) < 0)
{
return ERROR;
}
return OK;
}
```
--
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 #10110: Use lib_free for memory deallocation after strdup or asprintf
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #10110:
URL: https://github.com/apache/nuttx/pull/10110#discussion_r1286773821
##########
arch/risc-v/src/esp32c3/esp32c3_libc_stubs.c:
##########
@@ -59,12 +60,12 @@ struct _reent;
int _close_r(struct _reent *r, int fd)
{
- return close(fd);
+ return nx_close(fd);
Review Comment:
the major difference between close and nx_close is how to handle the error code:
1. nx_close return error code to caller
2. close set error code to errno
so, it is no real difference in this case.
--
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 #10110: Use lib_free for memory deallocation after strdup or asprintf
Posted by "gustavonihei (via GitHub)" <gi...@apache.org>.
gustavonihei commented on code in PR #10110:
URL: https://github.com/apache/nuttx/pull/10110#discussion_r1286810513
##########
arch/risc-v/src/esp32c3/esp32c3_libc_stubs.c:
##########
@@ -59,12 +60,12 @@ struct _reent;
int _close_r(struct _reent *r, int fd)
{
- return close(fd);
+ return nx_close(fd);
Review Comment:
> the major difference between close and nx_close is how to handle the error code:
>
> 1. nx_close return error code to caller
> 2. close set error code to errno
>
> so, it is no real difference in this case.
> For the return codes we can go with
>
> ```
> int _close_r(struct _reent *r, int fd)
> {
> if (nx_close(fd) < 0)
> {
> return ERROR;
> }
>
> return OK;
> }
> ```
Seems like a good suggestion.
One thing I just noticed is that `struct reent` provides an `_errno` field, so maybe it could save `nx_close` return value there, similar to how it is being done here: https://chromium.googlesource.com/native_client/nacl-newlib/+/refs/heads/main/newlib/libc/reent/closer.c
--
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 #10110: Use lib_free for memory deallocation after strdup or asprintf
Posted by "mu578 (via GitHub)" <gi...@apache.org>.
mu578 commented on PR #10110:
URL: https://github.com/apache/nuttx/pull/10110#issuecomment-1668809391
```
Error: chip/esp_libc_stubs.c:97:5: error: conflicting types for '_lseek_r'
[469](https://github.com/apache/nuttx/actions/runs/5791110743/job/15695470612?pr=10110#step:7:470)
97 | int _lseek_r(struct _reent *r, int fd, int offset, int whence)
[470](https://github.com/apache/nuttx/actions/runs/5791110743/job/15695470612?pr=10110#step:7:471)
| ^~~~~~~~
[471](https://github.com/apache/nuttx/actions/runs/5791110743/job/15695470612?pr=10110#step:7:472)
In file included from /github/workspace/sources/nuttx/arch/risc-v/src/chip/esp-hal-3rdparty/components/esp_rom/include/esp32c3/rom/libc_stubs.h:14,
[472](https://github.com/apache/nuttx/actions/runs/5791110743/job/15695470612?pr=10110#step:7:473)
from chip/esp_libc_stubs.c:41:
[473](https://github.com/apache/nuttx/actions/runs/5791110743/job/15695470612?pr=10110#step:7:474)
/tools/riscv64-unknown-elf-gcc/riscv64-unknown-elf/include/reent.h:148:15: note: previous declaration of '_lseek_r' was here
[474](https://github.com/apache/nuttx/actions/runs/5791110743/job/15695470612?pr=10110#step:7:475)
148 | extern _off_t _lseek_r (struct _reent *, int, _off_t, int);
```
--
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] pkarashchenko commented on a diff in pull request #10110: Use lib_free for memory deallocation after strdup or asprintf
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #10110:
URL: https://github.com/apache/nuttx/pull/10110#discussion_r1287074970
##########
arch/risc-v/src/esp32c3/esp32c3_libc_stubs.c:
##########
@@ -59,12 +60,12 @@ struct _reent;
int _close_r(struct _reent *r, int fd)
{
- return close(fd);
+ return nx_close(fd);
Review Comment:
> > Only one thing that I would like to highlight that for example `esp32_rom.ld` provides symbols `PROVIDE ( asprintf = 0x40056d9c );` and `PROVIDE ( strdup = 0x4000143c );` so if kernel code will link with it the memory allocation will most probably be done by newlib via `_malloc_r` implemented in this file and will lead to kernel memory allocation.
>
> The symbols included via `PROVIDE` keyword behave as weak references, so `asprintf` and `strdup` from NuttX `libkc.a` will prevail in the Kernel image.
>
> https://sourceware.org/binutils/docs/ld/PROVIDE.html
Ok. Anyway going with `lib_malloc`/`lib_free` here seems to be fine.
--
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