[Bug target/62109] __gthr_i486_lock_cmp_xchg missing clobber
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62109 --- Comment #14 from David --- I understand that stage 3 is now closed too. I don't have svn write access, so I can't check this in myself.
[Bug target/62109] __gthr_i486_lock_cmp_xchg missing clobber
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62109 --- Comment #13 from David --- Rumor has it that Phase 1 may be closing soon. Is there something else I need to do here?
[Bug target/62109] __gthr_i486_lock_cmp_xchg missing clobber
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62109 David changed: What|Removed |Added Attachment #33302|0 |1 is obsolete|| --- Comment #12 from David --- Created attachment 36577 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=36577=edit Update patch per comment 11 Instead of just adding the clobber, this second patch removes the whole routine that was only needed on win95. It also removes the (no longer used) define for __GTHREAD_I486_INLINE_LOCK_PRIMITIVES and updates the comment. Note that I do not have commit privileges to svn, which is why I post this here rather than to the patches ml.
[Bug target/62109] __gthr_i486_lock_cmp_xchg missing clobber
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62109 --- Comment #11 from Kai Tietz --- (In reply to David from comment #10) > (In reply to Kai Tietz from comment #5) > > This patch is clear stage 1 material. > > We're in stage 1. Is it time? Sure, patch for it is welcome. > The patch adds the memory clobber, but I think the conclusion here was to > remove __gthr_i486_lock_cmp_xchg since it only applies to (the no longer > supported) win95. Right, I prefer this.
[Bug target/62109] __gthr_i486_lock_cmp_xchg missing clobber
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62109 --- Comment #10 from David --- (In reply to Kai Tietz from comment #5) > This patch is clear stage 1 material. We're in stage 1. Is it time? The patch adds the memory clobber, but I think the conclusion here was to remove __gthr_i486_lock_cmp_xchg since it only applies to (the no longer supported) win95.
[Bug target/62109] __gthr_i486_lock_cmp_xchg missing clobber
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62109 --- Comment #9 from Kai Tietz ktietz at gcc dot gnu.org --- (In reply to David from comment #8) (In reply to Kai Tietz from comment #7) The first code block in comment #6 is what is in the code now. As you can see, it already has the #define you are describing. I don't understand what you mean by change it to this, since it is already there. Are you suggesting we delete the entire #ifdef __GTHREAD_I486_INLINE_LOCK_PRIMITIVES block and replace it with the single #define? Right, this I suggest. I would be ok with that. Using the #error (the second code block in comment #6) seemed like a more backward-compatible way to do this, since it would tell people what has happened and what to do to fix it rather than (silently) assuming we know what they want to do. If a target doesn't provide the Interlocked-API, build with fail caused by it. I don't think we need to error out on such cases. (We don't try to cover win 3.14 incompatibilities, so why we should start for Windows 9X?) But I am ok with either of these two solutions.
[Bug target/62109] __gthr_i486_lock_cmp_xchg missing clobber
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62109 --- Comment #8 from David gccbugzilla at limegreensocks dot com --- (In reply to Kai Tietz from comment #7) The first code block in comment #6 is what is in the code now. As you can see, it already has the #define you are describing. I don't understand what you mean by change it to this, since it is already there. Are you suggesting we delete the entire #ifdef __GTHREAD_I486_INLINE_LOCK_PRIMITIVES block and replace it with the single #define? I would be ok with that. Using the #error (the second code block in comment #6) seemed like a more backward-compatible way to do this, since it would tell people what has happened and what to do to fix it rather than (silently) assuming we know what they want to do. But I am ok with either of these two solutions.
[Bug target/62109] __gthr_i486_lock_cmp_xchg missing clobber
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62109 --- Comment #7 from Kai Tietz ktietz at gcc dot gnu.org --- I agree that we change it to #define __GTHR_W32_InterlockedCompareExchange InterlockedCompareExchange not sure if we actually should error out here at all. We might want to remove instead the use of __GTHREAD_I486_INLINE_LOCK_PRIMITIVES completely.
[Bug target/62109] __gthr_i486_lock_cmp_xchg missing clobber
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62109
--- Comment #6 from David gccbugzilla at limegreensocks dot com ---
Actually, the code already uses InterlockedCompareExchange. UNLESS users
explicitly tell it not to:
#ifdef __GTHREAD_I486_INLINE_LOCK_PRIMITIVES
static inline long
__gthr_i486_lock_cmp_xchg(long *__dest, long __xchg, long __comperand)
{
long result;
__asm__ __volatile__ (\n\
lock\n\
cmpxchg{l} {%4, %1|%1, %4}\n
: =a (result), =m (*__dest)
: 0 (__comperand), m (*__dest), r (__xchg)
: cc);
return result;
}
#define __GTHR_W32_InterlockedCompareExchange __gthr_i486_lock_cmp_xchg
#else /* __GTHREAD_I486_INLINE_LOCK_PRIMITIVES */
#define __GTHR_W32_InterlockedCompareExchange InterlockedCompareExchange
#endif /* __GTHREAD_I486_INLINE_LOCK_PRIMITIVES */
Since we are talking about postponing this to stage 1 (which I do not object
to), what if we change this to something like:
#ifdef __GTHREAD_I486_INLINE_LOCK_PRIMITIVES
#error __GTHREAD_I486_INLINE_LOCK_PRIMITIVES is no longer supported. Remove
this definition to use system calls for Win98 and above.
#else /* __GTHREAD_I486_INLINE_LOCK_PRIMITIVES */
#define __GTHR_W32_InterlockedCompareExchange InterlockedCompareExchange
#endif /* __GTHREAD_I486_INLINE_LOCK_PRIMITIVES */
Then wait to see if anyone cares.
I am ok with either fixing it (by adding the memory clobber) or removing it
(since the problem it was intended to fix only happens on OSs that aren't
supported anymore). But leaving it as-is leaves open the possibility that
people are using this without even knowing it (and getting
nearly-impossible-to-track-down timing problems). Or that someone might
copy/paste this code into their own projects.
[Bug target/62109] __gthr_i486_lock_cmp_xchg missing clobber
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62109 Kai Tietz ktietz at gcc dot gnu.org changed: What|Removed |Added Status|UNCONFIRMED |NEW Last reconfirmed||2015-03-17 Ever confirmed|0 |1 --- Comment #5 from Kai Tietz ktietz at gcc dot gnu.org --- This patch is clear stage 1 material. Patch itself looks ok, but raises a completely different question. Modern versions of mingw provides all Interlocked-functions, so we might want to use this instead. The argument about Win95/98 compatibility is actually pretty borged, as we have already in some of gcc's target-libraries strong requirements of using at least NT 4.0, and/or even more modern.
[Bug target/62109] __gthr_i486_lock_cmp_xchg missing clobber
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62109 --- Comment #4 from David gccbugzilla at limegreensocks dot com --- (In reply to Mikael Pettersson from comment #3) Do you have a test case which fails without your patch? I do not. But this is a by definition thing. This instruction is intended to emulate the Windows InterlockedCompareExchange instruction (see the comments above the code). And all Windows Interlocked* instructions perform an implicit compiler barrier. After all, it does no good for the 'lock' prefix to prevent the processor from re-ordering instructions if the compiler does it while building the code. Moving loads or stores past this statement makes a mess of thread synchronization. That said, this instruction was only intended to be used on Win95 (neither NT nor Win98 need it). Not exactly a common platform these days. But it's possible someone is still using this, or will copy this asm block for their own use, so it should still be correct.
[Bug target/62109] __gthr_i486_lock_cmp_xchg missing clobber
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62109 --- Comment #3 from Mikael Pettersson mikpelinux at gmail dot com --- Do you have a test case which fails without your patch?
[Bug target/62109] __gthr_i486_lock_cmp_xchg missing clobber
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62109 --- Comment #2 from David gccbugzilla at limegreensocks dot com --- Attaching a patch was just the clearest way I knew to show the problem. My hope was that posting it here would allow someone who knew how to submit patches to take this the rest of the way.
[Bug target/62109] __gthr_i486_lock_cmp_xchg missing clobber
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62109 Andrew Pinski pinskia at gcc dot gnu.org changed: What|Removed |Added Target||win32 --- Comment #1 from Andrew Pinski pinskia at gcc dot gnu.org --- Patches should be sent to gcc-patches@ after reading https://gcc.gnu.org/contribute.html.
