Skip to content

Conversation

@xuxin930
Copy link
Contributor

Summary

PR #14668 caused a build error after adjusting the layout.
This commit fixes this problem

/home/data/vela/github/nuttx/libs/libxx/libcxxabi/libcxxabi/src/cxa_exception.h:47:10: error: 'unexpected_handler' in namespace 'std' does not name a type
   47 |     std::unexpected_handler unexpectedHandler;
      |          ^~~~~~~~~~~~~~~~~~
/home/data/vela/github/nuttx/libs/libxx/libcxxabi/libcxxabi/src/cxa_exception.h:85:10: error: 'unexpected_handler' in namespace 'std' does not name a type
   85 |     std::unexpected_handler unexpectedHandler;
      |          ^~~~~~~~~~~~~~~~~~
/home/data/vela/github/nuttx/libs/libxx/libcxxabi/libcxxabi/src/cxa_exception.h:121:65: error: static assertion failed: propagationCount has wrong negative offset
  121 |                       sizeof(_Unwind_Exception) + sizeof(void*) ==
      |                                                                 ^
/home/data/vela/github/nuttx/libs/libxx/libcxxabi/libcxxabi/src/cxa_exception.h:121:65: note: the comparison reduces to '(124 == 128)'
/home/data/vela/github/nuttx/libs/libxx/libcxxabi/libcxxabi/src/cxa_exception.h:125:65: error: static assertion failed: propagationCount has wrong negative offset
  125 |                       sizeof(_Unwind_Exception) + sizeof(void*) ==
      |

/home/data/vela/github/nuttx/libs/libxx/libcxxabi/libcxxabi/src/cxa_default_handlers.cpp:20:10: fatal error: include/atomic_support.h: No such file or directory
   20 | #include "include/atomic_support.h" // from libc++
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.

Impact

bugfix

Testing

enable CONFIG_LIBCXX
enable CONFIG_LIBCXXABI

cmake -B build -DBOARD_CONFIG=qemu-armv7a:nsh -GNinja

build pass

/home/data/vela/github/nuttx/libs/libxx/libcxxabi/libcxxabi/src/cxa_exception.h:47:10: error: 'unexpected_handler' in namespace 'std' does not name a type
   47 |     std::unexpected_handler unexpectedHandler;
      |          ^~~~~~~~~~~~~~~~~~
/home/data/vela/github/nuttx/libs/libxx/libcxxabi/libcxxabi/src/cxa_exception.h:85:10: error: 'unexpected_handler' in namespace 'std' does not name a type
   85 |     std::unexpected_handler unexpectedHandler;
      |          ^~~~~~~~~~~~~~~~~~
/home/data/vela/github/nuttx/libs/libxx/libcxxabi/libcxxabi/src/cxa_exception.h:121:65: error: static assertion failed: propagationCount has wrong negative offset
  121 |                       sizeof(_Unwind_Exception) + sizeof(void*) ==
      |                                                                 ^
/home/data/vela/github/nuttx/libs/libxx/libcxxabi/libcxxabi/src/cxa_exception.h:121:65: note: the comparison reduces to '(124 == 128)'
/home/data/vela/github/nuttx/libs/libxx/libcxxabi/libcxxabi/src/cxa_exception.h:125:65: error: static assertion failed: propagationCount has wrong negative offset
  125 |                       sizeof(_Unwind_Exception) + sizeof(void*) ==
      |

/home/data/vela/github/nuttx/libs/libxx/libcxxabi/libcxxabi/src/cxa_default_handlers.cpp:20:10: fatal error: include/atomic_support.h: No such file or directory
   20 | #include "include/atomic_support.h" // from libc++
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.

Signed-off-by: xuxin19 <xuxin19@xiaomi.com>
@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 Nov 14, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 14, 2024

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the NuttX requirements, although some sections could be expanded for clarity. Here's a breakdown:

Strengths:

  • Clear Summary: The summary identifies the root cause (PR libxx: All cxx libraries move down one level #14668) and the nature of the fix. The error messages are helpful in understanding the problem.
  • Impact is Stated: While concise, "bugfix" clearly indicates the impact.
  • Testing Information Provided: The instructions to reproduce the issue and verify the fix are present, including the necessary configurations (CONFIG_LIBCXX, CONFIG_LIBCXXABI) and the build command. The confirmation of a successful build after the change is important.

Areas for Improvement (While Minor, they enhance clarity):

  • More Detail in Summary: Briefly explain how the fix addresses the build errors. For example, "This commit fixes the build errors by correcting the type definitions related to unexpected_handler and adjusting the size calculations in cxa_exception.h to resolve the static assertion failures. It also includes the correct path for atomic_support.h."
  • Expand on Impact Sections: While stating "bugfix" is acceptable, filling out the "NO/YES" sections explicitly enhances clarity. Example:
    • Is new feature added? NO
    • Is existing feature changed? NO (it's a fix, not a change in functionality)
    • Impact on user: NO
    • Impact on build: YES (previously failing build now succeeds)
    • Impact on hardware: NO
    • Impact on documentation: NO
    • Impact on security: NO
    • Impact on compatibility: NO
  • Before/After Logs: While stating "build pass" is sufficient in this case where the previous state was a complete build failure, ideally, snippets of the build output (especially the success message) before and after the fix would further demonstrate the correction. This is less critical when the before state is a total failure but good practice in general.

Conclusion:

The PR substantially meets the requirements. The suggested improvements would make it even stronger by providing more comprehensive information and clarity for reviewers.

@xiaoxiang781216 xiaoxiang781216 merged commit 5ffdf5b into apache:master Nov 14, 2024
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.

4 participants