-
Notifications
You must be signed in to change notification settings - Fork 70
[GEN][ZH] Implement interlocked compat helper functions #887
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
// VC6 compatible overloaded interlocked functions - No native 16 or 64 bit functions supported, use with care | ||
#if defined(_MSC_VER) && _MSC_VER < 1300 | ||
|
||
inline AtomicType16 interlockedIncrement(volatile AtomicType16 *addend) { return InterlockedIncrement((AtomicType32*)addend); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you try what chatgpt has suggested?
short AtomicIncrement16(volatile short* ptr) {
__asm {
mov ecx, ptr ; Load address of the value
mov ax, 1 ; Value to add
lock xadd [ecx], ax ; Atomically add and exchange
inc ax ; ax = original + 1
movzx eax, ax ; Zero extend to return a clean value
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tried a few different variations but can't get these to build in VS22 to test. But i don't think modern VS supports the type of ASM anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it build in VC6?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it build in VC6?
it appears to from the pipeline passing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could test it by launching 2 threads, have them race with counters, and then check if interlocked increment causes no wrong values, while non-interlocked increment causes wrong values. Edit: that is perhaps easier said than done. I am not sure what the verify condition is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To test that your atomic 16-bit increment works correctly, you need to simulate a concurrent multithreaded environment where multiple threads increment the same shared counter. If the operation is truly atomic:
- The final counter value should equal the total number of increments.
- No increments should be lost due to race conditions.
Here's a complete example using Win32 threads (compatible with VC6) to test atomic increment:
✅ Test Program for AtomicIncrement16
#include <windows.h>
#include <stdio.h>
volatile short counter = 0; // Shared counter
const int NUM_THREADS = 10;
const int INCREMENTS_PER_THREAD = 10000;
DWORD WINAPI ThreadProc(LPVOID lpParam) {
for (int i = 0; i < INCREMENTS_PER_THREAD; ++i) {
__asm {
mov ecx, offset counter
mov ax, 1
lock xadd [ecx], ax
}
}
return 0;
}
int main() {
HANDLE threads[NUM_THREADS];
for (int i = 0; i < NUM_THREADS; ++i) {
threads[i] = CreateThread(NULL, 0, ThreadProc, NULL, 0, NULL);
}
WaitForMultipleObjects(NUM_THREADS, threads, TRUE, INFINITE);
for (int i = 0; i < NUM_THREADS; ++i) {
CloseHandle(threads[i]);
}
printf("Expected: %d\n", NUM_THREADS * INCREMENTS_PER_THREAD);
printf("Actual: %d\n", counter);
return 0;
}
🧪 What This Does:
- Launches
NUM_THREADS
threads. - Each thread performs
INCREMENTS_PER_THREAD
atomic increments. - Total expected:
NUM_THREADS * INCREMENTS_PER_THREAD
. - Final value of
counter
is printed.
✅ Expected Output (if atomicity is correct):
Expected: 100000
Actual: 100000
If the increment is not atomic, you will see a lower actual value, because increments would be lost due to race conditions.
🛠 Notes:
volatile
prevents compiler optimization but does not ensure atomicity — your inlinelock xadd
does.- VC6 and older CPUs (Pentium/486/386) can run this just fine if the hardware supports
LOCK XADD
(386+). - For older 8086-like processors, you'd need to simulate atomicity with interrupt disabling or similar — not practical for multitasking systems.
Let me know if you want to test this using lock inc
instead, or if you're targeting SMP/multicore emulators or real hardware!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you increase the number of 'INCREMENTS_PER_THREAD' later on?
'counter' is a short, so it's likely to have a 16 bit width. So the counter would overflow (and since it's a signed type, that'd be undefined behavior, but that's beside the point).
I don't understand how your output can be 100'000.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will have a look tomorrow when i have some more time, will need to properly setup VC6 locally too.
f236da0
to
6f96528
Compare
Updated based on suggestions. |
6f96528
to
62edff6
Compare
This PR implements operating system, cross compatible, interlocked increment and decrement functions.
This is a follow up PR from unifying ASCII string for future use when replacing critical sections.
NOTE: For VC6 there are asm based functions for 16 and 64bit instead of relying on casting to the 32bit variant.