Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Use pathbuffer reduce stack #14632

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

crafcat7
Copy link
Contributor

@crafcat7 crafcat7 commented Nov 4, 2024

Summary

Why is this change necessary?
Benefit from the lib_pathbuffer feature introduced in #12969, we can further optimize the stack usage size in the NuttX kernel.
Changes
In this change, we replaced the declarations related to xxx[PATH_MAX] in the NuttX directory with the lib_pathbuffer method

Impact

Is a new feature added?: NO
Impact on build: NO
Impact on hardware: NO, this change does not specifically target any particular hardware architectures or boards.
Impact on documentation: NO,This patch does not introduce any new features
Impact on compatibility: This implementation aims to be backward compatible. No existing functionality is expected to be broken.

Testing

Build Host(s): Linux x86
Target(s): sim/nsh

By default

LIBC_MAX_PATHBUFFER = 2
LIBC_PATHBUFFER_MALLOC = y

So when there is not enough memory on the stack, it will be requested from the heap

Summary:
  Modified the usage logic, mainly introduced lib_get_pathbuffer and lib_put_pathbuffer

Signed-off-by: chenrun1 <[email protected]>
@github-actions github-actions bot added Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Area: Drivers Drivers issues Area: File System File System issues Area: OS Components OS Components issues Area: Sensors Sensors issues Size: L The size of the change in this PR is large labels Nov 4, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 4, 2024

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the NuttX requirements based on the provided information. However, there are a couple of minor improvements that could be made to strengthen the testing section.

While the PR states testing was done on sim/nsh, the "Testing logs before change" and "Testing logs after change" sections are empty. Providing concrete examples of the stack usage before and after the change would significantly improve the review process. For example, showing the output of a command that displays stack usage, or even a code snippet demonstrating how the stack size was measured would be beneficial. This would provide concrete evidence of the optimization.

@@ -539,7 +540,13 @@ int esp_timer_initialize(uint32_t timer_id)
break;
}

snprintf(devpath, sizeof(devpath), "/dev/timer%" PRIu32, timer_id);
devpath = lib_get_pathbuffer();
Copy link
Contributor

@anchao anchao Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since CONFIG_PATH_MAX can define a smaller size. let us add a configure CONFIG_LIBC_PATHBUFFER to enable/disable lib_get_pathbuffer implement.

#ifdef CONFIG_LIBC_PATHBUFFER
FAR char *lib_get_pathbuffer(void);
#else
#  define lib_get_pathbuffer() alloca(PATH_MAX)
#endif

Copy link
Contributor

@xiaoxiang781216 xiaoxiang781216 Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alloca isn't ANSI or POSIX Api, so it isn't available on all possible compiler, and should disable by default.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current code base:
https://github.com/apache/nuttx/blob/master/include/string.h#L40-L51
image

I think most toolchains support this feature (even MSVC has a similar implementation)
https://github.com/apache/nuttx/blob/master/include/alloca.h#L30-L42

libs/libc/locale/lib_catalog.c:197:9: warning: implicit declaration of function ‘lib_get_pathbuffer’ [-Wimplicit-function-declaration]
  197 |   buf = lib_get_pathbuffer();
      |         ^~~~~~~~~~~~~~~~~~
libs/libc/locale/lib_catalog.c:197:7: warning: assignment to ‘char *’ from ‘int’ makes pointer from integer without a cast [-Wint-conversion]
  197 |   buf = lib_get_pathbuffer();
      |       ^
libs/libc/locale/lib_catalog.c:294:13: warning: implicit declaration of function ‘lib_put_pathbuffer’ [-Wimplicit-function-declaration]
  294 |             lib_put_pathbuffer(buf);

Signed-off-by: zhangshoukui <[email protected]>
@crafcat7
Copy link
Contributor Author

crafcat7 commented Nov 5, 2024

I submitted a fix for the redefinition problem in libc under kernel build when using ffs/ffsl/ffsll.
a9e1d1b

@crafcat7
Copy link
Contributor Author

crafcat7 commented Nov 6, 2024

There is a ld problem here, I found that the static always inline ffs function implementation exists in the compilation expansion, but it cannot find the builtin ffs in the final link.

I tried changing the ffs part to __builtin_ffs in the code, but it still prompted an undefined reference to `ffs'.
Similarly, using ctz / __builtin_ctz / ffsl / __builtin_ffsl / ffsll / __builtin_ffsll does not have this problem

It seems that the implementation of _builtin_ffs cannot be found.

So is there any good way to solve this problem? Or should we add more restrictions to the implementation of builtin_ffs?

riscv-none-elf-ld: /github/workspace/sources/nuttx/staging/libc.a(lib_pathbuffer.o): in function `lib_get_pathbuffer':
/github/workspace/sources/nuttx/libs/libc/misc/lib_pathbuffer.c:89:(.text.lib_get_pathbuffer+0x1c): undefined reference to `ffs'
make[1]: *** [Makefile:189: nuttx] Error 1
make: *** [tools/Unix.mk:551: nuttx] Error 2

Summary
 When CONFIG_HAVE_BUILTIN_XXX is enabled, use builtin functions to avoid implementation conflicts
riscv-none-elf-ld: /github/workspace/sources/nuttx/staging/libc.a(exec_symtab.o): in function `ffs':
/github/workspace/sources/nuttx/libs/libc/exec_symtab.c:81: multiple definition of `ffs'; /github/workspace/sources/nuttx/staging/libc.a(lib_ffs.o):/github/workspace/sources/nuttx/libs/libc/string/lib_ffs.c:51: first defined here
make[1]: *** [Makefile:189: nuttx] Error 1
make: *** [tools/Unix.mk:551: nuttx] Error 2

Signed-off-by: chenrun1 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Area: Drivers Drivers issues Area: File System File System issues Area: OS Components OS Components issues Area: Sensors Sensors issues Size: L The size of the change in this PR is large
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants