[bug #33392] Multi-thread bug in NSObject retain and release

[Riccardo Mottola]

Follow-up Comment #9, bug #33392 (project gnustep):

hmm it would be nice to know the status of this. I admit, the "oldest" cpu I
have is i586, where we work but I don't have a thread-test to stress-test it.
Also, while probabil nobody uses a "real" i486 anymore (or even few real
pentiums) there are compatible, recent cpus which are reasonaby fast but which
implement only a more limited x86 instruction set (e.g. my 800Mhz Vortex CPU
is "only" an i586 instruction-wise)

I would rather not force a specific CPU version in during building, since a
package maintainer might want to set it to be consistent with the rest of the
system

___

Reply to this item at:

  

___
  Message sent via/by Savannah
  http://savannah.gnu.org/


___
Bug-gnustep mailing list
Bug-gnustep@gnu.org
https://lists.gnu.org/mailman/listinfo/bug-gnustep

[bug #33392] Multi-thread bug in NSObject retain and release

[Niels Grewe]

Follow-up Comment #8, bug #33392 (project gnustep):

I just (r33134) added a check to base that performs the following checks
before enabling USE_ATOMIC_BUILTINS:

(a) Whether the compiler understands the Itanium style __sync_* intrinsics.
(b) Whether we are targeting an i586 or later processor (if so, we set the
-march=i568 flag).
(c) Whether we need to explicitly link the static libgcc.

I concur that doing something like (b) in gnustep-make is probably a good
idea, but I think we should have that check here as a stop-gap measure because
if you have a gcc version built for i686 (like my Ubuntu VM, for example), the
libgcc will have been compiled without any atomic ops stuff.

It would be nice if somebody could check whether the change has the desired
effect on an i486 machine.

Cheers,

Niels

___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Savannah
  http://savannah.gnu.org/


___
Bug-gnustep mailing list
Bug-gnustep@gnu.org
https://lists.gnu.org/mailman/listinfo/bug-gnustep

Re: [bug #33392] Multi-thread bug in NSObject retain and release

[Jonathan Olson]

Richard,

I don't have a small example.  The application is a relatively large 
multi-threaded app server which supports a couple hundred TCP/IP sockets.  This 
typically crashed about every 30-60 minutes.

For x86, the fix could use the xaddl instruction which is an atomic exchange 
and increment instruction.  Note that the other architectures (mips, powerpc, 
68k) defined in NSObject.m all share the same bug, so you should fix these also.

Possibly, you can use a recent version of gcc to generate the instruction 
sequence for each CPU.  For example, compiling the following program generates 
the following for x86.

#include 

int
main(int argc, char **argv)
{
int lock = 0;
int lock1 = __sync_fetch_and_add(&lock, 1);
int lock2 = __sync_fetch_and_sub(&lock, 1);
fprintf(stderr, "lock = %d %d %d\n", lock, lock1, lock2);
return 0;
}

_main:
LFB3:
pushq   %rbp
LCFI0:
movq%rsp, %rbp
LCFI1:
subq$16, %rsp
LCFI2:
movl$0, -4(%rbp)
leaq-4(%rbp), %rax
movl$1, %ecx
lock
xaddl   %ecx, (%rax)
movl$-1, %r8d
lock
xaddl   %r8d, (%rax)
movl-4(%rbp), %edx
movq___stderrp@GOTPCREL(%rip), %rax
movq(%rax), %rdi
leaqLC0(%rip), %rsi
xorl%eax, %eax
call_fprintf
xorl%eax, %eax
leave
ret

On May 25, 2011, at 2:32 AM, Richard Frith-Macdonald wrote:

> Update of bug #33392 (project gnustep):
> 
>  Status:None => Fixed  
> Open/Closed:Open => In Test
> 
>___
> 
> Follow-up Comment #1:
> 
> Thanks ... the window for the bug to occur must be small for it not to have
> shown up in testing before now.  Do you have a small test program to
> demonstrate it?
> 
> Anyway, after spending several hours research on he web I hope I've got the
> correct fix for this (I'm not at all familiar with x86 assembler though) and
> have added it to svn trunk.
> 
> Please could you try this latest code and let me know if it's correct.
> 
> 
>___
> 
> Reply to this item at:
> 
>  
> 
> ___
>  Message sent via/by Savannah
>  http://savannah.gnu.org/
> 


___
Bug-gnustep mailing list
Bug-gnustep@gnu.org
https://lists.gnu.org/mailman/listinfo/bug-gnustep

[bug #33392] Multi-thread bug in NSObject retain and release

[Nicola Pero]

Follow-up Comment #7, bug #33392 (project gnustep):

Yes. :-)

It may be slightly more complicated but we'll need to address 
the complication. :-(

I think we'd like gnustep-make to generally add -march=xxx 
(where "xxx" is something more recent than i386, a CPU released 
24 years ago) but make it configurable when you configure 
gnustep-make.

I'd expect the default, for a i686 machine, would be 
-march=i686.  As the Pentium Pro was introduced in 1995, code 
built using -march=i686 should run on all ix86 CPUs produced in 
the last 16 years or so, which should probably be safe enough as 
a default ?

And, if you're building for your own machine, you could use -march=native to
get max performance.

Presumably gnustep-base's configure would then need to do the 
check that Niels was mentioning, but also taking into account the -march=xxx
flag added by gnustep-make.

Does it make sense as a plan ?

Thanks

___

Reply to this item at:

  

___
  Message sent via/by Savannah
  http://savannah.gnu.org/


___
Bug-gnustep mailing list
Bug-gnustep@gnu.org
https://lists.gnu.org/mailman/listinfo/bug-gnustep

[bug #33392] Multi-thread bug in NSObject retain and release

[Richard Frith-Macdonald]

Follow-up Comment #6, bug #33392 (project gnustep):

Yes please ... it would be great to use well tested/optimised code from gcc
rather than having  a non-expert like me trying to write assembler for
machines I don't even own.
Most of the assembler code I just took on trust from people contributing it,
and while I've now taken the time to check the i486 code as well as I
reasonably can, the other assembler is still largely unknown.

___

Reply to this item at:

  

___
  Message sent via/by Savannah
  http://savannah.gnu.org/


___
Bug-gnustep mailing list
Bug-gnustep@gnu.org
https://lists.gnu.org/mailman/listinfo/bug-gnustep

[bug #33392] Multi-thread bug in NSObject retain and release

[Niels Grewe]

Follow-up Comment #5, bug #33392 (project gnustep):

What Nicola is saying is perfectly accurate, but I think binary compatibility
is only an issue for packagers who want their package to work, e.g., on all
i[3-7]86 machines. People who build gnustep from source for their personal
machine probably want to utilize the complete feature-set their processor
provides. 

I don't think it's a big portability issue, though: On most platforms, GCC has
code emulating atomic operations in libgcc.a, which will be used when it
cannot (or refuses to) emit target specific assembly for that purpose. The
trick is to get it to link the static library if required. I have an autoconf
thingy in DBusKit that checks whether the atomic ops are provided as builtins
or library functions and adjusts the LDFLAGS accordingly. Should I try adding
it to -base as well?

___

Reply to this item at:

  

___
  Nachricht geschickt von/durch Savannah
  http://savannah.gnu.org/


___
Bug-gnustep mailing list
Bug-gnustep@gnu.org
https://lists.gnu.org/mailman/listinfo/bug-gnustep

[bug #33392] Multi-thread bug in NSObject retain and release

[Nicola Pero]

Follow-up Comment #4, bug #33392 (project gnustep):

IIRC, if GCC is compiling for a general x86 machine, it will 
generate code that can run on any i386 processor even produced; 
hence it won't use CPU operations available only in newer CPUs.  
So atomic builtins will become calls to custom functions (and 
could be even slower than a pthread lock).

To use atomic builtins, you need to tell GCC that the CPU is more 
than just a generic ix86 by using -march=pentium or something like 
that.  I'm not sure of the exact -march= or -mtune= option that we 
want to use; we need to look up the options and study them.

The extreme option would be to use -march=native, which would 
provide fast code, but most likely not portable.

Thanks

___

Reply to this item at:

  

___
  Message sent via/by Savannah
  http://savannah.gnu.org/


___
Bug-gnustep mailing list
Bug-gnustep@gnu.org
https://lists.gnu.org/mailman/listinfo/bug-gnustep

[bug #33392] Multi-thread bug in NSObject retain and release

[Richard Frith-Macdonald]

Follow-up Comment #3, bug #33392 (project gnustep):

I changed the atomic builtins as suggested ... and tried removing the check
for USE_ATOMIC_BUILTINS ... but the  result failed to link because the
compiler generated a link to a non-existent function to do the atomic builtin
decrement.  Presumably there is some compiler/linker option needed for the
atomic builtins to work, and gnustep-make doesn't supply them?

___

Reply to this item at:

  

___
  Message sent via/by Savannah
  http://savannah.gnu.org/


___
Bug-gnustep mailing list
Bug-gnustep@gnu.org
https://lists.gnu.org/mailman/listinfo/bug-gnustep

[bug #33392] Multi-thread bug in NSObject retain and release

[Eric Wasylishen]

Follow-up Comment #2, bug #33392 (project gnustep):

Just as a casual observer, it's good to see this bug identified and hopefully
fixed. :-)

However, I wondered why the ASM code was being used in the first place instead
of the GCC builtins:

#elif defined(__llvm__) || (defined(USE_ATOMIC_BUILDINS) && (__GNUC__ > 4 ||
(__GNUC__ ==  4 && __GNUC_MINOR__ >= 1)))
/* Use the GCC atomic operations with recent GCC versions */

The  USE_ATOMIC_BUILDINS is not defined anywhere, so the gcc builtins will
never be used unless the person building base defines it themselves. We should
simply delete the defined(USE_ATOMIC_BUILTINS) part so the builtins are always
used if available (they should be for most people as gcc 4.1.0 is now 5 years
old).



Possibly answering my own question, I believe we use the wrong gcc builtins!
see: http://gcc.gnu.org/onlinedocs/gcc-4.1.2/gcc/Atomic-Builtins.html

Currently we have:

typedef int32_t volatile *gsatomic_t;
#define GSATOMICREAD(X) (*(X))
#define GSAtomicIncrement(X)__sync_fetch_and_add(X, 1)
#define GSAtomicDecrement(X)__sync_fetch_and_sub(X, 1)

which means GSAtomicIncrement/Decrement return the old value. Don't we want
__sync_add_and_fetch and __sync_sub_and_fetch which return the new value? From
what I can tell the various ASM implementations return the new value as does
the windows call InterlockedIncrement.

___

Reply to this item at:

  

___
  Message sent via/by Savannah
  http://savannah.gnu.org/


___
Bug-gnustep mailing list
Bug-gnustep@gnu.org
https://lists.gnu.org/mailman/listinfo/bug-gnustep

[bug #33392] Multi-thread bug in NSObject retain and release

[Richard Frith-Macdonald]

Update of bug #33392 (project gnustep):

  Status:None => Fixed  
 Open/Closed:Open => In Test

___

Follow-up Comment #1:

Thanks ... the window for the bug to occur must be small for it not to have
shown up in testing before now.  Do you have a small test program to
demonstrate it?

Anyway, after spending several hours research on he web I hope I've got the
correct fix for this (I'm not at all familiar with x86 assembler though) and
have added it to svn trunk.

Please could you try this latest code and let me know if it's correct.


___

Reply to this item at:

  

___
  Message sent via/by Savannah
  http://savannah.gnu.org/


___
Bug-gnustep mailing list
Bug-gnustep@gnu.org
https://lists.gnu.org/mailman/listinfo/bug-gnustep

[bug #33392] Multi-thread bug in NSObject retain and release

[Jonathan Olson]

URL:
  

 Summary: Multi-thread bug in NSObject retain and release
 Project: GNUstep
Submitted by: jpolsonaz
Submitted on: Tue 24 May 2011 04:14:12 PM GMT
Category: Base/Foundation
Severity: 3 - Normal
  Item Group: Bug
  Status: None
 Privacy: Public
 Assigned to: None
 Open/Closed: Open
 Discussion Lock: Any

___

Details:

I have experienced frequent crashes in a heavily multi-threaded GNUstep
application.  All of these crashes indicate memory corruption, double frees,
etc.  I traced these crashes to the following atomic increment code in
NSObject.m which is NOT atomic at all.

static __inline__ int
GSAtomicIncrement(gsatomic_t X)
{
 __asm__ __volatile__ (
 "lock addl $1, %0"
 :"=m" (*X));
 return *X;
}

static __inline__ int
GSAtomicDecrement(gsatomic_t X)
{
 __asm__ __volatile__ (
 "lock subl $1, %0"
 :"=m" (*X));
 return *X;
}

Note that although the locked addl/subl instructions are atomic, the following
return *X results in a separate mov instruction.  If a context switch occurs
between these instructions, these functions can return incorrect values.

Later, NSObject uses the returned result from GSAtomicDecrement to determine
if the object must be deallocated.


  result =
GSAtomicDecrement((gsatomic_t)&(((obj)anObject)[-1].retained));
  if (result < 0)
 { /// deallocate object

If two threads simultaneously release the same object, it's possible for both
threads to invoke dealloc.

To work around this issue, I rebuilt GNUstep to use the slower NSLock rather
than the atomic increment/decrement instructions which eliminated these
crashes.






___

Reply to this item at:

  

___
  Message sent via/by Savannah
  http://savannah.gnu.org/


___
Bug-gnustep mailing list
Bug-gnustep@gnu.org
https://lists.gnu.org/mailman/listinfo/bug-gnustep