How does the attached patch look?
The patch adds two macros, _RWSTD_NO_STRING_ATOMIC_OPS and
_RWSTD_USE_STRING_ATOMIC_OPS. The first one is #defined for
all compilers on Linux/x86_64 (i.e., in wide mode), *unless*
the second one is defined, either on the command line on in
the generated config header by the user. This whole hackery
is guarded by _RWSTD_VER and automatically disabled (i.e.,
the library switches over to using atomic operations by
default) at version 5.
Martin
Martin Sebor wrote:
Travis Vitek wrote:
So the problem is that the size of the __string_ref has changed, right?
Yes. More precisely, that is one part of the problem (the other
part is the pthread functions that are inlined in user code and
that expect to get a mutex and not random garbage).
Specifically, this block of code causes the removal of the per-string
mutex from __string_ref.
+#ifndef _RWSTD_NO_ATOMIC_OPS
+ // disable string mutex when atomic operations are available
+# ifndef _RWSTD_NO_STRING_MUTEX
+# define _RWSTD_NO_STRING_MUTEX
+# endif // _RWSTD_NO_STRING_MUTEX
+#endif // _RWSTD_NO_ATOMIC_OPS
An obvious option would be to just remove that block and let the user
decide if they want to define _RWSTD_NO_STRING_MUTEX or not. If they
define it then they must know that they are breaking binary
compatibility with a library previously compiled without it. This
doesn't help users compiling new source using the default configuration,
but it does maintain compatibility.
I'm also leaning in this general direction. Keep the compatible
behavior and and let users opt into the breaking fix. I'll have
to think some more about how to control the behavior. I was
hoping to use an existing config macro without changing any
library code (except _config-gcc.h).
We could also ask that they define _RWSTD_NO_ATOMIC_OPS, but that may
cause other binary incompatibilities elsewhere or it will at the very
least cause performance problems in other places.
It is very tempting to go with the improved implementation by
default, but from a compatibility standpoint it would be the
wrong thing to do.
It would be nice if we could just insert an appropriately sized pad
buffer in place of the _C_mutex member. If the methods of __string_ref
were not inlined I'm pretty sure that this would work. Unfortunately
_C_inc_ref() and _C_dec_ref() are inlined, so their code may be compiled
in the user executable, so I'm not convinced that this is a viable
option.
All the __string_ref members are trivial inline wrappers that
are fully expected to be inlined. It's possible that some of
them will not be inlined under some conditions but I expect
those to be exceeding rare.
Martin
Travis
Martin Sebor wrote:
Okay, I've got it:
http://issues.apache.org/jira/browse/STDCXX-162
Damn that was hard!
So, what do we do? Going back to using a mutex for strings would
be *huge* performance hit on one of the most popular platforms
(if not the most popular one), but then again, keeping the status
quo will break binary compatibility on the (now) most popular
platform.
Opinions?
Martin
Martin Sebor wrote:
Martin Sebor wrote:
In a 12D build with the default gcc 4.1.0 on SuSE Linux Enterprise
Server 10 (x86_64), the following simple program abends with the
error below after upgrading the 4.1.3 library to 4.2.0:
I've enhanced the program to replace operators new and delete
and to print the value of the pointer. The enhanced test case
and the output obtained from a 12D build with gcc 3.4.6 on Red
Hat Enterprise Linux AS release 4 (Nahant Update 4) is below.
Interestingly, the 12d (32-bit) output with Sun C++ on Solaris
is fine.
$ cat t.cpp && LD_LIBRARY_PATH=../lib ./t
#include <cstdio>
#include <cstdlib>
#include <new>
#include <string>
int main ()
{
std::string s = "a";
}
void* operator new (std::size_t n) throw (std::bad_alloc)
{
void* const p = std::malloc (n);
std::fprintf (stdout, "operator new (%zu) ==> %#p\n", n, p);
return p;
}
void operator delete (void *p) throw ()
{
std::fprintf (stdout, "operator delete (%#p)\n", p);
std::free (p);
}
void* operator new[] (std::size_t n) throw (std::bad_alloc)
{
void* const p = std::malloc (n);
std::fprintf (stdout, "operator new[] (%zu) ==> %#p\n", n, p);
return p;
}
void operator delete[] (void *p) throw ()
{
std::fprintf (stdout, "operator delete[] (%#p)\n", p);
std::free (p);
}
operator new (58) ==> 0x502010
operator delete (0x501fe8)
*** glibc detected *** free(): invalid pointer:
0x0000000000501fe8 ***
Aborted
Index: include/rw/_config.h
===================================================================
--- include/rw/_config.h (revision 585584)
+++ include/rw/_config.h (working copy)
@@ -25,7 +25,7 @@
* implied. See the License for the specific language governing
* permissions and limitations under the License.
*
- * Copyright 1994-2006 Rogue Wave Software.
+ * Copyright 1994-2007 Rogue Wave Software, Inc.
*
**************************************************************************/
@@ -133,6 +133,24 @@
#if defined (__sparc__) || defined (__sparc)
#endif // SPARC
+/*** AMD64/Intel EM64T ****************************************************/
+
+#if defined (__amd64__) || defined (__amd64) \
+ || defined (__x86_64__) || defined (__x86_64)
+
+# if _RWSTD_VER < 0x05000000
+# ifdef _RWSTD_OS_LINUX
+ // on Linux/AMD64, unless explicitly requested, disable the use
+ // of atomic operations in string for binary compatibility with
+ // stdcxx 4.1.x
+# ifndef _RWSTD_USE_STRING_ATOMIC_OPS
+# define _RWSTD_NO_STRING_ATOMIC_OPS
+# endif // _RWSTD_USE_STRING_ATOMIC_OPS
+# endif // _WIN32
+# endif // stdcxx < 5.0
+
+#endif // AMD64/EM64T
+
/**************************************************************************
* COMPILER-SPECIFIC OVERRIDES *
**************************************************************************/
Index: include/rw/_strref.h
===================================================================
--- include/rw/_strref.h (revision 585584)
+++ include/rw/_strref.h (working copy)
@@ -9,22 +9,23 @@
*
***************************************************************************
*
- * Copyright 2005-2006 The Apache Software Foundation or its licensors,
- * as applicable.
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed
+ * with this work for additional information regarding copyright
+ * ownership. The ASF licenses this file to you under the Apache
+ * License, Version 2.0 (the "License"); you may not use this file
+ * except in compliance with the License. You may obtain a copy of
+ * the License at
*
- * Copyright 1994-2006 Rogue Wave Software.
+ * http://www.apache.org/licenses/LICENSE-2.0
*
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+ * implied. See the License for the specific language governing
+ * permissions and limitations under the License.
*
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
+ * Copyright 1994-2007 Rogue Wave Software, Inc.
*
**************************************************************************/
@@ -56,13 +57,14 @@
# define _RWSTD_STRING_REF_INT long
#endif
-#ifndef _RWSTD_NO_ATOMIC_OPS
+#if !defined (_RWSTD_NO_ATOMIC_OPS) && !defined (_RWSTD_NO_STRING_ATOMIC_OPS)
// disable string mutex when atomic operations are available
# ifndef _RWSTD_NO_STRING_MUTEX
# define _RWSTD_NO_STRING_MUTEX
# endif // _RWSTD_NO_STRING_MUTEX
-#endif // _RWSTD_NO_ATOMIC_OPS
+#endif // _RWSTD_NO_ATOMIC_OPS && !_RWSTD_NO_STRING_ATOMIC_OPS
+
_RWSTD_NAMESPACE (std) {
