Skip to content

fix(tests): name the raise-exception test's signal so macOS builds it - #12076

Merged
adriadam10 merged 1 commit into
boykopovar:mainfrom
011010:fix/macos-raise-exception-signal-name
Oct 29, 2026
Merged

adriadam10 merged 1 commit into
boykopovar:mainfrom
011010:fix/macos-raise-exception-signal-name

Conversation

@011010

@011010 011010 commented Oct 27, 2026

Copy link
Copy Markdown
Contributor

What

guest_raise_exception_tests stops a full macOS build with BUILD_TESTING=ON, and no open pull request covers it:

core/libs/tests/GuestRaiseException.cpp:21:22: error: expected unqualified-id
/Applications/Xcode.app/.../usr/include/sys/signal.h:122:17: note: expanded from macro 'SIGUSR1'

The test declares static constexpr int SIGUSR1 = 30; (from dd2890c1c5). On Windows and Linux that name is free in this translation unit; on macOS the SDK defines SIGUSR1 as a macro in <sys/signal.h>, which libc++ reaches through <stdlib.h> → <sys/wait.h>, so the declaration expands to static constexpr int 30 = 30;.

Rename it to GUEST_SIGUSR1, the name GuestPthreadCreateSuspended.cpp:18 and GuestSignalList.cpp:10-13 already use. The value and all 28 uses stay the same, so no behaviour changes. GuestRaiseNestedSleep.cpp has the same declaration but its target is inside if(WIN32) (libkernel/CMakeLists.txt:1205), so it never reaches a macOS build and is left alone.

Tested

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file
  • No notes, investigation .md files or images added to the repository (attach them to this PR)
  • New third-party code is a submodule built from source
  • Generic behaviour, not specific to one title
  • Depends on: none
  • Fixes: none
  • Unblocks: none
  • GPUs tested: n/a
  • AI-assisted: yes

The test declared its own `static constexpr int SIGUSR1 = 30;`, and the
macOS SDK defines `SIGUSR1` as a macro in <sys/signal.h>, which libc++
reaches through <stdlib.h> and <sys/wait.h>. The declaration then expands
to `static constexpr int 30 = 30;` and the translation unit does not
compile, so `guest_raise_exception_tests` and every full macOS build with
BUILD_TESTING=ON stop there.

Rename it to GUEST_SIGUSR1, the name the other signal tests already use.
The value and every use stay the same.

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed exact head 76c419a. The change consistently renames the test's guest signal constant to GUEST_SIGUSR1, avoiding the macOS SDK macro collision while preserving the value and all call sites. The author reports that the exact x86-64 macOS object, which failed before the rename, now compiles; this is the relevant verification because the Windows and Linux signal name was not conflicting. Hosted Windows and conventions checks pass, and the current-main merge preview and diff check are clean. No test behavior changed. Approved.

@github-actions

github-actions Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.

Pull request Shared files not compared
PR 4630 core/libs/tests/GuestRaiseException.cpp
PR 12682 core/libs/tests/GuestRaiseException.cpp
PR 12688 core/libs/tests/GuestRaiseException.cpp
PR 12700 core/libs/tests/GuestRaiseException.cpp

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I had approved this exact head earlier after reviewing the rename and the reported macOS compile check. The later overlap report flagged shared edits to GuestRaiseException.cpp. I checked the current heads: this PR merges cleanly with main by itself, but pairwise merge-tree checks conflict when combined with both open #11872 and open #11969. Please coordinate an order and declare the required Depends on entries (or rebase after those changes land) so the macro rename is not lost during integration. No issue with the rename itself.

#endif

static constexpr int SIGUSR1 = 30;
static constexpr int GUEST_SIGUSR1 = 30;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This rename is correct for the macOS macro collision. The overlap bot flags #11872 and #11969 as touching this same file, and exact-head pairwise merge-tree checks conflict with each of them. Please document the agreed landing order/dependencies or rebase after the overlapping changes land.

@babyhuey

Copy link
Copy Markdown
Contributor

Windows run of this head 76c419a5728, merged onto main f759e165d9c (clean merge, local only), with the selection CI's skipped Windows product job uses: Windows 11, Ryzen 7 5800X, RTX 3080, WinLibs GCC 15.2, Release, BUILD_TESTING=ON. The tree was configured fresh after the merge, then select_tests.py select / ninja -f select.ninja selected / --target libs / select_tests.py ctest --parallel 4 --timeout 120.

  • Selected 437 tests: 431 pass, 0 fail, 6 skip. No failures.

AI-assisted: yes (Claude Code)

@THEMANnt THEMANnt left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reviewed 76c419a5728dd4b4b992636a50b5fb7d8ab6be30 — approve. The guest signal constant avoids the host SIGUSR1 macro without changing its value or uses.

Windows/GCC 15.2: compiling the exact source with SIGUSR1 defined as 30 passes on head and fails at the constant declaration on base, reproducing the macro collision independently of Apple headers. An exact token-renaming comparison confirms the identifier rename is the only source change, including all uses. This is a syntax/control check, not a macOS SDK build or exception-runtime test. Hosted Build reports success.

@011010

011010 commented Oct 28, 2026

Copy link
Copy Markdown
Contributor Author

@MarcZX10 I checked the overlap again on today's heads, and the picture has changed since the report the bot posted:

PR State today Its own conflicts with main
#11872 CONFLICTING Pthread/src/Thread.cpp, docs/dev/debt/libkernel.md
#11969 CONFLICTING, still stacked on #11872 those two plus File/include/SharedMemory.hpp

Neither can merge as it stands, and neither of them closes this break: both still declare static constexpr int SIGUSR1 = 30; at GuestRaiseException.cpp:21. Declaring Depends on: #11872 would hold a one-line build fix behind two pull requests that cannot merge yet, so I have left Depends on: none.

I did measure the rebase before deciding. On #11872's head 21d5a977f9: take their structure, re-apply the rename. 25 renames, no conflict markers, their 15 Case*() functions intact, the object compiles (30,760 bytes, 0 errors), and merge-tree is clean against #11969 once that rebase is in. I did not push it, because based on #11872 this PR's diff becomes 99 files and 4,517 insertions instead of 1 file and 28 lines, which is not reviewable as one topic.

Current main 17f8dfad51: the full BUILD_TESTING=ON macOS build still fails in 14 objects. GuestRaiseException.cpp is no longer one of them with this head — it builds, and the other 14 are the ones #7631, #7804, #10552, #11447 and #4630 cover.

One more note for the integration order: #4630 renames the same constant, to GuestSigUsr1, and has been conflicting since 12 October; #11249 was closed yesterday in its favour for Exception.cpp. So whichever of the three lands first, the other two need the same one-line rebase — the rename is not lost as long as the rebase is done rather than a side taken.

If you still prefer #11872 first, I will rebase the moment it merges and drop this note. As it stands the branch is unchanged at 76c419a572, the head you both reviewed.

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-check — changes still requested

The old references to #11872 and #11969 are no longer actionable because both are closed. Their replacement PRs #12682 and #12688 are open and now conflict with this PR in GuestRaiseException.cpp; #4630 also remains open and conflicts in that file and the debt document. The current code-only fix still builds according to the author, but its merge order has not been recorded for the current open PRs.

What I checked

  • Refreshed the current PR, its latest author response, and hosted checks.
  • Read the full current diff: it renames the test’s guest signal constant references.
  • Compared the current head with main and ran pairwise merge previews with #12682, #12688, and #4630.
  • Ran git diff --check; no local build/test was run.

Results

Check Result
Current-main merge preview Clean
git diff --check Pass
Pairwise preview with #12682 Conflict in GuestRaiseException.cpp
Pairwise preview with #12688 Conflict in GuestRaiseException.cpp
Pairwise preview with #4630 Conflicts in GuestRaiseException.cpp and docs/dev/debt/libkernel.md
Hosted Windows and conventions Passed
Hosted Linux and specialized jobs Skipped
Author-reported macOS compile result Current head compiled; full build still has unrelated failures
Unedited local merge-check output
$ git rev-list --left-right --count upstream/main...refs/review/upstream-12076
1507 1
$ git merge-tree --write-tree upstream/main refs/review/upstream-12076
cbeb65c637a39342b317710425064c137243040c
$ git merge-tree --write-tree refs/review/upstream-12076 refs/review/upstream-12682
506a7db62c2580ab1668c78a0782a550d6e94d84
CONFLICT (content): core/libs/tests/GuestRaiseException.cpp
$ git merge-tree --write-tree refs/review/upstream-12076 refs/review/upstream-12688
e6d509fe6f02e96f57ea6b41b88fcfdbbd2c5089
CONFLICT (content): core/libs/tests/GuestRaiseException.cpp
$ git merge-tree --write-tree refs/review/upstream-12076 refs/review/upstream-4630
4a41e5a064465a027b0da9ec47061a016d9526af
CONFLICT (content): core/libs/tests/GuestRaiseException.cpp
CONFLICT (content): docs/dev/debt/libkernel.md
$ git diff --stat upstream/main...refs/review/upstream-12076
core/libs/tests/GuestRaiseException.cpp | 56 ++++++++++++++++-----------------
1 file changed, 28 insertions(+), 28 deletions(-)
$ git diff --check upstream/main...refs/review/upstream-12076
(no output; exit status 0)

Findings

Blocking

Please agree an order with the authors of the currently open #12682, #12688 and #4630, then record it in the relevant Depends on checklists or explain the chosen direct rebase strategy. The previous #11872/#11969 conflict report is obsolete; the replacements are separate open PRs, so resolve against their current heads.

Non-blocking

I found no new issue with the signal-constant rename itself. The author reports the current head removes this test file from the macOS compile failures. The reported full macOS build still has unrelated failures.

Sources

Not verified

I did not independently reproduce the author’s macOS build results.

@adriadam10
adriadam10 merged commit 1b580f1 into boykopovar:main Oct 29, 2026
10 checks passed
@adriadam10

Copy link
Copy Markdown
Collaborator

Checked that the diff is only the SIGUSR1 → GUEST_SIGUSR1 rename (28 uses, same value, no remaining SIGUSR1 in the file), the name matches GuestSignalList.cpp and GuestPthreadCreateSuspended.cpp, and the PR merges cleanly with main. The overlap with #4630, #12682 and #12688 is theirs to resolve on rebase, as each still declares the colliding name; merged.

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.

5 participants