Patchwork [c++] Keep tm, div_t, ldiv_t, lconv mangling on Solaris (PR libstdc++-v3/1773)

login
register
mail settings
Submitter Rainer Orth
Date Aug. 9, 2011, 9:44 a.m.
Message ID <ydd7h6mrkbb.fsf@manam.CeBiTec.Uni-Bielefeld.DE>
Download mbox | patch
Permalink /patch/109186/
State New
Headers show

Comments

Rainer Orth - Aug. 9, 2011, 9:44 a.m.
While checking how to fix

PR libstdc++-v3/1773	__cplusplus defined to 1, should be 199711L

it was discovered that without countermeasures, changing __cplusplus
from 1 to 199711L breaks the Solaris ABI by moving tm from the global
namespace to std, among others, e.g.

-FUNC:std::__timepunct<char>::_M_put(char*, unsigned long, char const*, tm const*) const@@GLIBCXX_3.4
+FUNC:std::__timepunct<char>::_M_put(char*, unsigned long, char const*, std::tm const*) const@@GLIBCXX_3.4
-FUNC:std::__timepunct<wchar_t>::_M_put(wchar_t*, unsigned long, wchar_t const*, tm const*) const@@GLIBCXX_3.4
+FUNC:std::__timepunct<wchar_t>::_M_put(wchar_t*, unsigned long, wchar_t const*, std::tm const*) const@@GLIBCXX_3.4

This patch (entirely Marc's work, I've only added the ChangeLog entry
and fixed some formatting issues) avoids that.

Together with the companion patches (already posted or in the process of
being so), it allowed successful i386-pc-solaris2.{8, 9, 10, 11}
bootstraps without regressions.

I have no idea if the approach is right, but something needs to be done
here.

Comments?

	Rainer


2011-08-07  Marc Glisse  <marc.glisse@normalesup.org>

	PR libstdc++-v3/1773
	* mangle.c (substitution_identifier_index_t): Define SUBID_TM,
	SUBID_DIV_T, SUBID_LDIV_T, SUBID_LCONV.
	(is_std_substitution2): New function.
	(write_unscoped_name): Don't emit tm, div_t, ldiv_t, lconv in ::std.
	(init_mangle): Cache tm, div_t, ldiv_t, lconv identifies.
Jason Merrill - Aug. 10, 2011, 2:56 p.m.
This seems like a problem with the Solaris headers that should be 
handled by fixincludes.

Jason
Marc Glisse - Aug. 10, 2011, 4:43 p.m.
On Wed, 10 Aug 2011, Jason Merrill wrote:

> This seems like a problem with the Solaris headers that should be handled by 
> fixincludes.

The goal of the set of patches, apart from defining __cplusplus=199711L, 
was to start using the Solaris headers properly, without setting weird 
macros to ignore half of them or fixincluding half of their content out. 
Fixincluding this seems like a shame. Solaris headers declare std::tm, 
which is great. It just happens to break binary compatibility, so until 
the next great g++ ABI break, we need some kind of workaround. Telling the 
compiler that std::tm should be mangled as if it was ::tm looked like a 
simple enough solution.

We could of course surround the 4 struct definitions with:
#if __cplusplus >= 199711L
}
#endif

and:
#if __cplusplus >= 199711L
namespace std {
using ::thing;
#endif

Or the switch to __cplusplus=199711L will have to wait until the next ABI 
break...

I understand that you may not be happy with the idea of touching the 
mangler for a platform-specific temporary issue.

(I am speaking independently of the quality of the patch itself)
Paolo Carlini - Aug. 10, 2011, 5:30 p.m.
Hi,
>> This seems like a problem with the Solaris headers that should be 
>> handled by fixincludes.
>
> The goal of the set of patches, apart from defining 
> __cplusplus=199711L, was to start using the Solaris headers properly, 
> without setting weird macros to ignore half of them or fixincluding 
> half of their content out. Fixincluding this seems like a shame. 
> Solaris headers declare std::tm, which is great. It just happens to 
> break binary compatibility, so until the next great g++ ABI break, we 
> need some kind of workaround. Telling the compiler that std::tm should 
> be mangled as if it was ::tm looked like a simple enough solution.
>
> We could of course surround the 4 struct definitions with:
> #if __cplusplus >= 199711L
> }
> #endif
>
> and:
> #if __cplusplus >= 199711L
> namespace std {
> using ::thing;
> #endif
>
> Or the switch to __cplusplus=199711L will have to wait until the next 
> ABI break...
All in all it seems to me that we are pretty close to be able to fix 
this old issue now and we are even in Stage 1, thus we can afford to 
take a bit of risk and handle possible fallout, thus I would recommend 
we do for now the above preprocessor dance (we are talking only about 4 
instances) but controlled by a macro in os_defines.h, as Rainer 
correctly did already elsewhere. Rainer, can you test such change?

Thanks,
Paolo.
Rainer Orth - Aug. 11, 2011, 2:49 p.m.
Hi Paolo,

> All in all it seems to me that we are pretty close to be able to fix this
> old issue now and we are even in Stage 1, thus we can afford to take a bit
> of risk and handle possible fallout, thus I would recommend we do for now
> the above preprocessor dance (we are talking only about 4 instances) but
> controlled by a macro in os_defines.h, as Rainer correctly did already
> elsewhere. Rainer, can you test such change?

I could, but am a bit reluctant to do so since such a fix feels quite
fragile, and `fixes' the Solaris headers in many places where they are
completely correct.  I'll also have to touch <time.h>, <wchar.h>,
<stdlib.h>, and <locale.h>, that all have using std::tm etc. clauses.

There might be an alternative implementation that is less invasive to
the C++ frontend, though: add

	&& TARGET_DECL_NAMESPACE_STD_P (decl)

in write_unscoped_name, defaulting to true, override it in sol2.h (which
gets included via tm.h) and have the remaining logic in a new sol2-cxx.c
file.  This way, the impact on the C++ frontend is minimal and we don't
have to resort to fragile fixincludes hacks.

Does this sound like a acceptable/viable alternative?

Thanks.
        Rainer
Marc Glisse - Aug. 11, 2011, 3:07 p.m.
On Thu, 11 Aug 2011, Rainer Orth wrote:

> I could, but am a bit reluctant to do so since such a fix feels quite
> fragile, and `fixes' the Solaris headers in many places where they are
> completely correct.  I'll also have to touch <time.h>, <wchar.h>,
> <stdlib.h>, and <locale.h>, that all have using std::tm etc. clauses.

Actually, you wouldn't need to touch those other places:

struct tm;
namespace std { using ::tm; }
using std::tm;

is perfectly ok, so it would really be only the four struct declarations.

(I am not saying that against your proposal below at all, just making sure 
we understand the issue)

> There might be an alternative implementation that is less invasive to
> the C++ frontend, though: add
>
> 	&& TARGET_DECL_NAMESPACE_STD_P (decl)
>
> in write_unscoped_name, defaulting to true, override it in sol2.h (which
> gets included via tm.h) and have the remaining logic in a new sol2-cxx.c
> file.  This way, the impact on the C++ frontend is minimal and we don't
> have to resort to fragile fixincludes hacks.
Rainer Orth - Aug. 11, 2011, 3:10 p.m.
Marc,

> On Thu, 11 Aug 2011, Rainer Orth wrote:
>
>> I could, but am a bit reluctant to do so since such a fix feels quite
>> fragile, and `fixes' the Solaris headers in many places where they are
>> completely correct.  I'll also have to touch <time.h>, <wchar.h>,
>> <stdlib.h>, and <locale.h>, that all have using std::tm etc. clauses.
>
> Actually, you wouldn't need to touch those other places:
>
> struct tm;
> namespace std { using ::tm; }
> using std::tm;
>
> is perfectly ok, so it would really be only the four struct declarations.

good to know.  I hadn't tried this yet, and am practically ignorant of
C++ ;-)

> (I am not saying that against your proposal below at all, just making sure 
> we understand the issue)

Understood, especially given that the mangling code is yours :-)

Thanks.
        Rainer
Jason Merrill - Aug. 11, 2011, 8:48 p.m.
On 08/11/2011 10:49 AM, Rainer Orth wrote:
> There might be an alternative implementation that is less invasive to
> the C++ frontend, though: add
>
> 	&&  TARGET_DECL_NAMESPACE_STD_P (decl)
>
> in write_unscoped_name, defaulting to true, override it in sol2.h (which
> gets included via tm.h) and have the remaining logic in a new sol2-cxx.c
> file.

I would be OK with a target hook.  I think a cleaner place would be to 
hook decl_mangling_context and then change write_unscoped_name to use 
decl_mangling_context instead of CP_DECL_CONTEXT.

Jason
Rainer Orth - Aug. 12, 2011, 9:49 a.m.
Jason Merrill <jason@redhat.com> writes:

> On 08/11/2011 10:49 AM, Rainer Orth wrote:
>> There might be an alternative implementation that is less invasive to
>> the C++ frontend, though: add
>>
>> 	&&  TARGET_DECL_NAMESPACE_STD_P (decl)
>>
>> in write_unscoped_name, defaulting to true, override it in sol2.h (which
>> gets included via tm.h) and have the remaining logic in a new sol2-cxx.c
>> file.
>
> I would be OK with a target hook.  I think a cleaner place would be to hook

I'd found that this is the way to go when having a more detailed look
last night.

> decl_mangling_context and then change write_unscoped_name to use
> decl_mangling_context instead of CP_DECL_CONTEXT.

Thanks for the hint, I'll see what I can come up with.

	Rainer

Patch

diff --git a/gcc/cp/mangle.c b/gcc/cp/mangle.c
--- a/gcc/cp/mangle.c
+++ b/gcc/cp/mangle.c
@@ -127,6 +127,10 @@  typedef enum
   SUBID_BASIC_ISTREAM,
   SUBID_BASIC_OSTREAM,
   SUBID_BASIC_IOSTREAM,
+  SUBID_TM,
+  SUBID_DIV_T,
+  SUBID_LDIV_T,
+  SUBID_LCONV,
   SUBID_MAX
 }
 substitution_identifier_index_t;
@@ -163,6 +167,8 @@  static inline tree canonicalize_for_subs
 static void add_substitution (tree);
 static inline int is_std_substitution (const tree,
 				       const substitution_identifier_index_t);
+static inline int is_std_substitution2 (const tree,
+					const substitution_identifier_index_t);
 static inline int is_std_substitution_char (const tree,
 					    const substitution_identifier_index_t);
 static int find_substitution (tree);
@@ -437,6 +443,33 @@  is_std_substitution (const tree node,
 	      == subst_identifiers[index]));
 }
 
+static inline int
+is_std_substitution2 (const tree node,
+		      const substitution_identifier_index_t index)
+{
+  tree type = NULL;
+  tree decl = NULL;
+
+  if (DECL_P (node))
+    {
+      type = TREE_TYPE (node);
+      decl = node;
+    }
+  else if (CLASS_TYPE_P (node))
+    {
+      type = node;
+      decl = TYPE_NAME (node);
+    }
+  else
+    /* These are not the droids you're looking for.  */
+    return 0;
+
+  return (DECL_NAMESPACE_STD_P (CP_DECL_CONTEXT (decl))
+	  && TYPE_LANG_SPECIFIC (type)
+	  && (DECL_NAME (decl) == subst_identifiers[index]));
+}
+
+
 /* Helper function for find_substitution.  Returns nonzero if NODE,
    which may be a decl or a CLASS_TYPE, is the template-id
    ::std::identifier<char>, where identifier is
@@ -864,6 +897,10 @@  write_unscoped_name (const tree decl)
   /* Is DECL in ::std?  */
   if (DECL_NAMESPACE_STD_P (context))
     {
+      if (!is_std_substitution2 (decl, SUBID_TM)
+	  && !is_std_substitution2 (decl, SUBID_DIV_T)
+	  && !is_std_substitution2 (decl, SUBID_LDIV_T)
+	  && !is_std_substitution2 (decl, SUBID_LCONV))
       write_string ("St");
       write_unqualified_name (decl);
     }
@@ -3095,6 +3132,10 @@  init_mangle (void)
   subst_identifiers[SUBID_BASIC_ISTREAM] = get_identifier ("basic_istream");
   subst_identifiers[SUBID_BASIC_OSTREAM] = get_identifier ("basic_ostream");
   subst_identifiers[SUBID_BASIC_IOSTREAM] = get_identifier ("basic_iostream");
+  subst_identifiers[SUBID_TM] = get_identifier ("tm");
+  subst_identifiers[SUBID_DIV_T] = get_identifier ("div_t");
+  subst_identifiers[SUBID_LDIV_T] = get_identifier ("ldiv_t");
+  subst_identifiers[SUBID_LCONV] = get_identifier ("lconv");
 }
 
 /* Generate the mangled name of DECL.  */