[Bug target/62109] New: __gthr_i486_lock_cmp_xchg missing clobber

[Bug target/62109] New: __gthr_i486_lock_cmp_xchg missing clobber

[gccbugzilla at limegreensocks dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62109

            Bug ID: 62109
           Summary: __gthr_i486_lock_cmp_xchg missing clobber
           Product: gcc
           Version: 4.10.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: gccbugzilla at limegreensocks dot com

Created attachment 33302
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=33302&action=edit
Update __gthr_i486_lock_cmp_xchg to add memory clobber

InterlockedCompareExchange is an implicit memory barrier.  Such being the case,
the asm needs to use the "memory" clobber.

[Bug target/62109] __gthr_i486_lock_cmp_xchg missing clobber

[pinskia at gcc dot gnu.org]

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.

[Bug target/62109] __gthr_i486_lock_cmp_xchg missing clobber

[gccbugzilla at limegreensocks dot com]

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

[mikpelinux at gmail dot com]

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

[gccbugzilla at limegreensocks dot com]

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

[ktietz at gcc dot gnu.org]

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

[gccbugzilla at limegreensocks dot com]

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

[ktietz at gcc dot gnu.org]

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

[gccbugzilla at limegreensocks dot com]

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

[ktietz at gcc dot gnu.org]

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

[gccbugzilla at limegreensocks dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62109

--- Comment #10 from David <gccbugzilla at limegreensocks dot com> ---
(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

[ktietz at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62109

--- Comment #11 from Kai Tietz <ktietz at gcc dot gnu.org> ---
(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

[gccbugzilla at limegreensocks dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62109

David <gccbugzilla at limegreensocks dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #33302|0                           |1
        is obsolete|                            |

--- Comment #12 from David <gccbugzilla at limegreensocks dot com> ---
Created attachment 36577
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=36577&action=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 libgcc/62109] __gthr_i486_lock_cmp_xchg missing clobber

[jyong at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62109

jyong at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED
                 CC|                            |jyong at gcc dot gnu.org

--- Comment #15 from jyong at gcc dot gnu.org ---
Pushed the patch as dcf8fe1eeab668a4d24bcc4baa3ad185dbf1b5e0, thanks David.