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
|
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
|
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
|
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.
|
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?
|
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
|
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?
|
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.
|
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.
|
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.
|