| Message ID | ydd7h6mrkbb.fsf@manam.CeBiTec.Uni-Bielefeld.DE |
|---|---|
| State | New |
| Headers | show |
This seems like a problem with the Solaris headers that should be handled by fixincludes. Jason
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)
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.
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
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.
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
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
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
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. */
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.