public inbox for libstdc++@gcc.gnu.org
 help / color / mirror / Atom feed
* Patch ping
@ 2022-01-04 12:45 Jakub Jelinek
  0 siblings, 0 replies; 17+ messages in thread
From: Jakub Jelinek @ 2022-01-04 12:45 UTC (permalink / raw)
  To: Jonathan Wakely; +Cc: gcc-patches, libstdc++

Hi!

I'd like to ping the
https://gcc.gnu.org/pipermail/libstdc++/2021-December/053680.html
time_get patch.

Thanks

	Jakub


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: Patch ping
  2005-10-07 22:07                 ` Ian Lance Taylor
  2005-10-08  0:48                   ` Benjamin Kosnik
@ 2005-10-11  0:22                   ` Benjamin Kosnik
  1 sibling, 0 replies; 17+ messages in thread
From: Benjamin Kosnik @ 2005-10-11  0:22 UTC (permalink / raw)
  To: Ian Lance Taylor; +Cc: pcarlini, cow, pdimov, libstdc++

[-- Attachment #1: Type: text/plain, Size: 1086 bytes --]


> > I believe in this *specific* case we can at least use mutexes without
> > penalizing performances.
> 
> I'm testing this patch.

Thanks for your patience with this.

Your patch looks acceptable to me, modulo some versioning tweaks. Since
gcc-4.0.2 was released with a defined set of GLIBCXX_3.4.6 exports, to
add new symbols we have to use GLIBCXX_3.4.7 and bump versions
accordingly. We are trying to keep mainline and gcc-4_0-branch in sync.

Using concurrence.h is an interesting exercise: we are not exporting
mutex objects, and using in .h files is out due to the multiple-copies
thing. It's helped us write exception-safe locks via the scoped lock
idiom but there is obvious room for improvement.

The long term plan (as far as I am concerned) is to look into using
Han's atomic_ops package, and building higher-level locking mechanisms
into concurrence.h that don't have such picky linkage issues. We
won't be able to use them in all situations, but hopefully more options
will mean more (and better) solutions.

Anyway.

This is ok for mainline/gcc-4_0-branch.

-benjamin


[-- Attachment #2: p.20051010 --]
[-- Type: text/plain, Size: 6142 bytes --]

2005-10-10  Benjamin Kosnik  <bkoz@redhat.com>

	* configure.ac (libtool_VERSION): To 6:7:0.
	* configure: Regenerate.
	* testsuite/testsuite_abi.cc (check_version): Add GLIBCXX_3.4.7.
	* config/linker-map.gnu: Export locale::_Impl::_M_install_cache.

2005-10-10  Ian Lance Taylor  <ian@airs.com>

	PR libstdc++/13583
	* include/bits/locale_classes.h (locale::_Impl::_M_install_cache):
	Move out of line.
	* src/locale.cc: Define here, add mutex.
	
Index: configure.ac
===================================================================
RCS file: /cvs/gcc/gcc/libstdc++-v3/configure.ac,v
retrieving revision 1.39
diff -c -p -r1.39 configure.ac
*** configure.ac	3 Aug 2005 23:17:05 -0000	1.39
--- configure.ac	10 Oct 2005 23:30:20 -0000
*************** AC_CONFIG_HEADER(config.h)
*** 12,18 ****
  ### am handles this now?  ORIGINAL_LD_FOR_MULTILIBS=$LD
  
  # For libtool versioning info, format is CURRENT:REVISION:AGE
! libtool_VERSION=6:6:0
  AC_SUBST(libtool_VERSION)
  
  # Find the rest of the source tree framework.
--- 12,18 ----
  ### am handles this now?  ORIGINAL_LD_FOR_MULTILIBS=$LD
  
  # For libtool versioning info, format is CURRENT:REVISION:AGE
! libtool_VERSION=6:7:0
  AC_SUBST(libtool_VERSION)
  
  # Find the rest of the source tree framework.
Index: config/linker-map.gnu
===================================================================
RCS file: /cvs/gcc/gcc/libstdc++-v3/config/linker-map.gnu,v
retrieving revision 1.86
diff -c -p -r1.86 linker-map.gnu
*** config/linker-map.gnu	8 Oct 2005 18:17:20 -0000	1.86
--- config/linker-map.gnu	10 Oct 2005 23:30:21 -0000
*************** GLIBCXX_3.4 {
*** 84,90 ****
        std::locale::[A-Zj-z]*;
        std::locale::_[A-Ha-z]*;
        std::locale::_Impl::[A-Za-z]*;
!       std::locale::_Impl::_M_[A-Za-z]*;
        std::locale::_[J-Ra-z]*;
        std::locale::_S_normalize_category*;
        std::locale::_[T-Za-z]*;
--- 84,90 ----
        std::locale::[A-Zj-z]*;
        std::locale::_[A-Ha-z]*;
        std::locale::_Impl::[A-Za-z]*;
! #     std::locale::_Impl::_M_[A-Za-z]*;
        std::locale::_[J-Ra-z]*;
        std::locale::_S_normalize_category*;
        std::locale::_[T-Za-z]*;
*************** GLIBCXX_3.4 {
*** 468,473 ****
--- 468,479 ----
  
      _ZNSt19istreambuf_iteratorI[cw]St11char_traitsI[cw]EEppEv;
  
+     # std::locale::Impl _M_ members
+     _ZNSt6locale5_Impl16_M_install_facetEPKNS_2idEPKNS_5facetE;
+     _ZNSt6locale5_Impl16_M_replace_facetEPKS0_PKNS_2idE;
+     _ZNSt6locale5_Impl19_M_replace_categoryEPKS0_PKPKNS_2idE;
+     _ZNSt6locale5_Impl21_M_replace_categoriesEPKS0_i;
+ 
    # DO NOT DELETE THIS LINE.  Port-specific symbols, if any, will be here.
  
    local:
*************** GLIBCXX_3.4.6 {
*** 571,580 ****
  
      _ZNKSt15basic_stringbufIwSt11char_traitsIwESaIwEE3strEv;
  
!    _ZN9__gnu_cxx6__poolILb1EE13_M_initializeEv;
  
  } GLIBCXX_3.4.5;
  
  # Symbols in the support library (libsupc++) have their own tag.
  CXXABI_1.3 {
  
--- 577,592 ----
  
      _ZNKSt15basic_stringbufIwSt11char_traitsIwESaIwEE3strEv;
  
!     _ZN9__gnu_cxx6__poolILb1EE13_M_initializeEv;
  
  } GLIBCXX_3.4.5;
  
+ GLIBCXX_3.4.7 {
+ 
+     _ZNSt6locale5_Impl16_M_install_cacheEPKNS_5facetE[jm];
+ 
+ } GLIBCXX_3.4.6;
+ 
  # Symbols in the support library (libsupc++) have their own tag.
  CXXABI_1.3 {
  
Index: include/bits/locale_classes.h
===================================================================
RCS file: /cvs/gcc/gcc/libstdc++-v3/include/bits/locale_classes.h,v
retrieving revision 1.25
diff -c -p -r1.25 locale_classes.h
*** include/bits/locale_classes.h	17 Aug 2005 02:12:52 -0000	1.25
--- include/bits/locale_classes.h	10 Oct 2005 23:30:21 -0000
*************** namespace std
*** 559,569 ****
        { _M_install_facet(&_Facet::id, __facet); }
  
      void
!     _M_install_cache(const facet* __cache, size_t __index) throw()
!     {
!       __cache->_M_add_reference();
!       _M_caches[__index] = __cache;
!     }
    };
  
    template<typename _Facet>
--- 559,565 ----
        { _M_install_facet(&_Facet::id, __facet); }
  
      void
!     _M_install_cache(const facet*, size_t);
    };
  
    template<typename _Facet>
Index: src/locale.cc
===================================================================
RCS file: /cvs/gcc/gcc/libstdc++-v3/src/locale.cc,v
retrieving revision 1.111
diff -c -p -r1.111 locale.cc
*** src/locale.cc	17 Aug 2005 02:14:22 -0000	1.111
--- src/locale.cc	10 Oct 2005 23:30:21 -0000
***************
*** 33,38 ****
--- 33,45 ----
  #include <cwctype>     // For towupper, etc.
  #include <locale>
  #include <bits/atomicity.h>
+ #include <bits/concurrence.h>
+ 
+ namespace __gnu_internal
+ {
+   // Mutex object for cache access
+   static __glibcxx_mutex_define_initialized(locale_cache_mutex);
+ }
  
  namespace std 
  {
*************** namespace std 
*** 366,371 ****
--- 373,395 ----
        }
    }
  
+   void
+   locale::_Impl::
+   _M_install_cache(const facet* __cache, size_t __index)
+   {
+     __gnu_cxx::lock sentry(__gnu_internal::locale_cache_mutex);
+     if (_M_caches[__index] != 0)
+       {
+ 	// Some other thread got in first.
+ 	delete __cache;
+       }
+     else
+       {
+ 	__cache->_M_add_reference();
+ 	_M_caches[__index] = __cache;
+       }
+   }
+ 
    // locale::id
    // Definitions for static const data members of locale::id
    _Atomic_word locale::id::_S_refcount;  // init'd to 0 by linker
Index: testsuite/testsuite_abi.cc
===================================================================
RCS file: /cvs/gcc/gcc/libstdc++-v3/testsuite/testsuite_abi.cc,v
retrieving revision 1.13
diff -c -p -r1.13 testsuite_abi.cc
*** testsuite/testsuite_abi.cc	17 Aug 2005 02:14:28 -0000	1.13
--- testsuite/testsuite_abi.cc	10 Oct 2005 23:30:21 -0000
*************** check_version(symbol& test, bool added)
*** 185,190 ****
--- 185,191 ----
        known_versions.push_back("GLIBCXX_3.4.4"); 
        known_versions.push_back("GLIBCXX_3.4.5");
        known_versions.push_back("GLIBCXX_3.4.6");
+       known_versions.push_back("GLIBCXX_3.4.7");
        known_versions.push_back("CXXABI_1.3");
        known_versions.push_back("CXXABI_1.3.1");
      }

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: Patch ping
  2005-10-07 22:07                 ` Ian Lance Taylor
@ 2005-10-08  0:48                   ` Benjamin Kosnik
  2005-10-11  0:22                   ` Benjamin Kosnik
  1 sibling, 0 replies; 17+ messages in thread
From: Benjamin Kosnik @ 2005-10-08  0:48 UTC (permalink / raw)
  To: Ian Lance Taylor; +Cc: pcarlini, cow, pdimov, libstdc++


It looks like Paolo has already clued you in to the linkage issues.
Please, although I've already sent you mail about this privately,
please allow me to look at this issue on Monday.

-benjamin

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: Patch ping
  2005-10-07 21:40               ` Paolo Carlini
  2005-10-07 21:46                 ` Jonathan Wakely
@ 2005-10-07 22:07                 ` Ian Lance Taylor
  2005-10-08  0:48                   ` Benjamin Kosnik
  2005-10-11  0:22                   ` Benjamin Kosnik
  1 sibling, 2 replies; 17+ messages in thread
From: Ian Lance Taylor @ 2005-10-07 22:07 UTC (permalink / raw)
  To: Paolo Carlini; +Cc: Jonathan Wakely, Peter Dimov, libstdc++

Paolo Carlini <pcarlini@suse.de> writes:

> Guys, thanks a lot for the nice ideas, but remember that we have first
> to figure out something for the short/mid term: for one, we don't have
> the compare_and_swap primitive implemented for all the targets and, for
> some, it will *never* be (i386, etc). Nothing new...

Sad but true.

> I believe in this *specific* case we can at least use mutexes without
> penalizing performances.

I'm testing this patch.

Ian

Index: config/linker-map.gnu
===================================================================
RCS file: /cvs/gcc/gcc/libstdc++-v3/config/linker-map.gnu,v
retrieving revision 1.85
diff -p -u -r1.85 linker-map.gnu
--- config/linker-map.gnu	12 Sep 2005 04:49:09 -0000	1.85
+++ config/linker-map.gnu	7 Oct 2005 22:05:31 -0000
@@ -84,7 +84,7 @@ GLIBCXX_3.4 {
       std::locale::[A-Zj-z]*;
       std::locale::_[A-Ha-z]*;
       std::locale::_Impl::[A-Za-z]*;
-      std::locale::_Impl::_M_[A-Za-z]*;
+#     std::locale::_Impl::_M_[A-Za-z]*;
       std::locale::_[J-Ra-z]*;
       std::locale::_S_normalize_category*;
       std::locale::_[T-Za-z]*;
@@ -468,6 +468,12 @@ GLIBCXX_3.4 {
 
     _ZNSt19istreambuf_iteratorI[cw]St11char_traitsI[cw]EEppEv;
 
+    # std::locale::Impl _M_ members
+    _ZNSt6locale5_Impl16_M_install_facetEPKNS_2idEPKNS_5facetE;
+    _ZNSt6locale5_Impl16_M_replace_facetEPKS0_PKNS_2idE;
+    _ZNSt6locale5_Impl19_M_replace_categoryEPKS0_PKPKNS_2idE;
+    _ZNSt6locale5_Impl21_M_replace_categoriesEPKS0_i;
+
   # DO NOT DELETE THIS LINE.  Port-specific symbols, if any, will be here.
 
   local:
@@ -571,7 +577,9 @@ GLIBCXX_3.4.6 {
 
     _ZNKSt15basic_stringbufIwSt11char_traitsIwESaIwEE3strEv;
 
-   _ZN9__gnu_cxx6__poolILb1EE13_M_initializeEv;
+    _ZN9__gnu_cxx6__poolILb1EE13_M_initializeEv;
+
+    _ZNSt6locale5_Impl16_M_install_cacheEPKNS_5facetEj;
 
 } GLIBCXX_3.4.5;
 
Index: include/bits/locale_classes.h
===================================================================
RCS file: /cvs/gcc/gcc/libstdc++-v3/include/bits/locale_classes.h,v
retrieving revision 1.25
diff -p -u -r1.25 locale_classes.h
--- include/bits/locale_classes.h	17 Aug 2005 02:12:52 -0000	1.25
+++ include/bits/locale_classes.h	7 Oct 2005 22:05:35 -0000
@@ -559,11 +559,7 @@ namespace std
       { _M_install_facet(&_Facet::id, __facet); }
 
     void
-    _M_install_cache(const facet* __cache, size_t __index) throw()
-    {
-      __cache->_M_add_reference();
-      _M_caches[__index] = __cache;
-    }
+    _M_install_cache(const facet* __cache, size_t __index);
   };
 
   template<typename _Facet>
Index: src/locale.cc
===================================================================
RCS file: /cvs/gcc/gcc/libstdc++-v3/src/locale.cc,v
retrieving revision 1.111
diff -p -u -r1.111 locale.cc
--- src/locale.cc	17 Aug 2005 02:14:22 -0000	1.111
+++ src/locale.cc	7 Oct 2005 22:05:39 -0000
@@ -33,6 +33,13 @@
 #include <cwctype>     // For towupper, etc.
 #include <locale>
 #include <bits/atomicity.h>
+#include <bits/concurrence.h>
+
+namespace __gnu_internal
+{
+  // Mutex object for cache access
+  static __glibcxx_mutex_define_initialized(locale_cache_mutex);
+}
 
 namespace std 
 {
@@ -366,6 +373,23 @@ namespace std 
       }
   }
 
+  void
+  locale::_Impl::
+  _M_install_cache(const facet* __cache, size_t __index)
+  {
+    __gnu_cxx::lock sentry(__gnu_internal::locale_cache_mutex);
+    if (_M_caches[__index] != 0)
+      {
+	// Some other thread got in first.
+	delete __cache;
+      }
+    else
+      {
+	__cache->_M_add_reference();
+	_M_caches[__index] = __cache;
+      }
+  }
+
   // locale::id
   // Definitions for static const data members of locale::id
   _Atomic_word locale::id::_S_refcount;  // init'd to 0 by linker

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: Patch ping
  2005-10-07 21:46                 ` Jonathan Wakely
@ 2005-10-07 21:52                   ` Paolo Carlini
  0 siblings, 0 replies; 17+ messages in thread
From: Paolo Carlini @ 2005-10-07 21:52 UTC (permalink / raw)
  To: Jonathan Wakely; +Cc: Peter Dimov, Ian Lance Taylor, libstdc++

Jonathan Wakely wrote:

>The main reason I thought of the CAS solution is that it wouldn't change
>the ABI, but hopefully you'll be able to squeeze a mutex in somewhere
>without too much trouble.
>  
>
You are right, that would make everything much simpler from the ABI
point of view, but really,  if we want to fix this bug in the 4.1 time
frame (for concreteness) either we use a mutex or we use only
__atomic_add and __exchange_and_add. This is our toolbox.

I'm also thinking that the memory leak is demonstrably bounded and if we
could find a very simple way to make it much smaller whereas not zero
yet, would be still a satisfactory improvement, for now.

Paolo.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: Patch ping
  2005-10-07 21:40               ` Paolo Carlini
@ 2005-10-07 21:46                 ` Jonathan Wakely
  2005-10-07 21:52                   ` Paolo Carlini
  2005-10-07 22:07                 ` Ian Lance Taylor
  1 sibling, 1 reply; 17+ messages in thread
From: Jonathan Wakely @ 2005-10-07 21:46 UTC (permalink / raw)
  To: Paolo Carlini; +Cc: Peter Dimov, Ian Lance Taylor, libstdc++

Paolo Carlini wrote:

> I believe in this *specific* case we can at least use mutexes without
> penalizing performances.

The main reason I thought of the CAS solution is that it wouldn't change
the ABI, but hopefully you'll be able to squeeze a mutex in somewhere
without too much trouble.

jon

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: Patch ping
  2005-10-07 21:36             ` Jonathan Wakely
@ 2005-10-07 21:40               ` Paolo Carlini
  2005-10-07 21:46                 ` Jonathan Wakely
  2005-10-07 22:07                 ` Ian Lance Taylor
  0 siblings, 2 replies; 17+ messages in thread
From: Paolo Carlini @ 2005-10-07 21:40 UTC (permalink / raw)
  To: Jonathan Wakely; +Cc: Peter Dimov, Ian Lance Taylor, libstdc++

Jonathan Wakely wrote:

>Peter Dimov wrote:
>
>>Can't this be solved using atomic_compare_exchange (I'm not sure of its 
>>exact spelling, or whether it's currently part of libstdc++)?
>>    
>>
>I was just thinking the same thing.  Currently the work you described
>is split across two functions, but would this work, given a pointer to
>the new object, "new" ...
>
>   old = caches[i];
>   new->add_reference();
>   while ( !compare_and_swap(caches[i], old, new) )
>   {
>       old = caches[i];
>   }
>   if (old)
>     old->remove_reference();
>  
>
Guys, thanks a lot for the nice ideas, but remember that we have first
to figure out something for the short/mid term: for one, we don't have
the compare_and_swap primitive implemented for all the targets and, for
some, it will *never* be (i386, etc). Nothing new...

I believe in this *specific* case we can at least use mutexes without
penalizing performances.

Paolo.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: Patch ping
  2005-10-07 21:21           ` Peter Dimov
@ 2005-10-07 21:36             ` Jonathan Wakely
  2005-10-07 21:40               ` Paolo Carlini
  0 siblings, 1 reply; 17+ messages in thread
From: Jonathan Wakely @ 2005-10-07 21:36 UTC (permalink / raw)
  To: Peter Dimov; +Cc: Paolo Carlini, Ian Lance Taylor, libstdc++

Peter Dimov wrote:

> Can't this be solved using atomic_compare_exchange (I'm not sure of its 
> exact spelling, or whether it's currently part of libstdc++)?

I was just thinking the same thing.  Currently the work you described
is split across two functions, but would this work, given a pointer to
the new object, "new" ...

   old = caches[i];
   new->add_reference();
   while ( !compare_and_swap(caches[i], old, new) )
   {
       old = caches[i];
   }
   if (old)
     old->remove_reference();

jon


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: Patch ping
  2005-10-07 20:25         ` Paolo Carlini
@ 2005-10-07 21:21           ` Peter Dimov
  2005-10-07 21:36             ` Jonathan Wakely
  0 siblings, 1 reply; 17+ messages in thread
From: Peter Dimov @ 2005-10-07 21:21 UTC (permalink / raw)
  To: Paolo Carlini, Ian Lance Taylor; +Cc: libstdc++

>> Or another option would be to eliminate the possible memory leak in
>> some other way.  The possibility of the memory leak arises because
>> two threads may call operator() on a __use_cache simultaneously. When 
>> this happens, it is possible for both threads to test that
>> __caches[__i] is not set, and for both to set it to a new cache
>> object.  In a preemptive threading model, the threads could switch
>> right at the assignment in _M_install_cache, so it seems
>> theoretically possible for the value stored in _M_caches[__index] to
>> be confused by a simultaneous assignment from two threads.  I don't
>> see this can be safely avoided without using a mutex.

Can't this be solved using atomic_compare_exchange (I'm not sure of its 
exact spelling, or whether it's currently part of libstdc++)?

for(;;)
{

if( atomic_load_acquire( __caches[i] ) == 0 )
{
    p = new cache;

    if( !atomic_compare_exchange_release( &__caches[i], 0, p ) )
    {
        // another thread has written to __caches[i] in the meantime
        delete p;
    }
    else
    {
        break;
    }
}

}

or something along those lines.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: Patch ping
  2005-10-07 20:05       ` Ian Lance Taylor
@ 2005-10-07 20:25         ` Paolo Carlini
  2005-10-07 21:21           ` Peter Dimov
  0 siblings, 1 reply; 17+ messages in thread
From: Paolo Carlini @ 2005-10-07 20:25 UTC (permalink / raw)
  To: Ian Lance Taylor; +Cc: libstdc++

Hi Ian,

>Well, one option would presumably be to try to find somewhere to stash
>the mutex which would not cause an ABI compatibility problem.  For
>example, perhaps we could add a new field to locale::_Impl in such a
>way that no ABI problem was introduced.  If we could ensure that the
>new field was at the end of the structure in memory, then presumably
>we would be OK as long as nothing depends on the size of the
>structure.  Unfortunately, I see that src/globals_locale.cc does
>depend on the size of locale::_Impl, so that won't work.
>
>Or another option would be to borrow an existing mutex, which would
>not introduce any new dependencies.  Unfortunately, all the mutexes I
>see are file static and can not be referenced.
>
>Or another option would be to eliminate the possible memory leak in
>some other way.  The possibility of the memory leak arises because two
>threads may call operator() on a __use_cache simultaneously.  When
>this happens, it is possible for both threads to test that
>__caches[__i] is not set, and for both to set it to a new cache
>object.  In a preemptive threading model, the threads could switch
>right at the assignment in _M_install_cache, so it seems theoretically
>possible for the value stored in _M_caches[__index] to be confused by
>a simultaneous assignment from two threads.  I don't see this can be
>safely avoided without using a mutex.
>  
>
Thanks for the detailed analysis of the various theoretical options, I 
learned from it.

>So the only options I see are to add a new exported symbol in
>libstdc++ (which could be a mutex, or we could, e.g., move
>locale::_Impl::_M_install_cache to locale.cc), or to not retain the
>caches.
>  
>
If by "not retaining" you mean, basically, not using the caching 
mechanism, it is not an option: basically is the only way to achieve 
performance and correctness in this area (barring a complete redesign 
according to a complete different "philosophy", i.e., using narrowing 
instead of widening, an approach that we don't like, in a nutshell). 
Therefore, we need mutual exclusion. I'm under the impression that a 
smaller reorganization, moving code dealing with those mutexes to one of 
the *.cc files is by far the preferred option, consistent with the rest 
of the library: are you willing to get into the details of that? I could 
certainly help.

Paolo.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: Patch ping
  2005-10-07 19:03     ` Paolo Carlini
@ 2005-10-07 20:05       ` Ian Lance Taylor
  2005-10-07 20:25         ` Paolo Carlini
  0 siblings, 1 reply; 17+ messages in thread
From: Ian Lance Taylor @ 2005-10-07 20:05 UTC (permalink / raw)
  To: Paolo Carlini; +Cc: libstdc++

Paolo Carlini <pcarlini@suse.de> writes:

> Ian Lance Taylor wrote:
> 
> >It would be easy to add a single instance of the mutex in
> >src/locale_init.cc.  But then code compiled with a newer version of
> >libstdc++ would not work with older versions of libstdc++.so, as there
> >would be an undefined symbol at link time.  Does that count as a bad
> >ABI change?  It would continue to be possible to use code compiled
> >with older versions of libstdc++ with the new libstdc++.so; the older
> >code just wouldn't use the mutex when initializing the cache.
> >
> I think this kind of "incompatibility" *in principle* is possible:
> i.e., the .so number of mainline is 6.0.6 whereas 3.4.0 had 6.0.0
> (http://gcc.gnu.org/onlinedocs/libstdc++/abi.html) and certainly the
> same difficulty is experienced by someone trying to use the 3.4.0 .so
> together with code compiled with the mainline - or 4_0-branch for that
> matter - headers.
> 
> However, I think Benjamin espressed some concerns about symbol exports
> (only one for your approach, right?) I'm not sure whether there are
> specific problems with exporting mutex objects or whether he is
> referring to our more general policy lately of adding exports only if
> really, really, needed.

Well, one option would presumably be to try to find somewhere to stash
the mutex which would not cause an ABI compatibility problem.  For
example, perhaps we could add a new field to locale::_Impl in such a
way that no ABI problem was introduced.  If we could ensure that the
new field was at the end of the structure in memory, then presumably
we would be OK as long as nothing depends on the size of the
structure.  Unfortunately, I see that src/globals_locale.cc does
depend on the size of locale::_Impl, so that won't work.

Or another option would be to borrow an existing mutex, which would
not introduce any new dependencies.  Unfortunately, all the mutexes I
see are file static and can not be referenced.

Or another option would be to eliminate the possible memory leak in
some other way.  The possibility of the memory leak arises because two
threads may call operator() on a __use_cache simultaneously.  When
this happens, it is possible for both threads to test that
__caches[__i] is not set, and for both to set it to a new cache
object.  In a preemptive threading model, the threads could switch
right at the assignment in _M_install_cache, so it seems theoretically
possible for the value stored in _M_caches[__index] to be confused by
a simultaneous assignment from two threads.  I don't see this can be
safely avoided without using a mutex.

So the only options I see are to add a new exported symbol in
libstdc++ (which could be a mutex, or we could, e.g., move
locale::_Impl::_M_install_cache to locale.cc), or to not retain the
caches.

Ian

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: Patch ping
  2005-10-07 18:06   ` Ian Lance Taylor
@ 2005-10-07 19:03     ` Paolo Carlini
  2005-10-07 20:05       ` Ian Lance Taylor
  0 siblings, 1 reply; 17+ messages in thread
From: Paolo Carlini @ 2005-10-07 19:03 UTC (permalink / raw)
  To: Ian Lance Taylor; +Cc: libstdc++

Ian Lance Taylor wrote:

>It would be easy to add a single instance of the mutex in
>src/locale_init.cc.  But then code compiled with a newer version of
>libstdc++ would not work with older versions of libstdc++.so, as there
>would be an undefined symbol at link time.  Does that count as a bad
>ABI change?  It would continue to be possible to use code compiled
>with older versions of libstdc++ with the new libstdc++.so; the older
>code just wouldn't use the mutex when initializing the cache.
>  
>
I think this kind of "incompatibility" *in principle* is possible: i.e., 
the .so number of mainline is 6.0.6 whereas 3.4.0 had 6.0.0 
(http://gcc.gnu.org/onlinedocs/libstdc++/abi.html) and certainly the 
same difficulty is experienced by someone trying to use the 3.4.0 .so 
together with code compiled with the mainline - or 4_0-branch for that 
matter - headers.

However, I think Benjamin espressed some concerns about symbol exports 
(only one for your approach, right?) I'm not sure whether there are 
specific problems with exporting mutex objects or whether he is 
referring to our more general policy lately of adding exports only if 
really, really, needed.

Paolo.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: Patch ping
  2005-10-07  9:52 ` Paolo Carlini
@ 2005-10-07 18:06   ` Ian Lance Taylor
  2005-10-07 19:03     ` Paolo Carlini
  0 siblings, 1 reply; 17+ messages in thread
From: Ian Lance Taylor @ 2005-10-07 18:06 UTC (permalink / raw)
  To: Paolo Carlini; +Cc: libstdc++

Paolo Carlini <pcarlini@suse.de> writes:

> Ian Lance Taylor wrote:

> >Patch for 4.1 regression PR libstdc++/13583 (limited memory leak in
> >threaded program):
> >    http://gcc.gnu.org/ml/gcc-patches/2005-09/msg01550.html
> >  
> >
> Ian, I think your approach has a serious problem: we end up with
> different mutexes in different translation units (locale_facets.tcc is
> included byt <locale>) and this doesn't seem safe, in general.
> 
> I would be happy to be wrong, because the same approach would solve
> another issue in our shared_ptr code, as you may have noticed. Is there
> something I'm missing?

You're quite right.  My patch is completely bogus.  Thanks for
noticing that.

It would be easy to add a single instance of the mutex in
src/locale_init.cc.  But then code compiled with a newer version of
libstdc++ would not work with older versions of libstdc++.so, as there
would be an undefined symbol at link time.  Does that count as a bad
ABI change?  It would continue to be possible to use code compiled
with older versions of libstdc++ with the new libstdc++.so; the older
code just wouldn't use the mutex when initializing the cache.

Ian

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: Patch ping
  2005-10-04 16:45 Ian Lance Taylor
@ 2005-10-07  9:52 ` Paolo Carlini
  2005-10-07 18:06   ` Ian Lance Taylor
  0 siblings, 1 reply; 17+ messages in thread
From: Paolo Carlini @ 2005-10-07  9:52 UTC (permalink / raw)
  To: Ian Lance Taylor; +Cc: libstdc++

Ian Lance Taylor wrote:

>[ Forgot to CC libstdc++ on this one ]
>
>Patch for 4.1 regression PR libstdc++/13583 (limited memory leak in
>threaded program):
>    http://gcc.gnu.org/ml/gcc-patches/2005-09/msg01550.html
>  
>
Ian, I think your approach has a serious problem: we end up with
different mutexes in different translation units (locale_facets.tcc is
included byt <locale>) and this doesn't seem safe, in general.

I would be happy to be wrong, because the same approach would solve
another issue in our shared_ptr code, as you may have noticed. Is there
something I'm missing?

Thanks,
Paolo.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Patch ping
@ 2005-10-04 16:45 Ian Lance Taylor
  2005-10-07  9:52 ` Paolo Carlini
  0 siblings, 1 reply; 17+ messages in thread
From: Ian Lance Taylor @ 2005-10-04 16:45 UTC (permalink / raw)
  To: libstdc++

[ Forgot to CC libstdc++ on this one ]

Patch for 4.1 regression PR libstdc++/13583 (limited memory leak in
threaded program):
    http://gcc.gnu.org/ml/gcc-patches/2005-09/msg01550.html

Ping!

Ian

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: Patch ping
  2004-06-21 13:38 ` Paolo Bonzini
@ 2004-06-22  2:11   ` Benjamin Kosnik
  0 siblings, 0 replies; 17+ messages in thread
From: Benjamin Kosnik @ 2004-06-22  2:11 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: libstdc++

On Mon, 21 Jun 2004 15:15:12 +0200
Paolo Bonzini <bonzini@gnu.org> wrote:

>http://gcc.gnu.org/ml/gcc-patches/2004-06/msg00049.html

FYI you should post libstdc++ patches to this list, in addition to gcc-patches.

This is fine, I guess. As long as it doesn't mess with PCH under linux I don't care.

-benjamin

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Patch ping
       [not found] <cb66q5$rv3$1@sea.gmane.org>
@ 2004-06-21 13:38 ` Paolo Bonzini
  2004-06-22  2:11   ` Benjamin Kosnik
  0 siblings, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2004-06-21 13:38 UTC (permalink / raw)
  To: libstdc++

> http://gcc.gnu.org/ml/gcc-patches/2004-06/msg00049.html
> Re: [Patch/RFC] Enable PCH for mingw32

This is a very simple patch to add the extension .gch to the libstdc++ 
PCH files (not only the directory).  See also 
http://gcc.gnu.org/ml/gcc-patches/2004-06/msg00031.html saying

 > There is still a bug that I tripped over when testing the libstdc++
 > pch file.
 >
 > On targets that HAVE_TARGET_EXECUTABLE_SUFFIX the command
 >  gcc foo.h -ofoo
 > creates foo.{TARGET_EXEXUTABLE_SUFFIX) not foo.  The problem arise in
 > the call to convert_filename in gcc.c.

It is mostly aesthetic because PCH actually works with libstdc++-v3 on 
mingw32.

Paolo

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2022-01-04 12:45 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-04 12:45 Patch ping Jakub Jelinek
  -- strict thread matches above, loose matches on Subject: below --
2005-10-04 16:45 Ian Lance Taylor
2005-10-07  9:52 ` Paolo Carlini
2005-10-07 18:06   ` Ian Lance Taylor
2005-10-07 19:03     ` Paolo Carlini
2005-10-07 20:05       ` Ian Lance Taylor
2005-10-07 20:25         ` Paolo Carlini
2005-10-07 21:21           ` Peter Dimov
2005-10-07 21:36             ` Jonathan Wakely
2005-10-07 21:40               ` Paolo Carlini
2005-10-07 21:46                 ` Jonathan Wakely
2005-10-07 21:52                   ` Paolo Carlini
2005-10-07 22:07                 ` Ian Lance Taylor
2005-10-08  0:48                   ` Benjamin Kosnik
2005-10-11  0:22                   ` Benjamin Kosnik
     [not found] <cb66q5$rv3$1@sea.gmane.org>
2004-06-21 13:38 ` Paolo Bonzini
2004-06-22  2:11   ` Benjamin Kosnik

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).