diff mbox

Elimitate duplication of get_catalogs in different abi

Message ID 55EB586A.8080405@gmail.com
State New
Headers show

Commit Message

François Dumont Sept. 5, 2015, 9:02 p.m. UTC
On 22/08/2015 14:24, Daniel Krügler wrote:
> 2015-08-21 23:11 GMT+02:00 François Dumont <frs.dumont@gmail.com>:
>> I think I found a better way to handle this problem. It is c++locale.cc
>> that needs to be built with --fimplicit-templates. I even think that the
>> *_cow.cc file do not need this option but as I don't know what is the
>> drawback of this option I kept it. I also explicitely used the file name
>> c++locale.cc even if it is an alias to a configurable source file.  I
>> guess there must be some variable to use no ?
>>
>> With this patch there are 6 additional symbols. I guess I need to
>> declare those in the scripts even if it is for internal library usage,
>> right ?
> I would expect that the new Catalog_info definition either has deleted
> or properly (user-)defined copy constructor and copy assignment
> operator.
>
>
> - Daniel
>
This type is used in C++98 so I need to make those private, not deleted.

With this change, is the patch ok to commit ?

François

Comments

François Dumont Sept. 23, 2015, 7:28 p.m. UTC | #1
On 05/09/2015 23:02, François Dumont wrote:
> On 22/08/2015 14:24, Daniel Krügler wrote:
>> 2015-08-21 23:11 GMT+02:00 François Dumont <frs.dumont@gmail.com>:
>>> I think I found a better way to handle this problem. It is c++locale.cc
>>> that needs to be built with --fimplicit-templates. I even think that the
>>> *_cow.cc file do not need this option but as I don't know what is the
>>> drawback of this option I kept it. I also explicitely used the file name
>>> c++locale.cc even if it is an alias to a configurable source file.  I
>>> guess there must be some variable to use no ?
>>>
>>> With this patch there are 6 additional symbols. I guess I need to
>>> declare those in the scripts even if it is for internal library usage,
>>> right ?
>> I would expect that the new Catalog_info definition either has deleted
>> or properly (user-)defined copy constructor and copy assignment
>> operator.
>>
>>
>> - Daniel
>>
> This type is used in C++98 so I need to make those private, not deleted.
>
> With this change, is the patch ok to commit ?
>
> François
>

What about this patch ?

I am still uncomfortable in exposing those implementation details in the
versionned symbols but I don't know how to do otherwise. Do you want me
to push this code in std::__detail namespace ?

François
Jonathan Wakely Sept. 25, 2015, 3:08 p.m. UTC | #2
On 23/09/15 21:28 +0200, François Dumont wrote:
>On 05/09/2015 23:02, François Dumont wrote:
>> On 22/08/2015 14:24, Daniel Krügler wrote:
>>> 2015-08-21 23:11 GMT+02:00 François Dumont <frs.dumont@gmail.com>:
>>>> I think I found a better way to handle this problem. It is c++locale.cc
>>>> that needs to be built with --fimplicit-templates. I even think that the
>>>> *_cow.cc file do not need this option but as I don't know what is the
>>>> drawback of this option I kept it. I also explicitely used the file name
>>>> c++locale.cc even if it is an alias to a configurable source file.  I
>>>> guess there must be some variable to use no ?
>>>>
>>>> With this patch there are 6 additional symbols. I guess I need to
>>>> declare those in the scripts even if it is for internal library usage,
>>>> right ?
>>> I would expect that the new Catalog_info definition either has deleted
>>> or properly (user-)defined copy constructor and copy assignment
>>> operator.
>>>
>>>
>>> - Daniel
>>>
>> This type is used in C++98 so I need to make those private, not deleted.
>>
>> With this change, is the patch ok to commit ?
>>
>> François
>>
>
>What about this patch ?
>
>I am still uncomfortable in exposing those implementation details in the
>versionned symbols but I don't know how to do otherwise. Do you want me
>to push this code in std::__detail namespace ?

I think because the types are only used internally in the library we
don't need to export them. The other code inside the shared library
can refer to those symbols without them being exported.

That way users can't see their names (because they're not in any
public headers) and can't use the symbols (because they're not
exported) so they're pure internal implementation details.

I tested it briefly and it seems to work, so if you can confirm it
still works then the patch is OK without the changes to gnu.ver

