Skip to content

Avoid GCC 15 null-dereference warning in ThreadSafeSignal - #1287

Open
nirwester wants to merge 1 commit into
BehaviorTree:masterfrom
nirwester:fix/gcc15-thread-safe-signal-warning
Open

nirwester wants to merge 1 commit into
BehaviorTree:masterfrom
nirwester:fix/gcc15-thread-safe-signal-warning

Conversation

@nirwester

Copy link
Copy Markdown

GCC 15 emits a (I think) false positive -Wnull-dereference when copying the subscriber vector in ThreadSafeSignal::notify(), breaking downstream builds that enable -Werror, including Nav2 (an example is here).

Copy-constructing the snapshot before moving it into the local vector should avoid the problematic assignment path. Locking, callback behavior, and API/ABI remain unchanged.

Validated with GCC 15.2 at -O2 and -O3, on existing callback and blackboard tests, and ASan/UBSan checks.
If I get a bit of time I can add a report for GCC too.

@facontidavide

Copy link
Copy Markdown
Collaborator

CI is unhappy

@nirwester

Copy link
Copy Markdown
Author

Seem race conditions unrelated from the change. According to Claudio (I didn't have time to try to follow the flow myself):

  1. The test signals startup too early. starts++ happens inside onStart(), before executeTick() publishes RUNNING. The main thread can halt the still-IDLE node, then the in-flight tick overwrites that halt with RUNNING. Forcing this ordering reproduces the failure consistently.
  2. The worker can restart an already halted tree. A diagnostic run captured:
    before_halt=RUNNING after_halt=IDLE at_timeout=RUNNING starts=2
    In tickRoot(), the halt check precedes the wake-up poll. A halt arriving between those operations can be consumed as an ordinary wake-up, causing another tick and restarting the root.
  3. The condition variable can miss a notification. WakeUpSignal::emitSignal() updates its predicate without holding the waiter’s mutex. Notification can arrive between the predicate check and entering the wait. I reproduced the waiter sleeping until its timeout despite receiving a signal; locking the producer’s predicate update prevented that failure.

I also tried to have him propose patches for these, but they looked quite invasive, and I don't want to PR AI rubbish :)

@SteveMacenski

Copy link
Copy Markdown
Contributor

BTW I filed a ticket this would resolve: #1288

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants