Skip to content

Fix Windows atomic ops to return pre-operation value (match Linux/C11 semantics)#812

Merged
ithewei merged 3 commits intomasterfrom
copilot/fix-atomic-operations-windows
Mar 11, 2026
Merged

Fix Windows atomic ops to return pre-operation value (match Linux/C11 semantics)#812
ithewei merged 3 commits intomasterfrom
copilot/fix-atomic-operations-windows

Conversation

Copy link
Contributor

Copilot AI commented Mar 10, 2026

ATOMIC_INC/ATOMIC_DEC/ATOMIC_ADD/ATOMIC_SUB return the new value on Windows (InterlockedIncrement/InterlockedDecrement/InterlockedAdd) but the old value on Linux (__sync_fetch_and_add) and C11 (atomic_fetch_add). This breaks cross-platform code like reference counting:

// Works on Linux (returns old value), broken on Windows (returns new value)
if (ATOMIC_DEC(&ref->refcnt) == 1) {
    release_cb(ref);
}
  • Reorder #ifdef checks so GCC-based compilers (including MinGW) use __sync_fetch_and_add builtins via a direct __GNUC__ version check, instead of the broken __GNUC_PREREQ macro (which always evaluates to 0 on MinGW since it lacks glibc's <features.h>)
  • For pure MSVC (non-GCC), replace InterlockedAdd/InterlockedIncrement/InterlockedDecrement with InterlockedExchangeAdd, which returns the original value
  • Fix ATOMIC_SUB macro parenthesization: -(n) instead of -n
Original prompt

This section details on the original issue you should resolve

<issue_title>hatomic.h windows下原子操作错误</issue_title>
<issue_description>不同长度的整型使用这些原子操作接口时得到的结果不正确。
比如:atomic_flag_test_and_set函数(内部代码:InterlockedCompareExchange((LONG*)&p->_Value, 1, 0))调用后并不能让atomic_flag置1。

还有这些原子操作接口在不同平台下的返回值不同。比如我用来实现引用计数的代码:

if (ATOMIC_DEC(&ref->refcnt) == 1)
{
    release_cb(ref);
}

在linux下,ATOMIC_DEC返回的减少之前的值,而在windows下返回的是减少之后的值,这导致这段代码在Windows下不正确。</issue_description>

<agent_instructions>Please analyze and respond to this issue.</agent_instructions>

Comments on the Issue (you are @copilot in this section)

@ithewei libhv里确实没有用到ATOMIC_INC、ATOMIC_DEC的返回值,这里windows下可以考虑使用InterlockedAdd,而不是InterlockedIncrement/InterlockedDecrement,应该就可以了吧

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

…value

Replace InterlockedAdd/InterlockedIncrement/InterlockedDecrement with
InterlockedExchangeAdd which returns the value before the operation,
matching the behavior of __sync_fetch_and_add (GCC) and
atomic_fetch_add (C11).

Also fix ATOMIC_SUB macro parenthesization: -(n) instead of -n.

Co-authored-by: ithewei <26049660+ithewei@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix atomic operations issue on Windows platform Fix Windows atomic ops to return pre-operation value (match Linux/C11 semantics) Mar 10, 2026
@ithewei ithewei marked this pull request as ready for review March 11, 2026 03:34
Copilot AI review requested due to automatic review settings March 11, 2026 03:34
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

Aligns Windows atomic operation return values with Linux (__sync_fetch_and_*) and C11 (atomic_fetch_*) semantics by making ATOMIC_* return the pre-operation value, fixing cross-platform correctness for patterns like reference counting.

Changes:

  • Switch Windows ATOMIC_ADD/ATOMIC_SUB to InterlockedExchangeAdd (returns original value).
  • Implement Windows ATOMIC_INC/ATOMIC_DEC via InterlockedExchangeAdd with +1/-1 (returns original value).
  • Fix ATOMIC_SUB negation parenthesization (-(n)).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

base/hatomic.h Outdated
Comment on lines +55 to +58
#define ATOMIC_ADD InterlockedExchangeAdd
#define ATOMIC_SUB(p, n) InterlockedExchangeAdd(p, -(n))
#define ATOMIC_INC(p) InterlockedExchangeAdd(p, 1)
#define ATOMIC_DEC(p) InterlockedExchangeAdd(p, -1)
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

This change intentionally alters Windows return semantics for ATOMIC_ADD/SUB/INC/DEC to match fetch_* (returning the pre-operation value). There’s a unit test for hatomic, but it only prints values and doesn’t assert these semantics; please add an explicit regression test that verifies both the returned value and the stored counter value for inc/dec/add/sub (especially on Windows) so this doesn’t regress again.

Copilot uses AI. Check for mistakes.
@ithewei
Copy link
Owner

ithewei commented Mar 11, 2026

@copilot InterlockedExchangeAdd 在mingw平台上有吗,担心影响mingw编译

Reorder #ifdef checks so GCC-based compilers (including MinGW) use
__sync_fetch_and_add builtins via a direct __GNUC__ version check,
instead of relying on __GNUC_PREREQ which is broken on MinGW.

Only pure MSVC now uses InterlockedExchangeAdd for atomic operations.
This completely avoids any MinGW compatibility concerns with Windows
Interlocked functions.

Co-authored-by: ithewei <26049660+ithewei@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Mar 11, 2026

@copilot InterlockedExchangeAdd 在mingw平台上有吗,担心影响mingw编译

InterlockedExchangeAdd is available on MinGW (it's been in Windows API since XP, actually more compatible than the previous InterlockedAdd). But to fully avoid the concern, in 0f3ec91 I reordered the #ifdef checks: GCC-based compilers (including MinGW) now use __sync_fetch_and_add builtins via a direct __GNUC__ version check instead of the broken __GNUC_PREREQ macro (which always evaluates to 0 on MinGW since it lacks glibc's <features.h>). Only pure MSVC falls through to the InterlockedExchangeAdd path.

@ithewei ithewei merged commit 35342db into master Mar 11, 2026
12 checks passed
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.

hatomic.h windows下原子操作错误

3 participants