Thanks.
Jonathan Wakely Sept. 25, 2015, 3:10 p.m. UTC | #3
On 25/09/15 16:08 +0100, Jonathan Wakely wrote:
>On 23/09/15 21:28 +0200, François Dumont wrote:
>>On 05/09/2015 23:02, François Dumont wrote:
>>>On 22/08/2015 14:24, Daniel Krügler wrote:
>>>>2015-08-21 23:11 GMT+02:00 François Dumont <frs.dumont@gmail.com>:
>>>>>I think I found a better way to handle this problem. It is c++locale.cc
>>>>>that needs to be built with --fimplicit-templates. I even think that the
>>>>>*_cow.cc file do not need this option but as I don't know what is the
>>>>>drawback of this option I kept it. I also explicitely used the file name
>>>>>c++locale.cc even if it is an alias to a configurable source file.  I
>>>>>guess there must be some variable to use no ?
>>>>>
>>>>>With this patch there are 6 additional symbols. I guess I need to
>>>>>declare those in the scripts even if it is for internal library usage,
>>>>>right ?
>>>>I would expect that the new Catalog_info definition either has deleted
>>>>or properly (user-)defined copy constructor and copy assignment
>>>>operator.
>>>>
>>>>
>>>>- Daniel
>>>>
>>>This type is used in C++98 so I need to make those private, not deleted.
>>>
>>>With this change, is the patch ok to commit ?
>>>
>>>François
>>>
>>
>>What about this patch ?
>>
>>I am still uncomfortable in exposing those implementation details in the
>>versionned symbols but I don't know how to do otherwise. Do you want me
>>to push this code in std::__detail namespace ?
>
>I think because the types are only used internally in the library we
>don't need to export them. The other code inside the shared library
>can refer to those symbols without them being exported.
>
>That way users can't see their names (because they're not in any
>public headers) and can't use the symbols (because they're not
>exported) so they're pure internal implementation details.
>
>I tested it briefly and it seems to work, so if you can confirm it
>still works then the patch is OK without the changes to gnu.ver

Oh, the problem is that the symbols are matched by patterns in the
_GLIBCXX_3.4 version, so get exported with that version instead. Gah.

In that case your patch would not have worked on Solaris anyway, as
the SOlaris linker gives an error if a symbol matches patterns in more
than one symbol version.

Let me try to adjust the gnu.ver script to make this work ...
diff mbox

Patch

diff --git a/libstdc++-v3/config/abi/pre/gnu.ver b/libstdc++-v3/config/abi/pre/gnu.ver
index fc97cdf..8f9f99a 100644
--- a/libstdc++-v3/config/abi/pre/gnu.ver
+++ b/libstdc++-v3/config/abi/pre/gnu.ver
@@ -1871,6 +1871,14 @@  GLIBCXX_3.4.22 {
     # std::uncaught_exceptions()
     _ZSt19uncaught_exceptionsv;
 
+    # std::Catalogs::*
+    extern "C++"
+    {
+      std::Catalogs::*;
+    };
+
+    _ZNSt6vectorIPSt12Catalog_info*;
+
 } GLIBCXX_3.4.21;
 
 # Symbols in the support library (libsupc++) have their own tag.
diff --git a/libstdc++-v3/config/locale/gnu/c++locale_internal.h b/libstdc++-v3/config/locale/gnu/c++locale_internal.h
index f1959d6..7db354c 100644
--- a/libstdc++-v3/config/locale/gnu/c++locale_internal.h
+++ b/libstdc++-v3/config/locale/gnu/c++locale_internal.h
@@ -36,8 +36,13 @@ 
 #include <cstddef>
 #include <langinfo.h>
 
+#include <vector>
+#include <string.h>	// ::strdup
+
+#include <ext/concurrence.h>
+
 #if __GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ > 2)
-                                                  
+
 extern "C" __typeof(nl_langinfo_l) __nl_langinfo_l;
 extern "C" __typeof(strcoll_l) __strcoll_l;
 extern "C" __typeof(strftime_l) __strftime_l;
@@ -61,3 +66,55 @@  extern "C" __typeof(wctype_l) __wctype_l;
 #endif 
 
 #endif // GLIBC 2.3 and later
+
+namespace std _GLIBCXX_VISIBILITY(default)
+{
+_GLIBCXX_BEGIN_NAMESPACE_VERSION
+
+  struct Catalog_info
+  {
+    Catalog_info(messages_base::catalog __id, const char* __domain,
+		 locale __loc)
+      : _M_id(__id), _M_domain(strdup(__domain)), _M_locale(__loc)
+    { }
+
+    ~Catalog_info()
+    { free(_M_domain); }
+
+    messages_base::catalog _M_id;
+    char* _M_domain;
+    locale _M_locale;
+
+  private:
+    Catalog_info(const Catalog_info&);
+
+    Catalog_info&
+    operator=(const Catalog_info&);
+  };
+
+  class Catalogs
+  {
+  public:
+    Catalogs() : _M_catalog_counter(0) { }
+    ~Catalogs();
+
+    messages_base::catalog
+    _M_add(const char* __domain, locale __l);
+
+    void
+    _M_erase(messages_base::catalog __c);
+
+    const Catalog_info*
+    _M_get(messages_base::catalog __c) const;
+
+  private:
+    mutable __gnu_cxx::__mutex _M_mutex;
+    messages_base::catalog _M_catalog_counter;
+    vector<Catalog_info*> _M_infos;
+  };
+
+  Catalogs&
+  get_catalogs();
+
+_GLIBCXX_END_NAMESPACE_VERSION
+} // namespace
diff --git a/libstdc++-v3/config/locale/gnu/c_locale.cc b/libstdc++-v3/config/locale/gnu/c_locale.cc
index 4612c64..708af0e 100644
--- a/libstdc++-v3/config/locale/gnu/c_locale.cc
+++ b/libstdc++-v3/config/locale/gnu/c_locale.cc
@@ -31,9 +31,12 @@ 
 #include <locale>
 #include <stdexcept>
 #include <limits>
+#include <algorithm>
 #include <langinfo.h>
 #include <bits/c++locale_internal.h>
 
+#include <backward/auto_ptr.h>
+
 namespace std _GLIBCXX_VISIBILITY(default)
 {
 _GLIBCXX_BEGIN_NAMESPACE_VERSION
@@ -170,6 +173,85 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
     return __changed;
   }
 
+  struct _CatalogIdComp
+  {
+    bool
+    operator()(messages_base::catalog __cat, const Catalog_info* __info) const
+    { return __cat < __info->_M_id; }
+
+    bool
+    operator()(const Catalog_info* __info, messages_base::catalog __cat) const
+    { return __info->_M_id < __cat; }
+  };
+
+  Catalogs::~Catalogs()
+  {
+    for (vector<Catalog_info*>::iterator __it = _M_infos.begin();
+	 __it != _M_infos.end(); ++__it)
+      delete *__it;
+  }
+
+  messages_base::catalog
+  Catalogs::_M_add(const char* __domain, locale __l)
+  {
+    __gnu_cxx::__scoped_lock lock(_M_mutex);
+
+    // The counter is not likely to roll unless catalogs keep on being
+    // opened/closed which is consider as an application mistake for the
+    // moment.
+    if (_M_catalog_counter == numeric_limits<messages_base::catalog>::max())
+      return -1;
+
+    auto_ptr<Catalog_info> info(new Catalog_info(_M_catalog_counter++,
+						 __domain, __l));
+
+    // Check if we managed to allocate memory for domain.
+    if (!info->_M_domain)
+      return -1;
+
+    _M_infos.push_back(info.get());
+    return info.release()->_M_id;
+  }
+
+  void
+  Catalogs::_M_erase(messages_base::catalog __c)
+  {
+    __gnu_cxx::__scoped_lock lock(_M_mutex);
+
+    vector<Catalog_info*>::iterator __res =
+      lower_bound(_M_infos.begin(), _M_infos.end(), __c, _CatalogIdComp());
+    if (__res == _M_infos.end() || (*__res)->_M_id != __c)
+      return;
+
+    delete *__res;
+    _M_infos.erase(__res);
+
+    // Just in case closed catalog was the last open.
+    if (__c == _M_catalog_counter - 1)
+      --_M_catalog_counter;
+  }
+
+  const Catalog_info*
+  Catalogs::_M_get(messages_base::catalog __c) const
+  {
+    __gnu_cxx::__scoped_lock lock(_M_mutex);
+
+    vector<Catalog_info*>::const_iterator __res =
+      lower_bound(_M_infos.begin(), _M_infos.end(), __c, _CatalogIdComp());
+
+    if (__res != _M_infos.end() && (*__res)->_M_id == __c)
+      return *__res;
+
+    return 0;
+  }
+
+  Catalogs&
+  get_catalogs()
+  {
+    static Catalogs __catalogs;
+    return __catalogs;
+  }
+
 _GLIBCXX_END_NAMESPACE_VERSION
 } // namespace
 
