bugGNUstep - Bugs: bug #33392, Multi-thread bug in NSObject...

Group
 
 

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

Submitter:  Jonathan Olson <jpolsonaz>
Submitted:  Tue 24 May 2011 04:14:12 PM UTC
   
 
Category:  Base/Foundation Severity:  3 - Normal
Item Group:  Bug Status:  Fixed
Privacy:  Public Assigned to:  None
Open/Closed:  In Test
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Tue 21 Oct 2014 04:34:38 PM UTC, comment #9: 

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

Riccardo Mottola <rmottola>
Group Member
Thu 26 May 2011 07:08:30 PM UTC, comment #8: 

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

Niels Grewe <thebeing>
Group Member
Thu 26 May 2011 12:07:38 PM UTC, comment #7: 

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

Nicola Pero <nico>
Group Member
Thu 26 May 2011 11:15:39 AM UTC, comment #6: 

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.

Richard Frith-Macdonald <CaS>
Group Member
Thu 26 May 2011 11:05:37 AM UTC, comment #5: 

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?

Niels Grewe <thebeing>
Group Member
Thu 26 May 2011 09:38:09 AM UTC, comment #4: 

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

Nicola Pero <nico>
Group Member
Thu 26 May 2011 09:23:33 AM UTC, comment #3: 

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?

Richard Frith-Macdonald <CaS>
Group Member
Wed 25 May 2011 08:08:15 PM UTC, comment #2: 

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.

Eric Wasylishen <ericw>
Group Member
Wed 25 May 2011 09:32:37 AM UTC, 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.

Richard Frith-Macdonald <CaS>
Group Member
Tue 24 May 2011 04:14:12 PM UTC, original submission:  

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.


Jonathan Olson <jpolsonaz>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rmottola (Posted a comment)
  • -email is unavailable- added by thebeing (Posted a comment)
  • -email is unavailable- added by nico (Posted a comment)
  • -email is unavailable- added by ericw (Posted a comment)
  • -email is unavailable- added by CaS (Posted a comment)
  • -email is unavailable- added by jpolsonaz (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 2 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2011-05-25 CaS StatusNone Fixed
        Open/ClosedOpen In Test

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code