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

libcxxabi: Optimize cxx code size. #13721

Merged
merged 1 commit into from
Oct 8, 2024

Conversation

cuiziwei1
Copy link
Contributor

Summary

enable LIBSUPCXX:
text data bss dec hex filename
93080 592 5472 99144 18348 nuttx/nuttx

enable LIBCXXABI:
before:
text data bss dec hex filename
126468 352 5960 132780 206ac nuttx/nuttx

after:
text data bss dec hex filename
68048 352 5960 74360 12278 nuttx/nuttx

Impact

Testing

@github-actions github-actions bot added Area: OS Components OS Components issues Size: S The size of the change in this PR is small labels Sep 29, 2024
@nuttxpr
Copy link

nuttxpr commented Sep 29, 2024

[Experimental Bot, please feedback here]

Let's break down whether this Pull Request (PR) description meets the NuttX requirements.

Areas Where the PR Description is Lacking:

  • Summary:
    • Why is this change necessary? Is it fixing a bug, adding a feature, or improving performance? The description only states "enable LIBSUPCXX" and "enable LIBCXXABI" without explaining the motivation.
    • What functional part of the code is changed? Be specific. What files or modules are affected by enabling these libraries?
    • How does the change work? Provide a technical explanation of how enabling these libraries impacts the NuttX build or runtime.
  • Impact:
    • Almost all sections are blank. You must address each impact area. For enabled libraries, consider:
      • New feature: Does this unlock new C++ capabilities?
      • Build: How does enabling these libraries affect the build process, dependencies, or configuration?
      • Hardware: Are there any architecture-specific implications?
      • Compatibility: Does this affect compatibility with existing NuttX codebases or applications?
      • Security: Do these libraries introduce any potential security concerns?
  • Testing:
    • Insufficient Detail:
      • Build Hosts: Specify the operating systems, architectures, and compilers used for testing.
      • Targets: List the specific architectures, boards, and configurations tested on.
    • Missing Logs: The "Testing logs before change" and "Testing logs after change" sections are empty. Provide actual log snippets demonstrating the change's effect.

How to Improve the PR Description:

  1. Motivation: Clearly explain the reason for enabling LIBSUPCXX and LIBCXXABI. What benefits do they provide? What problem are you solving?

  2. Technical Details:

    • Describe the specific code changes (files modified, configuration options, etc.).
    • Explain how enabling these libraries affects NuttX internally (memory usage, dependencies, etc.).
  3. Complete the Impact Section:

    • Carefully analyze and document the impact on every aspect listed in the NuttX requirements.
    • Don't just say "NO" – provide justification. For example, "Impact on hardware: NO. These libraries are standard C++ components and do not introduce hardware-specific changes."
  4. Provide Detailed Testing Information:

    • List the exact build hosts and target environments used for testing.
    • Include relevant snippets from your testing logs to demonstrate the before-and-after behavior.

Example of an Improved Summary:

This PR enables the LIBSUPCXX and LIBCXXABI libraries in the NuttX build system to provide support for essential C++ features, including exception handling and runtime type information (RTTI). These libraries are required for applications using modern C++ techniques and are commonly found in standard C++ environments. The change involves modifying the NuttX configuration system to include these libraries during the build process.

By following these recommendations, you'll create a much stronger PR that is more likely to be accepted. Remember, a good PR description is clear, comprehensive, and convincing!

@pkarashchenko
Copy link
Contributor

====================================================================================
Configuration/Tool: esp32c3-devkit/cxx
2024-09-29 07:10:38
------------------------------------------------------------------------------------
  Cleaning...
  Configuring...
  Building NuttX...
riscv-none-elf-ld: warning: /Users/runner/work/nuttx/nuttx/sources/nuttx/nuttx has a LOAD segment with RWX permissions
riscv-none-elf-ld: /Users/runner/work/nuttx/nuttx/sources/nuttx/staging/libxx.a(exception.o): in function `std::uncaught_exception()':
exception.cpp:(.text._ZSt18uncaught_exceptionv+0x4): undefined reference to `__cxa_uncaught_exceptions'
make[1]: *** [nuttx] Error 1
make: *** [nuttx] Error 2