@@ -211,3 +293,4 @@  _GLIBCXX_END_NAMESPACE_VERSION
   extern "C" void ldbl (void) __attribute__ ((alias (#dbl)))
 _GLIBCXX_LDBL_COMPAT(_ZSt14__convert_to_vIdEvPKcRT_RSt12_Ios_IostateRKP15__locale_struct, _ZSt14__convert_to_vIeEvPKcRT_RSt12_Ios_IostateRKP15__locale_struct);
 #endif // _GLIBCXX_LONG_DOUBLE_COMPAT
+
diff --git a/libstdc++-v3/config/locale/gnu/messages_members.cc b/libstdc++-v3/config/locale/gnu/messages_members.cc
index 2e6122d..90f4b8d 100644
--- a/libstdc++-v3/config/locale/gnu/messages_members.cc
+++ b/libstdc++-v3/config/locale/gnu/messages_members.cc
@@ -31,115 +31,13 @@ 
 #include <locale>
 #include <bits/c++locale_internal.h>
 
-#include <limits>
-#include <algorithm>
-#include <vector>
 #include <cstdlib>	// std::free
 #include <string.h>	// ::strdup
 
-#include <backward/auto_ptr.h>
-#include <ext/concurrence.h>
-
 namespace
 {
   using namespace std;
 
-  typedef messages_base::catalog catalog;
-
-  struct Catalog_info
-    {
-    Catalog_info(catalog __id, const string& __domain, locale __loc)
-      : _M_id(__id), _M_domain(__domain), _M_locale(__loc)
-    { }
-
-    catalog _M_id;
-    string _M_domain;
-    locale _M_locale;
-  };
-
-  class Catalogs
-  {
-  public:
-    Catalogs() : _M_catalog_counter(0) { }
-
-    ~Catalogs()
-    {
-      for (vector<Catalog_info*>::iterator __it = _M_infos.begin();
-	   __it != _M_infos.end(); ++__it)
-	delete *__it;
-    }
-
-    catalog
-    _M_add(const string& __domain, locale __l)
-    {
-      __gnu_cxx::__scoped_lock lock(_M_mutex);
-
-      // The counter is not likely to roll unless catalogs keep on being
-      // opened/closed which is consider as an application mistake for the
-      // moment.
-      if (_M_catalog_counter == numeric_limits<catalog>::max())
-	return -1;
-
-      std::auto_ptr<Catalog_info> info(new Catalog_info(_M_catalog_counter++,
-							__domain, __l));
-      _M_infos.push_back(info.get());
-      return info.release()->_M_id;
-    }
-
-    void
-    _M_erase(catalog __c)
-    {
-      __gnu_cxx::__scoped_lock lock(_M_mutex);
-
-      vector<Catalog_info*>::iterator __res =
-	lower_bound(_M_infos.begin(), _M_infos.end(), __c, _Comp());
-      if (__res == _M_infos.end() || (*__res)->_M_id != __c)
-	return;
-
-      delete *__res;
-      _M_infos.erase(__res);
-
-      // Just in case closed catalog was the last open.
-      if (__c == _M_catalog_counter - 1)
-	--_M_catalog_counter;
-    }
-
-    const Catalog_info*
-    _M_get(catalog __c) const
-    {
-      __gnu_cxx::__scoped_lock lock(_M_mutex);
-
-      vector<Catalog_info*>::const_iterator __res =
-	lower_bound(_M_infos.begin(), _M_infos.end(), __c, _Comp());
-
-      if (__res != _M_infos.end() && (*__res)->_M_id == __c)
-	return *__res;
-
-      return 0;
-    }
-
-  private:
-    struct _Comp
-    {
-      bool operator()(catalog __cat, const Catalog_info* __info) const
-      { return __cat < __info->_M_id; }
-
-      bool operator()(const Catalog_info* __info, catalog __cat) const
-      { return __info->_M_id < __cat; }
-    };
-
-    mutable __gnu_cxx::__mutex _M_mutex;
-    catalog _M_catalog_counter;
-    std::vector<Catalog_info*> _M_infos;
-  };
-
-  Catalogs&
-  get_catalogs()
-  {
-    static Catalogs __catalogs;
-    return __catalogs;
-  }
-
   const char*
   get_glibc_msg(__c_locale __locale_messages __attribute__((unused)),
 		const char* __name_messages __attribute__((unused)),
@@ -180,7 +78,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
       bind_textdomain_codeset(__s.c_str(),
 	  __nl_langinfo_l(CODESET, __codecvt._M_c_locale_codecvt));
-      return get_catalogs()._M_add(__s, __l);
+      return get_catalogs()._M_add(__s.c_str(), __l);
     }
 
   template<>
@@ -202,7 +100,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	return __dfault;
 
       return get_glibc_msg(_M_c_locale_messages, _M_name_messages,
-			   __cat_info->_M_domain.c_str(),
+			   __cat_info->_M_domain,
 			   __dfault.c_str());
     }
 
@@ -218,7 +116,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       bind_textdomain_codeset(__s.c_str(),
 	  __nl_langinfo_l(CODESET, __codecvt._M_c_locale_codecvt));
 
-      return get_catalogs()._M_add(__s, __l);
+      return get_catalogs()._M_add(__s.c_str(), __l);
     }
 
   template<>
@@ -248,7 +146,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
       __builtin_memset(&__state, 0, sizeof(mbstate_t));
       {
 	const wchar_t* __wdfault_next;
-	size_t __mb_size = __wdfault.size() * __conv.max_length();;
+	size_t __mb_size = __wdfault.size() * __conv.max_length();
 	char* __dfault =
 	  static_cast<char*>(__builtin_alloca(sizeof(char) * (__mb_size + 1)));
 	char* __dfault_next;
@@ -260,7 +158,7 @@  _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	// Make sure string passed to dgettext is \0 terminated.
 	*__dfault_next = '\0';
 	__translation = get_glibc_msg(_M_c_locale_messages, _M_name_messages,
-				      __cat_info->_M_domain.c_str(), __dfault);
+				      __cat_info->_M_domain, __dfault);
 
 	// If we end up getting default value back we can simply return original
 	// default value.
diff --git a/libstdc++-v3/src/c++98/Makefile.am b/libstdc++-v3/src/c++98/Makefile.am
index a5b68a1..c5a8866 100644
--- a/libstdc++-v3/src/c++98/Makefile.am
+++ b/libstdc++-v3/src/c++98/Makefile.am
@@ -155,6 +155,12 @@  vpath % $(top_srcdir)/src/c++98
 
 libc__98convenience_la_SOURCES = $(sources)
 
+# Use special rules to compile with -fimplicit-templates.
+c++locale.lo: c++locale.cc
+	$(LTCXXCOMPILE) -fimplicit-templates -c $<
+c++locale.o: c++locale.cc
+	$(CXXCOMPILE) -fimplicit-templates -c $<
+
 if ENABLE_DUAL_ABI
 GLIBCXX_ABI_FLAGS = -D_GLIBCXX_USE_CXX11_ABI=@glibcxx_cxx98_abi@
 # Use special rules to compile with the non-default string ABI.
diff --git a/libstdc++-v3/src/c++98/Makefile.in b/libstdc++-v3/src/c++98/Makefile.in
index b1a1b49..3c3bbbd 100644
--- a/libstdc++-v3/src/c++98/Makefile.in
+++ b/libstdc++-v3/src/c++98/Makefile.in
@@ -776,6 +776,12 @@  basic_file.cc: ${glibcxx_srcdir}/$(BASIC_FILE_CC)
 	$(LN_S) ${glibcxx_srcdir}/$(BASIC_FILE_CC) ./$@ || true
 
 vpath % $(top_srcdir)/src/c++98
+
+# Use special rules to compile with -fimplicit-templates.
+c++locale.lo: c++locale.cc
+	$(LTCXXCOMPILE) -fimplicit-templates -c $<
+c++locale.o: c++locale.cc
+	$(CXXCOMPILE) -fimplicit-templates -c $<
 # Use special rules to compile with the non-default string ABI.
 @ENABLE_DUAL_ABI_TRUE@collate_members_cow.lo: collate_members_cow.cc
 @ENABLE_DUAL_ABI_TRUE@	$(LTCXXCOMPILE) $(GLIBCXX_ABI_FLAGS) -fimplicit-templates -c $<