Skip to content

Conversation

@Blackhex
Copy link
Member

@Blackhex Blackhex commented May 26, 2025

Implements stabilize_sig_stack  routine for AArch64 that is used both in setjmp and longjmp routines implementation. Please, refer to x64 implementation for comparison.

Validated by https://github.com/Windows-on-ARM-Experiments/mingw-woarm64-build/actions/runs/15304776330

@Blackhex Blackhex requested a review from Copilot May 26, 2025 08:23
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Implements the stabilize_sig_stack routine on AArch64 to mirror the x64 behavior used by setjmp/longjmp.

  • Adds AArch64 assembly prologue/epilogue and register saves/restores
  • Implements an atomic lock loop, incy g counter management, and conditional signal handling
  • Calls into call_signal_handler and handles cleanup before returning the TLS pointer
Comments suppressed due to low confidence (3)

winsup/cygwin/scripts/gendef:450

  • This add instruction overwrites the TLS pointer in x0 instead of preparing the argument. It should likely be add x1, x10, x1 (or use a fresh register) to compute the address without clobbering x0.
add x0, x0, x1

winsup/cygwin/scripts/gendef:455

  • This uses x1 instead of the offset in x11, so the address of incy g will be calculated incorrectly. It should read add x11, x10, x11.
add x11, x10, x1

winsup/cygwin/scripts/gendef:408

  • There’s no visible test coverage for the new stabilize_sig_stack routine; consider adding a targeted test or integration scenario to validate lock acquisition, signal handling, and stack restoration.
.seh_proc stabilize_sig_stack

@Blackhex Blackhex self-assigned this May 26, 2025
@Blackhex Blackhex force-pushed the stabilize-sig-stack branch from 7d28934 to bdf0148 Compare May 26, 2025 13:13
@Blackhex Blackhex marked this pull request as draft May 27, 2025 06:29
@Blackhex Blackhex changed the title Implement stabilize_sig_stack WIP: Implement stabilize_sig_stack May 27, 2025
@Blackhex Blackhex requested a review from Copilot May 27, 2025 07:09
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Implements the AArch64 version of stabilize_sig_stack to mirror existing x64 logic for use in setjmp/longjmp.

  • Adds function prologue/epilogue and per-thread lock around signal processing
  • Manages the incyg counter and invokes the C++-style signal handler in a loop
  • Returns the TLS base pointer in x0 and releases locks appropriately
Comments suppressed due to low confidence (4)

winsup/cygwin/scripts/gendef:422

  • [nitpick] Swapping with the same register for source and destination works but is subtle. For clarity and to avoid surprises, use separate registers for the value to store and the old value (e.g. mov w8, #1; swp w9, w8, [x11]).
swp	w9, w9, [x11]		// try to acquire the lock using atomic swap

winsup/cygwin/scripts/gendef:410

  • [nitpick] This new AArch64 routine appears untested. Adding unit or integration tests for stabilize_sig_stack would help catch regressions and verify signal-handling behavior.
// prologue

winsup/cygwin/scripts/gendef:455

  • The comment says "store incremented value back," but this is after decrementing the counter. It should read "store decremented value back."
str	w9, [x11]		// store incremented value back

winsup/cygwin/scripts/gendef:435

  • The addressing mode ldr w9, [x10, =_cygtls.current_sig] is not valid AArch64 syntax. Use a separate literal load and offset addition (e.g. ldr x11, =_cygtls.current_sig; add x11, x10, x11; ldr w9, [x11]) to compute the correct address.
ldr	w9, [x10, =_cygtls.current_sig]	// load current value of current_sig

@Blackhex Blackhex force-pushed the stabilize-sig-stack branch from 4a6fbc1 to 2e7d7e9 Compare May 27, 2025 12:35
@Blackhex Blackhex requested a review from Copilot May 27, 2025 12:37
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds the AArch64 implementation of the stabilize_sig_stack routine—used by both setjmp and longjmp—to manage TLS-based signal locking and dispatch.

  • Introduces a new stabilize_sig_stack function in assembly for ARM64
  • Implements a spinlock on stacklock, increments/decrements the incyg counter, and invokes the signal handler if needed
  • Pushes/restores callee-saved registers and returns the TLS base in x0
Comments suppressed due to low confidence (2)

winsup/cygwin/scripts/gendef:414

  • [nitpick] The comment could be clearer; consider rephrasing to "Save x13 (scratch) and x14 to maintain stack alignment" to concisely explain why both registers are pushed.
stp\tx13, x14, [sp, #-0x10]!\t\t// save x13 register used next and x14 just to keep stack orientation and alignment

winsup/cygwin/scripts/gendef:471

  • The comment says "store incremented value back" but this path actually decrements the counter. Update it to "store decremented value back" for accuracy.
str\tw9, [x11]\t\t// store incremented value back

@Blackhex Blackhex marked this pull request as ready for review May 27, 2025 13:26
@Blackhex Blackhex changed the title WIP: Implement stabilize_sig_stack Implement stabilize_sig_stack May 27, 2025
@Blackhex Blackhex force-pushed the stabilize-sig-stack branch from 2e7d7e9 to 58d8814 Compare May 28, 2025 11:47
@Blackhex Blackhex force-pushed the stabilize-sig-stack branch from 58d8814 to b402a39 Compare May 28, 2025 15:47
mov x0, x10 // return TLS address in x0 (return value)
// epilogue
.seh_startepilogue
Copy link
Member

Choose a reason for hiding this comment

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

.seh commands are not needed here

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, thanks. I believe that you know far more about this topic. Could you possibly share link to some documentation where I can learn why?

Copy link
Member

Choose a reason for hiding this comment

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

The only available documentation is https://learn.microsoft.com/en-us/cpp/build/arm64-exception-handling?view=msvc-170#unwind-codes
and binutils/gcc source code.

in short, for a single instruction like this, not having seh_endepilogue or duplicating seh commands is not changing anything. Both options are acceptable.

Copy link
Member Author

Choose a reason for hiding this comment

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

From this answer I'd deduce that the fact that it has no effect is an implementation detail of GCC/binutils so it makes sense to keep it for consistency with the prologue and accross the compilers (Regardled that Cygwin is build only with GCC, one never know who will copy&paste this code elsewhere).

Copy link
Member

Choose a reason for hiding this comment

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

Coming back to this thread, it is worth mentioning again that .seh_startepilogue is not needed here and unwinding should be fully covered by prolog. Current implementation emits duplicated unwinding codes.

@Blackhex Blackhex requested a review from Copilot May 28, 2025 17:26
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements the stabilize_sig_stack routine for AArch64, mirroring the x64 implementation for improved support in setjmp/longjmp functionality.

  • Introduces an assembly implementation for signal stack stabilization.
  • Adds atomic operations for lock handling and incyg counter management in the new routine.

@Blackhex Blackhex force-pushed the stabilize-sig-stack branch from b402a39 to 424d171 Compare May 28, 2025 17:57
@Blackhex Blackhex requested a review from Copilot May 28, 2025 18:03
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements the AArch64 version of the stabilize_sig_stack routine used by setjmp/longjmp, following the existing x64 implementation.

  • Implements a new signal stack stabilization routine with atomic lock acquisition for thread local storage.
  • Includes loop-based lock acquisition with yield and signal handler invocation.

@Blackhex Blackhex force-pushed the stabilize-sig-stack branch 2 times, most recently from c79dc35 to 7143206 Compare May 29, 2025 13:43
@Blackhex Blackhex force-pushed the stabilize-sig-stack branch from 7143206 to 69e08ec Compare May 29, 2025 18:27
@Blackhex Blackhex requested a review from eukarpov May 29, 2025 18:34
Base automatically changed from jmpbuff-length to woarm64 May 29, 2025 18:35
@Blackhex Blackhex force-pushed the stabilize-sig-stack branch from 69e08ec to ace3e4e Compare May 29, 2025 18:39
@Blackhex Blackhex force-pushed the stabilize-sig-stack branch from ace3e4e to 1ffacac Compare May 29, 2025 19:41
@Blackhex Blackhex force-pushed the stabilize-sig-stack branch from af8957d to f2de0a5 Compare July 3, 2025 13:54
@Blackhex Blackhex force-pushed the woarm64 branch 3 times, most recently from ccb77cc to aa712ee Compare July 15, 2025 19:49
@Blackhex Blackhex force-pushed the stabilize-sig-stack branch from f2de0a5 to 323c28f Compare July 17, 2025 15:15
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.

4 participants