Repository navigation
Bug 1496426 - Use GCC atomic builtins for Linux x86/x86_64 - #65
Conversation
7b8bf5e to
2d618d8
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The compiler guards permit toolchains and i386 targets that may not support the selected atomic builtins.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Replaces Linux x86/x86_64 assembly atomics with GCC-compatible builtins.
Changes:
- Adds
__sync_*atomic implementations. - Removes obsolete assembly sources.
- Updates generated and source configure scripts.
| File | Description |
|---|---|
pr/include/md/_linux.h |
Defines builtin atomic operations. |
configure.in |
Stops selecting x86 assembly sources. |
configure |
Mirrors the configuration change. |
pr/src/md/unix/os_Linux_x86.s |
Deletes 32-bit assembly atomics. |
pr/src/md/unix/os_Linux_x86_64.s |
Deletes 64-bit assembly atomics. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #65 +/- ##
==========================================
+ Coverage 42.20% 48.91% +6.70%
==========================================
Files 70 93 +23
Lines 12071 17828 +5757
Branches 2261 2261
==========================================
+ Hits 5095 8721 +3626
- Misses 6126 8687 +2561
+ Partials 850 420 -430
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
58e5448 to
52d0206
Compare
nspr4 backport check
Alone: cherry-picks onto nspr4 by itself. In order: cherry-picks onto nspr4 after the earlier commits, skipping any that conflict. Verdict: needs review abidiff reports |
github-actions
Bot
added
nspr4: needs review
and removed
nspr4: not backportable
labels
| #endif | ||
|
|
||
| #if defined(__i386__) | ||
| #if defined(__GNUC__) && defined(__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4) |
There was a problem hiding this comment.
does Firefox's config/external/nspr/pr/moz.build still list os_Linux_x86.s / os_Linux_x86_64.s?
if so, it needs updating in the same landing, otherwise the next NSPR import breaks the build
There was a problem hiding this comment.
Yes, moz.build:31 and :34 still list them. They need to be removed when NSPR with this change is vendored.
Match ppc64/mips/riscv/loongarch: use `__sync_*` builtins instead of hand-written asm, requiring `__GNUC__` (and `__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4` for i386, matching `pratom.h`). Stop building the now-unused `.s` files. Matches Arch and RHEL packaging patches. https://bugzilla.mozilla.org/show_bug.cgi?id=1496426 Needs a perf/try comparison before merging.
52d0206 to
b1e546e
Compare
Match ppc64/mips/riscv/loongarch: use
__sync_*builtins instead of hand-written asm, requiring__GNUC__(and__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4for i386, matchingpratom.h). Stop building the now-unused.sfiles. Matches Arch and RHEL packaging patches.https://bugzilla.mozilla.org/show_bug.cgi?id=1496426
Needs a perf/try comparison before merging.