@cuiziwei1
Copy link
Contributor Author

====================================================================================
Configuration/Tool: esp32c3-devkit/cxx
2024-09-29 07:10:38
------------------------------------------------------------------------------------
  Cleaning...
  Configuring...
  Building NuttX...
riscv-none-elf-ld: warning: /Users/runner/work/nuttx/nuttx/sources/nuttx/nuttx has a LOAD segment with RWX permissions
riscv-none-elf-ld: /Users/runner/work/nuttx/nuttx/sources/nuttx/staging/libxx.a(exception.o): in function `std::uncaught_exception()':
exception.cpp:(.text._ZSt18uncaught_exceptionv+0x4): undefined reference to `__cxa_uncaught_exceptions'
make[1]: *** [nuttx] Error 1
make: *** [nuttx] Error 2

@cuiziwei1 cuiziwei1 closed this Sep 30, 2024
@cuiziwei1 cuiziwei1 reopened this Sep 30, 2024
@cuiziwei1
Copy link
Contributor Author

This is my fault. A fix patch has been submitted. Please try compiling again.

@pkarashchenko
Copy link
Contributor

The CI error now is

: && /tools/ccache/bin/c++  -Wl,--gc-sections -T nuttx.ld CMakeFiles/nuttx.dir/arch/sim/src/sim/posix/sim_hostirq.c.o CMakeFiles/nuttx.dir/arch/sim/src/sim/posix/sim_hostmemory.c.o CMakeFiles/nuttx.dir/arch/sim/src/sim/posix/sim_hostmisc.c.o CMakeFiles/nuttx.dir/arch/sim/src/sim/posix/sim_hosttime.c.o CMakeFiles/nuttx.dir/arch/sim/src/sim/posix/sim_hostuart.c.o CMakeFiles/nuttx.dir/arch/sim/src/sim/posix/sim_tapdev.c.o CMakeFiles/nuttx.dir/arch/sim/src/sim/posix/sim_hosthcisocket.c.o CMakeFiles/nuttx.dir/arch/sim/src/sim/posix/sim_hostfs.c.o CMakeFiles/nuttx.dir/empty.cxx.o -o nuttx  nuttx.rel  -lpthread  -lz  -lrt && :
/usr/bin/ld: nuttx.rel: in function `std::uncaught_exceptions()':
/github/workspace/sources/nuttx/libs/libxx/libcxx/src/support/runtime/exception_libcxxabi.ipp:21: undefined reference to `__cxa_uncaught_exceptions'
collect2: error: ld returned 1 exit status
ninja: build stopped: subcommand failed.
cp: cannot stat 'nuttx': No such file or directory
HEAD detached at pull/13721/merge
Untracked files:
  (use "git add <file>..." to include in what will be committed)
	libs/libxx/libcxx.rej

libs/libxx/libcxxabi.defs Outdated Show resolved Hide resolved
libs/libxx/libcxxabi.cmake Outdated Show resolved Hide resolved
@xiaoxiang781216
Copy link
Contributor

@cuiziwei1 please fix the ci error.

@cuiziwei1 cuiziwei1 force-pushed the opt_libcxxabi branch 3 times, most recently from 8b28f27 to ed70d00 Compare October 8, 2024 11:32
enable LIBSUPCXX:
text data bss dec hex filename
93080 592 5472 99144 18348 nuttx/nuttx

enable LIBCXXABI:
before:
text data bss dec hex filename
126468 352 5960 132780 206ac nuttx/nuttx

after:
text data bss dec hex filename
68048 352 5960 74360 12278 nuttx/nuttx

Summary:
Optimize text segment code size by about 46%.

Signed-off-by: cuiziwei <[email protected]>
@xiaoxiang781216 xiaoxiang781216 merged commit 39576af into apache:master Oct 8, 2024
32 checks passed
@cuiziwei1 cuiziwei1 deleted the opt_libcxxabi branch October 18, 2024 03:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: OS Components OS Components issues Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants