Skip to content

Conversation

@crafcat7
Copy link
Contributor

Summary:

Summary

Change nxmutex to spinlock to fix bug:

_assert
|
 ->dump_fatal_info
   |
    ->dump_tasks
      |
       ->dump_filelist
         |
          ->files_dumplist
            |
             ->lib_get_pathbuffer
               |
                ->nxmutex_lock      <-- hold mutex on assert will trigger an nested exception

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
Test cases based on local qemu environment passed

@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 15, 2024
@crafcat7 crafcat7 force-pushed the lib_pathbuffer_change_spinlock branch from dd81ba1 to a986ff4 Compare November 15, 2024 02:42
@nuttxpr
Copy link

nuttxpr commented Nov 15, 2024

[Experimental Bot, please feedback here]

Yes, this PR mostly meets the NuttX requirements, but could be improved.

Here's what's good:

  • Clear problem description: The summary clearly identifies the bug and its root cause with a helpful stack trace.
  • Impact assessment: Addresses all impact points.
  • Testing information: Provides host and target information, and mentions passing tests.

Here's what needs improvement:

  • Summary - "How does the change exactly work?" is missing: While the "why" is clear, the PR description lacks detail on how nxmutex is changed to a spinlock. Explain the specific code changes made. For example: "This PR replaces the nxmutex_lock call in lib_get_pathbuffer with a spinlock. A new spinlock variable, g_pathbuffer_lock, is declared statically in the file and initialized during system startup." Be precise.
  • Missing "Testing logs before/after change": While it mentions tests passing, it needs to show actual log output demonstrating the fix. Include relevant snippets demonstrating the error before the change and its absence after. Empty code blocks are insufficient.
  • Target details lacking: "local qemu environment" is vague. Specify the QEMU architecture being used (e.g., qemu-arm, qemu-riscv). Mention the specific NuttX configuration (e.g., nsh).

By adding these details, the PR will be much stronger and easier to review.

@hujun260
Copy link
Contributor

->nxmutex_lock <-- hold mutex on assert will trigger an nested exception

Is this exception caused because assert is called within an interrupt?

@xiaoxiang781216
Copy link
Contributor

xiaoxiang781216 commented Nov 15, 2024

->nxmutex_lock <-- hold mutex on assert will trigger an nested exception

Is this exception caused because assert is called within an interrupt?

lib_get_pathbuffer is used in many places, so it's better to the interrupt safe.

@hujun260
Copy link
Contributor

LGTM

@crafcat7 crafcat7 force-pushed the lib_pathbuffer_change_spinlock branch from a986ff4 to 7ceaab7 Compare November 15, 2024 14:16
Summary:
_assert
|
 ->dump_fatal_info
   |
    ->dump_tasks
      |
       ->dump_filelist
         |
          ->files_dumplist
            |
             ->lib_get_pathbuffer
               |
                ->nxmutex_lock      <-- hold mutex on assert will trigger an nested exception

Signed-off-by: chenrun1 <chenrun1@xiaomi.com>
@crafcat7 crafcat7 force-pushed the lib_pathbuffer_change_spinlock branch from 7ceaab7 to f811637 Compare November 15, 2024 14:18
@xiaoxiang781216 xiaoxiang781216 merged commit f9647b4 into apache:master Nov 15, 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.

6 participants