public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* Libstdc++ as DLL on windows, alternative approach [was Re: cygwin  patch review]
       [not found] <20091019092452.2e271791@mcgee.artheist.org>
@ 2009-11-12 22:33 ` Dave Korn
  2009-11-16  2:57   ` Dave Korn
                     ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Dave Korn @ 2009-11-12 22:33 UTC (permalink / raw)
  To: Benjamin Kosnik
  Cc: libstdc++, GCC Patches, Danny Smith, Aaron W. LaFramboise (GCC),
	Kai Tietz, Dave Korn

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

Benjamin Kosnik wrote:
> From older mail thread:

  Likewise.  I had to take some time away from this to do other stuff, but
I've gotten back to it this week.

> No. There is already visibility markup on libstdc++ headers for this.
> You can just put the attribute visibility bits on namespace std, and be
> done with it.
> 
> Here, from include/bits/c++config.h:
> 
> # define _GLIBCXX_BEGIN_NAMESPACE(X) namespace X
> _GLIBCXX_VISIBILITY_ATTR(default)

  Ok, so as we discussed before, this doesn't currently work.  However it does
sound like the right thing to do, so I've spun a new version of the patch that
applies dllimport to the namespace.  We could commit this and then open a PR
about dllimport not working on namespaces.  It's much smaller and cleaner than
the version that adds markup everywhere, but I doubt I'll be able to fix the
dllimport-vs-namespace bug in time for 4.5.  That's not the end of the world;
getting dllimport right is a good optimisation, but we can live with the
compromise of auto-import for now.

  Initial test results (c and c++ only) are posted(*) and look right, but I'll
do a clean run on the same source revision for comparison anyway.  Note that I
had to patch various testsuite/lib/ files to avoid the current utf-8
problem(**) that would otherwise have provoked many spurious 'excess errors'
warnings.  That'll be the subject of a separate patch, I'll do the clean run
using them as well so the comparison is fair.

gcc/ChangeLog:

	* configure.ac (USE_CYGWIN_LIBSTDCXX_WRAPPERS): Define to reflect
	status of AC_CHECK_FUNC for Cygwin DLL libstdc++ support wrappers.
	* configure: Regenerate.
	* config.in: Regenerate.

	* config/i386/cygwin.h (CXX_WRAP_SPEC_LIST): Define list of --wrap
	options for Cygwin DLL libstdc++ support wrappers.
	(CXX_WRAP_SPEC_OPT): Define spec to use wrappers or not by default
	according to defined value of USE_CYGWIN_LIBSTDCXX_WRAPPERS.
	(CXX_WRAP_SPEC): Define entire wrapper spec in or out according to
	whether USE_CYGWIN_LIBSTDCXX_WRAPPERS is even defined or not.
	(LINK_SPEC): Include CXX_WRAP_SPEC.
	* gcc/config/i386/winnt.c (wrapper_strcmp): New qsort helper function.
	(i386_find_on_wrapper_list): Check if a function is found on the list
	of libstdc++ wrapper options.
	(i386_pe_file_end): If we are importing a wrapped function, also emit
	an external declaration for the real version.
	* config/i386/cygming.opt (muse-libstdc-wrappers): New option for
	Cygwin targets. Update copyright year.

	* gcc.c (LINK_COMMAND_SPEC): Allow and ignore -static-libstdc++
	similarly to -static.

gcc/cp/ChangeLog:

	* g++spec.c (SKIPOPT): Delete.
	(lang_specific_driver): Do not skip -static-libstdc++ option.

libstdc++-v3/ChangeLog:

	* libstdc++-v3/acinclude.m4 (GLIBCXX_ENABLE_SYMVERS): Don't disable
	on PE targets.
	* libstdc++-v3/configure: Regenerate.
	* libstdc++-v3/configure.host: Add libtool DLL options for Cygwin
	and MinGW platforms.

	* libstdc++-v3/include/bits/c++config (_GLIBCXX_VISIBILITY_ATTR): On
	platforms that don't support visibility, allow them to declare a macro
	_GLIBCXX_PSEUDO_VISIBILITY that is applied in place of visibility.

	* libstdc++-v3/config/os/newlib/os_defines.h
	(_GLIBCXX_PSEUDO_VISIBILITY_default): New macro for dllimport.
	(_GLIBCXX_PSEUDO_VISIBILITY_hidden): New empty macro.
	(_GLIBCXX_PSEUDO_VISIBILITY): Evaluate to one of the above.
	* libstdc++-v3/config/os/mingw32/os_defines.h
	(_GLIBCXX_PSEUDO_VISIBILITY_default,
	_GLIBCXX_PSEUDO_VISIBILITY_hidden,
	_GLIBCXX_PSEUDO_VISIBILITY): Likewise.

  So, is this any more like OK for head?

    cheers,
      DaveK
-- 
(*)  - http://gcc.gnu.org/ml/gcc-testresults/2009-11/msg01084.html
(**) - http://gcc.gnu.org/ml/gcc/2009-10/threads.html#00480


[-- Attachment #2: libstdc-dll-vis.diff --]
[-- Type: text/x-c, Size: 12694 bytes --]

Index: gcc/gcc.c
===================================================================
--- gcc/gcc.c	(revision 154011)
+++ gcc/gcc.c	(working copy)
@@ -766,7 +766,10 @@ proper position among the other output files.  */
 
 /* -u* was put back because both BSD and SysV seem to support it.  */
 /* %{static:} simply prevents an error message if the target machine
-   doesn't handle -static.  */
+   doesn't handle -static; %{static-libstdc++} is a C++ FE option, 
+   handled entirely in cp/g++spec.c, yet we cannot just remove it
+   from the command-line there or it cannot be tested in target spec
+   processing, so we simply ignore it here similarly.  */
 /* We want %{T*} after %{L*} and %D so that it can be used to specify linker
    scripts which exist in user specified directories, or in standard
    directories.  */
@@ -789,7 +792,7 @@ proper position among the other output files.  */
     %{flto} %{fwhopr} %l " LINK_PIE_SPEC \
    "%X %{o*} %{A} %{d} %{e*} %{m} %{N} %{n} %{r}\
     %{s} %{t} %{u*} %{x} %{z} %{Z} %{!A:%{!nostdlib:%{!nostartfiles:%S}}}\
-    %{static:} %{L*} %(mfwrap) %(link_libgcc) %o\
+    %{static:} %{static-libstdc++:} %{L*} %(mfwrap) %(link_libgcc) %o\
     %{fopenmp|ftree-parallelize-loops=*:%:include(libgomp.spec)%(link_gomp)} %(mflib)\
     %{fprofile-arcs|fprofile-generate*|coverage:-lgcov}\
     %{!nostdlib:%{!nodefaultlibs:%(link_ssp) %(link_gcc_c_sequence)}}\
Index: gcc/cp/g++spec.c
===================================================================
--- gcc/cp/g++spec.c	(revision 154011)
+++ gcc/cp/g++spec.c	(working copy)
@@ -30,8 +30,6 @@ along with GCC; see the file COPYING3.  If not see
 #define MATHLIB		(1<<2)
 /* This bit is set if they did `-lc'.  */
 #define WITHLIBC	(1<<3)
-/* Skip this option.  */
-#define SKIPOPT		(1<<4)
 
 #ifndef MATH_LIBRARY
 #define MATH_LIBRARY "-lm"
@@ -211,10 +209,7 @@ lang_specific_driver (int *in_argc, const char *co
 	  else if (strcmp (argv[i], "-static-libgcc") == 0)
 	    shared_libgcc = 0;
 	  else if (strcmp (argv[i], "-static-libstdc++") == 0)
-	    {
 	      library = library >= 0 ? 2 : library;
-	      args[i] |= SKIPOPT;
-	    }
 	  else if (DEFAULT_WORD_SWITCH_TAKES_ARG (&argv[i][1]))
 	    i++;
 	  else
@@ -324,9 +319,6 @@ lang_specific_driver (int *in_argc, const char *co
 	  arglist[j] = "-xnone";
 	}
 
-      if ((args[i] & SKIPOPT) != 0)
-	--j;
-
       i++;
       j++;
     }
Index: gcc/configure.ac
===================================================================
--- gcc/configure.ac	(revision 154011)
+++ gcc/configure.ac	(working copy)
@@ -2989,6 +2989,25 @@ changequote(,)dnl
   i[34567]86-*-* | x86_64-*-*)
 changequote([,])dnl
     case $target_os in
+      cygwin*)
+	# Full C++ conformance when using a shared libstdc++-v3 requires some
+	# support from the Cygwin DLL, which in more recent versions exports
+	# wrappers to aid in interposing and redirecting operators new, delete,
+	# etc., as per n2800 #17.6.4.6 [replacement.functions].  Check if we
+	# are configuring for a version of Cygwin that exports the wrappers.
+	if test x$host = x$target; then
+	  AC_CHECK_FUNC([__wrap__Znaj],[gcc_ac_cygwin_dll_wrappers=yes],[gcc_ac_cygwin_dll_wrappers=no])
+	else
+	  # Can't check presence of libc functions during cross-compile, so
+	  # we just have to assume we're building for an up-to-date target.
+	  gcc_ac_cygwin_dll_wrappers=yes
+	fi
+	AC_DEFINE_UNQUOTED(USE_CYGWIN_LIBSTDCXX_WRAPPERS,
+	  [`if test $gcc_ac_cygwin_dll_wrappers = yes; then echo 1; else echo 0; fi`],
+	  [Define if you want to generate code by default that assumes that the
+	   Cygwin DLL exports wrappers to support libstdc++ function replacement.])
+    esac
+    case $target_os in
       cygwin* | pe | mingw32*)
 	# Recent binutils allows the three-operand form of ".comm" on PE.  This
 	# definition is used unconditionally to initialise the default state of
Index: gcc/config/i386/cygwin.h
===================================================================
--- gcc/config/i386/cygwin.h	(revision 154011)
+++ gcc/config/i386/cygwin.h	(working copy)
@@ -85,9 +85,41 @@ along with GCC; see the file COPYING3.  If not see
   %{mwindows:-lgdi32 -lcomdlg32} \
   -luser32 -lkernel32 -ladvapi32 -lshell32"
 
+/* To implement C++ function replacement we always wrap the cxx
+   malloc-like operators.  See N2800 #17.6.4.6 [replacement.functions] */
+#define CXX_WRAP_SPEC_LIST "%{!static: %{!static-libstdc++: \
+  --wrap _Znwj \
+  --wrap _Znaj \
+  --wrap _ZdlPv \
+  --wrap _ZdaPv \
+  --wrap _ZnwjRKSt9nothrow_t \
+  --wrap _ZnajRKSt9nothrow_t \
+  --wrap _ZdlPvRKSt9nothrow_t \
+  --wrap _ZdaPvRKSt9nothrow_t \
+  }}"
+
+#if defined (USE_CYGWIN_LIBSTDCXX_WRAPPERS)
+
+#if USE_CYGWIN_LIBSTDCXX_WRAPPERS
+/* Default on, only explict -mno disables.  */
+#define CXX_WRAP_SPEC_OPT "!mno-use-libstdc-wrappers"
+#else
+/* Default off, only explict -m enables.  */
+#define CXX_WRAP_SPEC_OPT "muse-libstdc-wrappers"
+#endif
+
+#define CXX_WRAP_SPEC "%{" CXX_WRAP_SPEC_OPT ":" CXX_WRAP_SPEC_LIST "}"
+
+#else /* !defined (USE_CYGWIN_LIBSTDCXX_WRAPPERS)  */
+
+#define CXX_WRAP_SPEC ""
+
+#endif /* ?defined (USE_CYGWIN_LIBSTDCXX_WRAPPERS) */
+
 #define LINK_SPEC "\
   %{mwindows:--subsystem windows} \
   %{mconsole:--subsystem console} \
+  " CXX_WRAP_SPEC " \
   %{shared: %{mdll: %eshared and mdll are not compatible}} \
   %{shared: --shared} %{mdll:--dll} \
   %{static:-Bstatic} %{!static:-Bdynamic} \
Index: gcc/config/i386/winnt.c
===================================================================
--- gcc/config/i386/winnt.c	(revision 154011)
+++ gcc/config/i386/winnt.c	(working copy)
@@ -603,6 +603,64 @@ i386_pe_maybe_record_exported_symbol (tree decl, c
   export_head = p;
 }
 
+#ifdef CXX_WRAP_SPEC_LIST
+
+/*  Hash table equality helper function.  */
+
+static int
+wrapper_strcmp (const void *x, const void *y)
+{
+  return !strcmp ((const char *) x, (const char *) y);
+}
+
+/* Search for a function named TARGET in the list of library wrappers
+   we are using, returning a pointer to it if found or NULL if not.
+   This function might be called on quite a few symbols, and we only
+   have the list of names of wrapped functions available to us as a
+   spec string, so first time round we lazily initialise a hash table
+   to make things quicker.  */
+
+static const char *
+i386_find_on_wrapper_list (const char *target)
+{
+  static char first_time = 1;
+  static htab_t wrappers;
+
+  if (first_time)
+    {
+      /* Beware that this is not a complicated parser, it assumes
+         that any sequence of non-whitespace beginning with an
+	 underscore is one of the wrapped symbols.  For now that's
+	 adequate to distinguish symbols from spec substitutions
+	 and command-line options.  */
+      static char wrapper_list_buffer[] = CXX_WRAP_SPEC_LIST;
+      char *bufptr;
+      /* Breaks up the char array into separated strings
+         strings and enter them into the hash table.  */
+      wrappers = htab_create_alloc (8, htab_hash_string, wrapper_strcmp,
+	0, xcalloc, free);
+      for (bufptr = wrapper_list_buffer; *bufptr; ++bufptr)
+	{
+	  char *found = NULL;
+	  if (ISSPACE (*bufptr))
+	    continue;
+	  if (*bufptr == '_')
+	    found = bufptr;
+	  while (*bufptr && !ISSPACE (*bufptr))
+	    ++bufptr;
+	  if (*bufptr)
+	    *bufptr = 0;
+	  if (found)
+	    *htab_find_slot (wrappers, found, INSERT) = found;
+	}
+      first_time = 0;
+    }
+
+  return (const char *) htab_find (wrappers, target);
+}
+
+#endif /* CXX_WRAP_SPEC_LIST */
+
 /* This is called at the end of assembly.  For each external function
    which has not been defined, we output a declaration now.  We also
    output the .drectve section.  */
@@ -624,6 +682,15 @@ i386_pe_file_end (void)
       if (! TREE_ASM_WRITTEN (decl)
 	  && TREE_SYMBOL_REFERENCED (DECL_ASSEMBLER_NAME (decl)))
 	{
+#ifdef CXX_WRAP_SPEC_LIST
+	  /* To ensure the DLL that provides the corresponding real
+	     functions is still loaded at runtime, we must reference
+	     the real function so that an (unused) import is created.  */
+	  const char *realsym = i386_find_on_wrapper_list (p->name);
+	  if (realsym)
+	    i386_pe_declare_function_type (asm_out_file,
+		concat ("__real_", realsym, NULL), TREE_PUBLIC (decl));
+#endif /* CXX_WRAP_SPEC_LIST */
 	  TREE_ASM_WRITTEN (decl) = 1;
 	  i386_pe_declare_function_type (asm_out_file, p->name,
 					 TREE_PUBLIC (decl));
Index: gcc/config/i386/cygming.opt
===================================================================
--- gcc/config/i386/cygming.opt	(revision 154011)
+++ gcc/config/i386/cygming.opt	(working copy)
@@ -1,6 +1,6 @@
 ; Cygwin- and MinGW-specific options.
 
-; Copyright (C) 2005, 2007 Free Software Foundation, Inc.
+; Copyright (C) 2005, 2007, 2009 Free Software Foundation, Inc.
 ;
 ; This file is part of GCC.
 ;
@@ -49,3 +49,7 @@ Create GUI application
 mpe-aligned-commons
 Target Var(use_pe_aligned_common) Init(HAVE_GAS_ALIGNED_COMM)
 Use the GNU extension to the PE format for aligned common data
+
+muse-libstdc-wrappers
+Target Condition({defined (USE_CYGWIN_LIBSTDCXX_WRAPPERS)})
+Compile code that relies on Cygwin DLL wrappers to support C++ operator new/delete replacement
Index: libstdc++-v3/configure
===================================================================
--- libstdc++-v3/configure	(revision 154011)
+++ libstdc++-v3/configure	(working copy)
@@ -58050,7 +58050,7 @@ if test x$enable_symvers = xyes ; then
   else
     if test $with_gnu_ld = yes ; then
       case ${target_os} in
-        cygwin* | pe | mingw32* | hpux*)
+        hpux*)
           enable_symvers=no ;;
         *)
           enable_symvers=gnu ;;
Index: libstdc++-v3/configure.host
===================================================================
--- libstdc++-v3/configure.host	(revision 154011)
+++ libstdc++-v3/configure.host	(working copy)
@@ -209,6 +209,7 @@ case "${host_os}" in
     ;;
   cygwin*)
     os_include_dir="os/newlib"
+    OPT_LDFLAGS="${OPT_LDFLAGS} -no-undefined -bindir \$(bindir)"
     ;;
   darwin | darwin[1-7] | darwin[1-7].*)
     # On Darwin, performance is improved if libstdc++ is single-module.
@@ -258,6 +259,7 @@ case "${host_os}" in
   mingw32*)
     os_include_dir="os/mingw32"
     error_constants_dir="os/mingw32"
+    OPT_LDFLAGS="${OPT_LDFLAGS} -no-undefined -bindir \$(bindir)"
     ;;
   netbsd*)
     os_include_dir="os/bsd/netbsd"
Index: libstdc++-v3/include/bits/c++config
===================================================================
--- libstdc++-v3/include/bits/c++config	(revision 154011)
+++ libstdc++-v3/include/bits/c++config	(working copy)
@@ -41,6 +41,8 @@
 
 #if _GLIBCXX_HAVE_ATTRIBUTE_VISIBILITY
 # define _GLIBCXX_VISIBILITY_ATTR(V) __attribute__ ((__visibility__ (#V)))
+#elif defined (_GLIBCXX_PSEUDO_VISIBILITY)
+# define _GLIBCXX_VISIBILITY_ATTR(V) _GLIBCXX_PSEUDO_VISIBILITY(V)
 #else
 # define _GLIBCXX_VISIBILITY_ATTR(V) 
 #endif
Index: libstdc++-v3/config/os/newlib/os_defines.h
===================================================================
--- libstdc++-v3/config/os/newlib/os_defines.h	(revision 154011)
+++ libstdc++-v3/config/os/newlib/os_defines.h	(working copy)
@@ -36,6 +36,15 @@
 #ifdef __CYGWIN__
 #define _GLIBCXX_GTHREAD_USE_WEAK 0
 
+#if defined (_GLIBCXX_DLL)
+#define _GLIBCXX_PSEUDO_VISIBILITY_default __attribute__ ((__dllimport__))
+#else
+#define _GLIBCXX_PSEUDO_VISIBILITY_default
+#endif
+#define _GLIBCXX_PSEUDO_VISIBILITY_hidden
+
+#define _GLIBCXX_PSEUDO_VISIBILITY(V) _GLIBCXX_PSEUDO_VISIBILITY_ ## V
+
 // See libstdc++/20806.
 #define _GLIBCXX_HAVE_DOS_BASED_FILESYSTEM 1
 #endif
Index: libstdc++-v3/config/os/mingw32/os_defines.h
===================================================================
--- libstdc++-v3/config/os/mingw32/os_defines.h	(revision 154011)
+++ libstdc++-v3/config/os/mingw32/os_defines.h	(working copy)
@@ -45,6 +45,15 @@
 #undef NOMINMAX
 #define NOMINMAX 1
 
+#if defined (_GLIBCXX_DLL)
+#define _GLIBCXX_PSEUDO_VISIBILITY_default __attribute__ ((__dllimport__))
+#else
+#define _GLIBCXX_PSEUDO_VISIBILITY_default
+#endif
+#define _GLIBCXX_PSEUDO_VISIBILITY_hidden
+
+#define _GLIBCXX_PSEUDO_VISIBILITY(V) _GLIBCXX_PSEUDO_VISIBILITY_ ## V
+
 // See libstdc++/20806.
 #define _GLIBCXX_HAVE_DOS_BASED_FILESYSTEM 1
 
Index: libstdc++-v3/acinclude.m4
===================================================================
--- libstdc++-v3/acinclude.m4	(revision 154011)
+++ libstdc++-v3/acinclude.m4	(working copy)
@@ -2737,7 +2737,7 @@ if test x$enable_symvers = xyes ; then
   else
     if test $with_gnu_ld = yes ; then
       case ${target_os} in
-        cygwin* | pe | mingw32* | hpux*)
+        hpux*)
           enable_symvers=no ;;
         *)
           enable_symvers=gnu ;;

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

* Re: Libstdc++ as DLL on windows, alternative approach [was Re: cygwin  patch review]
  2009-11-12 22:33 ` Libstdc++ as DLL on windows, alternative approach [was Re: cygwin patch review] Dave Korn
@ 2009-11-16  2:57   ` Dave Korn
  2009-11-16  3:34   ` Danny Smith
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 10+ messages in thread
From: Dave Korn @ 2009-11-16  2:57 UTC (permalink / raw)
  To: Dave Korn
  Cc: Benjamin Kosnik, libstdc++,
	GCC Patches, Danny Smith, Aaron W. LaFramboise (GCC),
	Kai Tietz

Dave Korn wrote:

>   Initial test results (c and c++ only) are posted(*) and look right, but I'll
> do a clean run on the same source revision for comparison anyway.  Note that I
> had to patch various testsuite/lib/ files to avoid the current utf-8
> problem(**) that would otherwise have provoked many spurious 'excess errors'
> warnings.  That'll be the subject of a separate patch, I'll do the clean run
> using them as well so the comparison is fair.

  In fact I found that my first version of that fix wasn't quite right, which
accounts for all but one of the discrepancies between these two sets of results:

  Libstdc dll results
http://gcc.gnu.org/ml/gcc-testresults/2009-11/msg01084.html

  Clean run results:
http://gcc.gnu.org/ml/gcc-testresults/2009-11/msg01432.html

  For the clean run, I used LC_ALL=C.ASCII.  My first try (when I did the
libstdc++ dll tests) had C.CP437 which led to spurious 'test for/excess
errors' failures in the gcc.dg/cpp/_Pragma3.c, gcc.dg/attr-alias-5.c and
gcc.dg/ucnid-*.c tests.  Updating the test scripts to correctly use C.ASCII
and re-running the failing tests in the original libstdc++ dll build tree
shows them all passing as expected.

  The one remaining difference between the two sets of results is in the
g++.dg/tree-prof/partition1.C results; this is also an artifact, as I was
inadvertently using an older binutils when I tested the libstdc++ dll build,
and the linker script choked on the .text.unlikely section.  We fixed this
recently in binutils CVS, and using a more recent build of the linker fixed
the apparent failure.

  So: there are no regressions caused by the libstdc++ dll patch.

    cheers,
      DaveK

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

* Re: Libstdc++ as DLL on windows, alternative approach [was Re: cygwin   patch review]
  2009-11-12 22:33 ` Libstdc++ as DLL on windows, alternative approach [was Re: cygwin patch review] Dave Korn
  2009-11-16  2:57   ` Dave Korn
@ 2009-11-16  3:34   ` Danny Smith
  2009-11-16  3:37     ` Dave Korn
  2009-11-19  1:25   ` Dave Korn
  2009-11-19 17:39   ` Benjamin Kosnik
  3 siblings, 1 reply; 10+ messages in thread
From: Danny Smith @ 2009-11-16  3:34 UTC (permalink / raw)
  To: Dave Korn
  Cc: Benjamin Kosnik, libstdc++,
	GCC Patches, Aaron W. LaFramboise (GCC),
	Kai Tietz

On Fri, Nov 13, 2009 at 11:40 AM, Dave Korn  wrote:
> Benjamin Kosnik wrote:
>> From older mail thread:
>
>  Likewise.  I had to take some time away from this to do other stuff, but
> I've gotten back to it this week.
>
>> No. There is already visibility markup on libstdc++ headers for this.
>> You can just put the attribute visibility bits on namespace std, and be
>> done with it.
>>
>> Here, from include/bits/c++config.h:
>>
>> # define _GLIBCXX_BEGIN_NAMESPACE(X) namespace X
>> _GLIBCXX_VISIBILITY_ATTR(default)
>
<snip>
>
>        * libstdc++-v3/include/bits/c++config (_GLIBCXX_VISIBILITY_ATTR): On
>        platforms that don't support visibility, allow them to declare a macro
>        _GLIBCXX_PSEUDO_VISIBILITY that is applied in place of visibility.
>

I like the idea but the name _GLIBCXX_PSEUDO_VISIBILITY is a bit
strange to me (and probably others with psuedo-dyslexia).
Why not _GLIBCXX_TARGET_VISIBILITY_ATTR or (more generically)
_GLIBCXX_TARGET_ACCESSIBILITY_ATTR that defaults to
_GLIBCXX_VISIBILITY_ATTR.

Danny

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

* Re: Libstdc++ as DLL on windows, alternative approach [was Re: cygwin  patch review]
  2009-11-16  3:34   ` Danny Smith
@ 2009-11-16  3:37     ` Dave Korn
  0 siblings, 0 replies; 10+ messages in thread
From: Dave Korn @ 2009-11-16  3:37 UTC (permalink / raw)
  To: Danny Smith
  Cc: Dave Korn, Benjamin Kosnik, libstdc++,
	GCC Patches, Aaron W. LaFramboise (GCC),
	Kai Tietz

Danny Smith wrote:
> On Fri, Nov 13, 2009 at 11:40 AM, Dave Korn  wrote:
>> Benjamin Kosnik wrote:
>>> From older mail thread:
>>  Likewise.  I had to take some time away from this to do other stuff, but
>> I've gotten back to it this week.
>>
>>> No. There is already visibility markup on libstdc++ headers for this.
>>> You can just put the attribute visibility bits on namespace std, and be
>>> done with it.
>>>
>>> Here, from include/bits/c++config.h:
>>>
>>> # define _GLIBCXX_BEGIN_NAMESPACE(X) namespace X
>>> _GLIBCXX_VISIBILITY_ATTR(default)
> <snip>
>>        * libstdc++-v3/include/bits/c++config (_GLIBCXX_VISIBILITY_ATTR): On
>>        platforms that don't support visibility, allow them to declare a macro
>>        _GLIBCXX_PSEUDO_VISIBILITY that is applied in place of visibility.
>>
> 
> I like the idea but the name _GLIBCXX_PSEUDO_VISIBILITY is a bit
> strange to me (and probably others with psuedo-dyslexia).
> Why not _GLIBCXX_TARGET_VISIBILITY_ATTR or (more generically)
> _GLIBCXX_TARGET_ACCESSIBILITY_ATTR that defaults to
> _GLIBCXX_VISIBILITY_ATTR.

  Well, the rationale is "because it's not visibility, but it's something that
stands in its place".  So, I chose "pseudo"-visibility because it's not
visibility but it's like visibility.

  I don't mind renaming it, but not to anything like TARGET_VISIBILITY please,
because it just isn't visibility.  It's not really even accessibility either,
because after all those functions are just as 'accessible' at link time
regardless of whether they were marked up as _dllimport or not at compiletime,
it only changes whether they'll need to be auto-imported or not.  What it
really is is "An arbitrary tag of any kind at all, that can be applied to the
namespaces in the C++ library, differently according to whether or not those
namespaces are intended to be visible or not, but it doesn't actually
necessarily have to do visibility stuff to those namespaces in practice,
particularly on platforms that don't support visibility".

  Got a third suggestion?


    cheers,
      DaveK

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

* Re: Libstdc++ as DLL on windows, alternative approach [was Re: cygwin  patch review]
  2009-11-12 22:33 ` Libstdc++ as DLL on windows, alternative approach [was Re: cygwin patch review] Dave Korn
  2009-11-16  2:57   ` Dave Korn
  2009-11-16  3:34   ` Danny Smith
@ 2009-11-19  1:25   ` Dave Korn
  2009-11-19 17:39   ` Benjamin Kosnik
  3 siblings, 0 replies; 10+ messages in thread
From: Dave Korn @ 2009-11-19  1:25 UTC (permalink / raw)
  To: Dave Korn
  Cc: Benjamin Kosnik, libstdc++,
	GCC Patches, Danny Smith, Aaron W. LaFramboise (GCC),
	Kai Tietz

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

Dave Korn wrote:
> Benjamin Kosnik wrote:
>> From older mail thread:

>> No. There is already visibility markup on libstdc++ headers for this.
>> You can just put the attribute visibility bits on namespace std, and be
>> done with it.
>>
>> Here, from include/bits/c++config.h:
>>
>> # define _GLIBCXX_BEGIN_NAMESPACE(X) namespace X
>> _GLIBCXX_VISIBILITY_ATTR(default)
> 
>   Ok, so as we discussed before, this doesn't currently work.  However it does
> sound like the right thing to do, so I've spun a new version of the patch that
> applies dllimport to the namespace.  We could commit this and then open a PR
> about dllimport not working on namespaces.  It's much smaller and cleaner than
> the version that adds markup everywhere, but I doubt I'll be able to fix the
> dllimport-vs-namespace bug in time for 4.5.  That's not the end of the world;
> getting dllimport right is a good optimisation, but we can live with the
> compromise of auto-import for now.


  I've done a little bit of scoping out what it would take to enable
dllimport/dllexport attributes on namespaces, and I think it looks roughly
something like the attached crude (contains debug printf and commented-out
bits) WIP patch.

  There are some small tweaks to the C++ parser and core attribute handler to
allow the attributes to be attached to namespace_decl nodes (probably more
than needed in fact, I think rather than passing in a tree* pointer to
handle_namespace_attrs in cp/name-lookup.c I just want to gcc_assert that
cplus_decl_attributes doesn't return a changed pointer; it shouldn't anyway,
it's only meant to do so for types, not decls), and then in the backend we
process class definitions and other decls using the adjust_class_at_definition
and encode_section_info flags, walking our way up the namespace/class context
hierarchy and looking for the attributes.

  I'm going to put this through bootstrap and test, and then do some more
manual testing.  I'm not sure whether it handles everything correctly w.r.t.
templates and odd corner cases of nested contexts and would be glad of any
guidance anyone can offer about the algorithm for walking up out of the nested
scopes looking for dll{im,ex}port attributes.

  Another open question is whether for the sake of this usage (tagging dll
attributes on namespaces) it wouldn't make sense to add nodllimport and
nodllexport tags, which would serve to block off portions of the hierarchy
(i.e. if the scope tree climb ascends past a node tagged with one of these
first, it stops early).  This would go in the places where ELF platforms use
hidden visibility attributes.

  Anyway, FYI and FTR, this is what I'm trying at the moment.  Doesn't look
like there's anything we could do solely in the backend to try and make it
suitable for stage 3, which I had kinda hoped, but the attributes just get
discarded in the parser without the core compiler mods; shame.

    cheers,
      DaveK



[-- Attachment #2: dllimport-namespace-wip.diff --]
[-- Type: text/x-c, Size: 11729 bytes --]

Index: gcc/tree.c
===================================================================
--- gcc/tree.c	(revision 154010)
+++ gcc/tree.c	(working copy)
@@ -5307,7 +5307,8 @@ handle_dll_attribute (tree * pnode, tree name, tre
 
   if (TREE_CODE (node) != FUNCTION_DECL
       && TREE_CODE (node) != VAR_DECL
-      && TREE_CODE (node) != TYPE_DECL)
+      && TREE_CODE (node) != TYPE_DECL
+      && TREE_CODE (node) != NAMESPACE_DECL)
     {
       *no_add_attrs = true;
       warning (OPT_Wattributes, "%qE attribute ignored",
Index: gcc/cp/parser.c
===================================================================
--- gcc/cp/parser.c	(revision 154010)
+++ gcc/cp/parser.c	(working copy)
@@ -12854,7 +12854,7 @@ cp_parser_namespace_definition (cp_parser* parser)
       push_namespace (identifier);
     }
 
-  has_visibility = handle_namespace_attrs (current_namespace, attribs);
+  has_visibility = handle_namespace_attrs (&current_namespace, attribs);
 
   /* Parse the body of the namespace.  */
   cp_parser_namespace_body (parser);
Index: gcc/cp/name-lookup.c
===================================================================
--- gcc/cp/name-lookup.c	(revision 154010)
+++ gcc/cp/name-lookup.c	(working copy)
@@ -3192,7 +3192,7 @@ current_decl_namespace (void)
    NAMESPACE_DECL at all.  Returns true if attribute visibility is seen.  */
 
 bool
-handle_namespace_attrs (tree ns, tree attributes)
+handle_namespace_attrs (tree *ns, tree attributes)
 {
   tree d;
   bool saw_vis = false;
@@ -3214,7 +3214,7 @@ bool
 	      continue;
 	    }
 
-	  if (!TREE_PUBLIC (ns))
+	  if (!TREE_PUBLIC (*ns))
 	    warning (OPT_Wattributes,
 		     "%qD attribute is meaningless since members of the "
 		     "anonymous namespace get local symbols", name);
@@ -3224,7 +3224,12 @@ bool
 	}
       else
 #endif
+      if (is_attribute_p ("dllimport", name) || is_attribute_p ("dllexport", name))
 	{
+	  cplus_decl_attributes (ns, attributes, 0);
+	}
+      else
+	{
 	  warning (OPT_Wattributes, "%qD attribute directive ignored",
 		   name);
 	  continue;
Index: gcc/cp/name-lookup.h
===================================================================
--- gcc/cp/name-lookup.h	(revision 154010)
+++ gcc/cp/name-lookup.h	(working copy)
@@ -304,7 +304,7 @@ extern void push_namespace (tree);
 extern void pop_namespace (void);
 extern void push_nested_namespace (tree);
 extern void pop_nested_namespace (tree);
-extern bool handle_namespace_attrs (tree, tree);
+extern bool handle_namespace_attrs (tree *, tree);
 extern void pushlevel_class (void);
 extern void poplevel_class (void);
 extern tree pushdecl_with_scope (tree, cxx_scope *, bool);
Index: gcc/config/i386/winnt-stubs.c
===================================================================
--- gcc/config/i386/winnt-stubs.c	(revision 154010)
+++ gcc/config/i386/winnt-stubs.c	(working copy)
@@ -46,6 +46,11 @@ i386_pe_type_dllexport_p (tree decl ATTRIBUTE_UNUS
   return false;
 }
 
+bool
+inherit_dllattribute_from_namespace (const char *attrname ATTRIBUTE_UNUSED, tree context ATTRIBUTE_UNUSED)
+{
+  return false;
+}
 
 void
 i386_pe_adjust_class_at_definition (tree t ATTRIBUTE_UNUSED)
Index: gcc/config/i386/winnt.c
===================================================================
--- gcc/config/i386/winnt.c	(revision 154010)
+++ gcc/config/i386/winnt.c	(working copy)
@@ -112,7 +112,7 @@ i386_pe_determine_dllexport_p (tree decl)
   if (lookup_attribute ("dllexport", DECL_ATTRIBUTES (decl)))
     return true;
 
-  return false;
+  return inherit_dllattribute_from_namespace ("dllexport", /*DECL_CONTEXT*/ (decl));
 }
 
 /* Return true if DECL should be a dllimport'd object.  */
@@ -143,7 +143,7 @@ i386_pe_determine_dllimport_p (tree decl)
 	error ("definition of static data member %q+D of "
 	       "dllimport'd class", decl);
 
-  return false;
+  return inherit_dllattribute_from_namespace ("dllimport", /*DECL_CONTEXT*/ (decl));
 }
 
 /* Handle the -mno-fun-dllimport target switch.  */
@@ -603,6 +603,64 @@ i386_pe_maybe_record_exported_symbol (tree decl, c
   export_head = p;
 }
 
+#ifdef CXX_WRAP_SPEC_LIST
+
+/*  Hash table equality helper function.  */
+
+static int
+wrapper_strcmp (const void *x, const void *y)
+{
+  return !strcmp ((const char *) x, (const char *) y);
+}
+
+/* Search for a function named TARGET in the list of library wrappers
+   we are using, returning a pointer to it if found or NULL if not.
+   This function might be called on quite a few symbols, and we only
+   have the list of names of wrapped functions available to us as a
+   spec string, so first time round we lazily initialise a hash table
+   to make things quicker.  */
+
+static const char *
+i386_find_on_wrapper_list (const char *target)
+{
+  static char first_time = 1;
+  static htab_t wrappers;
+
+  if (first_time)
+    {
+      /* Beware that this is not a complicated parser, it assumes
+         that any sequence of non-whitespace beginning with an
+	 underscore is one of the wrapped symbols.  For now that's
+	 adequate to distinguish symbols from spec substitutions
+	 and command-line options.  */
+      static char wrapper_list_buffer[] = CXX_WRAP_SPEC_LIST;
+      char *bufptr;
+      /* Breaks up the char array into separated strings
+         strings and enter them into the hash table.  */
+      wrappers = htab_create_alloc (8, htab_hash_string, wrapper_strcmp,
+	0, xcalloc, free);
+      for (bufptr = wrapper_list_buffer; *bufptr; ++bufptr)
+	{
+	  char *found = NULL;
+	  if (ISSPACE (*bufptr))
+	    continue;
+	  if (*bufptr == '_')
+	    found = bufptr;
+	  while (*bufptr && !ISSPACE (*bufptr))
+	    ++bufptr;
+	  if (*bufptr)
+	    *bufptr = 0;
+	  if (found)
+	    *htab_find_slot (wrappers, found, INSERT) = found;
+	}
+      first_time = 0;
+    }
+
+  return (const char *) htab_find (wrappers, target);
+}
+
+#endif /* CXX_WRAP_SPEC_LIST */
+
 /* This is called at the end of assembly.  For each external function
    which has not been defined, we output a declaration now.  We also
    output the .drectve section.  */
@@ -624,6 +682,15 @@ i386_pe_file_end (void)
       if (! TREE_ASM_WRITTEN (decl)
 	  && TREE_SYMBOL_REFERENCED (DECL_ASSEMBLER_NAME (decl)))
 	{
+#ifdef CXX_WRAP_SPEC_LIST
+	  /* To ensure the DLL that provides the corresponding real
+	     functions is still loaded at runtime, we must reference
+	     the real function so that an (unused) import is created.  */
+	  const char *realsym = i386_find_on_wrapper_list (p->name);
+	  if (realsym)
+	    i386_pe_declare_function_type (asm_out_file,
+		concat ("__real_", realsym, NULL), TREE_PUBLIC (decl));
+#endif /* CXX_WRAP_SPEC_LIST */
 	  TREE_ASM_WRITTEN (decl) = 1;
 	  i386_pe_declare_function_type (asm_out_file, p->name,
 					 TREE_PUBLIC (decl));
Index: gcc/config/i386/winnt-cxx.c
===================================================================
--- gcc/config/i386/winnt-cxx.c	(revision 154010)
+++ gcc/config/i386/winnt-cxx.c	(working copy)
@@ -91,15 +91,115 @@ static inline void maybe_add_dllexport (tree decl)
     }
 }
 
+static bool
+inherit_dllattribute_from_namespace_1 (const char *attrname, tree context)
+{
+  if (context == NULL_TREE || context == global_namespace)
+    return false;
+
+  /*fflush (0); fprintf (stderr, "inherit1: %s\n", attrname); fflush (0);
+  debug_tree (context);*/
+
+  if (CLASS_TYPE_P (context))
+    {
+      return lookup_attribute (attrname, TYPE_ATTRIBUTES (context)) != NULL_TREE
+	|| inherit_dllattribute_from_namespace_1 (attrname, CP_TYPE_CONTEXT (context));
+    }
+  else if (TREE_CODE (context) == NAMESPACE_DECL)
+    {
+      return lookup_attribute (attrname, DECL_ATTRIBUTES (context)) != NULL_TREE
+	|| (inherit_dllattribute_from_namespace_1 (attrname, CP_DECL_CONTEXT (context)));
+    }
+  else if (TREE_CODE (context) == FUNCTION_DECL)
+    {
+      /* Types declared in a function don't get exported just because
+         the function itself is, so we don't call lookup_attribute.  */
+      /* If the context is a nested function, don't escape.  */
+      return (TREE_CODE (CP_DECL_CONTEXT (context)) != FUNCTION_DECL
+	    && inherit_dllattribute_from_namespace_1 (attrname, CP_DECL_CONTEXT (context)));
+    }
+
+  fflush (0); fprintf (stderr, "unknown tree code\n"); fflush (0);
+  debug_tree (context);
+
+  return false;
+}
+
+/* The only valid inputs T to this function are VAR_DECL and FUNCTION_DECL
+   nodes passed in from i386_pe_determine_dll{im,ex}port_p, and class type
+   nodes passed in from i386_pe_adjust_class_at_definition.  */
+bool
+inherit_dllattribute_from_namespace (const char *attrname, tree t)
+{
+  tree tinfo = NULL_TREE;
+  tree context = NULL_TREE;
+
+  /*fflush (0); fprintf (stderr, "inherit: %s\n", attrname); fflush (0);
+  debug_tree (t);*/
+
+  if (t == NULL_TREE)
+    return false;
+
+  if (CLASS_TYPE_P (t))
+    context = CP_TYPE_CONTEXT (t);
+  else if (TREE_CODE (t) == VAR_DECL || TREE_CODE (t) == FUNCTION_DECL)
+    context = CP_DECL_CONTEXT (t);
+
+  if (inherit_dllattribute_from_namespace_1 (attrname, context))
+    return true;
+
+  /*fflush (0); fprintf (stderr, "inherit2: %s\n", attrname); fflush (0);
+  debug_tree (context);*/
+
+  /*if (context == NULL_TREE || context == global_namespace)
+    return false;*/
+
+  if ((TREE_CODE (t) == VAR_DECL || TREE_CODE (t) == FUNCTION_DECL)
+	&& DECL_LANG_SPECIFIC (t)
+	&& DECL_USE_TEMPLATE (t)
+	&& DECL_TEMPLATE_INFO (t))
+    {
+      tinfo = DECL_TEMPLATE_INFO (t);
+    }
+  else if (CLASS_TYPE_P (t)
+	&& TYPE_LANG_SPECIFIC (t)
+	&& CLASSTYPE_USE_TEMPLATE (t)
+	&& CLASSTYPE_TEMPLATE_INFO (t))
+    {
+      tinfo = CLASSTYPE_TEMPLATE_INFO (t);
+    }
+
+  if (tinfo)
+    {
+      tree ti_template = TI_TEMPLATE (tinfo);
+  /*fflush (0); fprintf (stderr, "inherit3: %s\n", attrname); fflush (0);
+  debug_tree (ti_template);*/
+      gcc_assert (TREE_CODE (ti_template) == TEMPLATE_DECL);
+      return inherit_dllattribute_from_namespace_1 (attrname, CP_DECL_CONTEXT (ti_template));
+    }
+
+  return false;
+}
+
 void
 i386_pe_adjust_class_at_definition (tree t)
 {
   tree member;
+  bool is_import, is_export;
 
   gcc_assert (CLASS_TYPE_P (t));
  
+  is_export = lookup_attribute ("dllexport", TYPE_ATTRIBUTES (t)) != NULL_TREE;
+  if (!is_export)
+    is_export = inherit_dllattribute_from_namespace ("dllexport", /*CP_TYPE_CONTEXT*/ (t));
+  if (!is_export)
+    {
+      is_import = lookup_attribute ("dllimport", TYPE_ATTRIBUTES (t)) != NULL_TREE;
+      if (!is_export && !is_import)
+	is_import = inherit_dllattribute_from_namespace ("dllimport", /*CP_TYPE_CONTEXT*/ (t));
+    }
  
-  if (lookup_attribute ("dllexport", TYPE_ATTRIBUTES (t)) != NULL_TREE)
+  if (is_export)
     {
       /* Check static VAR_DECL's.  */
       for (member = TYPE_FIELDS (t); member; member = TREE_CHAIN (member))
@@ -124,7 +224,7 @@ i386_pe_adjust_class_at_definition (tree t)
 	  maybe_add_dllexport (member);
     }
 
-  else if (lookup_attribute ("dllimport", TYPE_ATTRIBUTES (t)) != NULL_TREE)
+  else if (is_import)
     {
       /* We don't actually add the attribute to the decl, just set the flag
 	 that signals that the address of this symbol is not a compile-time
Index: gcc/config/i386/i386-protos.h
===================================================================
--- gcc/config/i386/i386-protos.h	(revision 154010)
+++ gcc/config/i386/i386-protos.h	(working copy)
@@ -240,6 +240,7 @@ extern void i386_pe_file_end (void);
 extern tree i386_pe_mangle_decl_assembler_name (tree, tree);
 
 /* In winnt-cxx.c and winnt-stubs.c  */
+extern bool inherit_dllattribute_from_namespace (const char *, tree);
 extern void i386_pe_adjust_class_at_definition (tree);
 extern bool i386_pe_type_dllimport_p (tree);
 extern bool i386_pe_type_dllexport_p (tree);

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

* Re: Libstdc++ as DLL on windows, alternative approach [was Re:  cygwin patch review]
  2009-11-12 22:33 ` Libstdc++ as DLL on windows, alternative approach [was Re: cygwin patch review] Dave Korn
                     ` (2 preceding siblings ...)
  2009-11-19  1:25   ` Dave Korn
@ 2009-11-19 17:39   ` Benjamin Kosnik
  2009-11-20 16:56     ` Dave Korn
  3 siblings, 1 reply; 10+ messages in thread
From: Benjamin Kosnik @ 2009-11-19 17:39 UTC (permalink / raw)
  To: Dave Korn
  Cc: libstdc++, GCC Patches, Danny Smith, Aaron W. LaFramboise (GCC),
	Kai Tietz, Dave Korn


>   Ok, so as we discussed before, this doesn't currently work.
> However it does sound like the right thing to do, so I've spun a new
> version of the patch that applies dllimport to the namespace.  We
> could commit this and then open a PR about dllimport not working on
> namespaces.  It's much smaller and cleaner than the version that adds
> markup everywhere, but I doubt I'll be able to fix the
> dllimport-vs-namespace bug in time for 4.5.  That's not the end of
> the world; getting dllimport right is a good optimisation, but we can
> live with the compromise of auto-import for now.

I like this plan. What's the PR number? And can you affix your
patch from here to it:
http://gcc.gnu.org/ml/gcc-patches/2009-11/msg00979.html

Thanks. 
 
>   Initial test results (c and c++ only) are posted(*) and look right,
> but I'll do a clean run on the same source revision for comparison
> anyway.  Note that I had to patch various testsuite/lib/ files to
> avoid the current utf-8 problem(**) that would otherwise have
> provoked many spurious 'excess errors' warnings.  That'll be the
> subject of a separate patch, I'll do the clean run using them as well
> so the comparison is fair.

It looks like (in later messages) you have an updated patch. But your
updated results here looks great:
http://gcc.gnu.org/ml/gcc-testresults/2009-11/msg01084.html

> libstdc++-v3/ChangeLog:
> 
> 	* libstdc++-v3/acinclude.m4 (GLIBCXX_ENABLE_SYMVERS): Don't
> disable on PE targets.
> 	* libstdc++-v3/configure: Regenerate.
> 	* libstdc++-v3/configure.host: Add libtool DLL options for
> Cygwin and MinGW platforms.
> 
> 	* libstdc++-v3/include/bits/c++config
> (_GLIBCXX_VISIBILITY_ATTR): On platforms that don't support
> visibility, allow them to declare a macro _GLIBCXX_PSEUDO_VISIBILITY
> that is applied in place of visibility.
> 
> 	* libstdc++-v3/config/os/newlib/os_defines.h
> 	(_GLIBCXX_PSEUDO_VISIBILITY_default): New macro for dllimport.
> 	(_GLIBCXX_PSEUDO_VISIBILITY_hidden): New empty macro.
> 	(_GLIBCXX_PSEUDO_VISIBILITY): Evaluate to one of the above.
> 	* libstdc++-v3/config/os/mingw32/os_defines.h
> 	(_GLIBCXX_PSEUDO_VISIBILITY_default,
> 	_GLIBCXX_PSEUDO_VISIBILITY_hidden,
> 	_GLIBCXX_PSEUDO_VISIBILITY): Likewise.
> 
>   So, is this any more like OK for head?

Yes, exactly. Thanks. This is OK with me.

best,
benjamin

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

* Re: Libstdc++ as DLL on windows, alternative approach [was Re: cygwin  patch review]
  2009-11-19 17:39   ` Benjamin Kosnik
@ 2009-11-20 16:56     ` Dave Korn
  2009-11-21  6:34       ` Danny Smith
                         ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Dave Korn @ 2009-11-20 16:56 UTC (permalink / raw)
  To: Benjamin Kosnik
  Cc: Dave Korn, libstdc++,
	GCC Patches, Danny Smith, Aaron W. LaFramboise (GCC),
	Kai Tietz

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

Benjamin Kosnik wrote:

>> However it does sound like the right thing to do, so I've spun a new
>> version of the patch that applies dllimport to the namespace.  We
>> could commit this and then open a PR about dllimport not working on
>> namespaces.  

> I like this plan. What's the PR number? And can you affix your
> patch from here to it:
> http://gcc.gnu.org/ml/gcc-patches/2009-11/msg00979.html

  This is now PR 42123.  (The WIP patch is on the right lines but not quite
correct yet, details in the PR.)

> Thanks. 
>  
>>   Initial test results (c and c++ only) are posted(*) and look right,
>> but I'll do a clean run on the same source revision for comparison
>> anyway.  Note that I had to patch various testsuite/lib/ files to
>> avoid the current utf-8 problem(**) that would otherwise have
>> provoked many spurious 'excess errors' warnings.  That'll be the
>> subject of a separate patch, I'll do the clean run using them as well
>> so the comparison is fair.
> 
> It looks like (in later messages) you have an updated patch. 

  I've since made one small change (apart from experimenting with the
namespace patch).  In testing the namespace patch, I discovered an order of
include problem, where this code from include/bits/c++config:

>  #if _GLIBCXX_HAVE_ATTRIBUTE_VISIBILITY
>  # define _GLIBCXX_VISIBILITY_ATTR(V) __attribute__ ((__visibility__ (#V)))
> +#elif defined (_GLIBCXX_PSEUDO_VISIBILITY)
> +# define _GLIBCXX_VISIBILITY_ATTR(V) _GLIBCXX_PSEUDO_VISIBILITY(V)
>  #else
>  # define _GLIBCXX_VISIBILITY_ATTR(V)
>  #endif

is before any of the os/cpu-specific headers get #included, so it didn't work.
 I've rearranged it now so that this site at the top of the file just says

> #if _GLIBCXX_HAVE_ATTRIBUTE_VISIBILITY
> # define _GLIBCXX_VISIBILITY_ATTR(V) __attribute__ ((__visibility__ (#V)))
> #else
> // If this is not supplied by the OS-specific or CPU-specific
> // headers included below, it will be defined to an empty default.
> # define _GLIBCXX_VISIBILITY_ATTR(V) _GLIBCXX_PSEUDO_VISIBILITY(V)
> #endif

.. and then after the #includes I added

> // If platform uses neither visibility nor psuedo-visibility,
> // specify empty default for namespace annotation macros.
> #ifndef _GLIBCXX_PSEUDO_VISIBILITY
> #define _GLIBCXX_PSEUDO_VISIBILITY(V)
> #endif

  I've verified this change DTRT by diffing pre-processed sources compiled
with and without -D_GLIBCXX_DLL.

> But your
> updated results here looks great:
> http://gcc.gnu.org/ml/gcc-testresults/2009-11/msg01084.html

  Yep, the results are now consistently the same as for static libstdc++.

>>   So, is this any more like OK for head?
> 
> Yes, exactly. Thanks. This is OK with me.

  :)  Fantastic.  Three small things which need explicit confirmation:

- can your OK cover the c++-related change to LINK_SPEC in gcc/gcc.c or do I
need to get another maintainer (probably jsm) for that?
- is it OK even with the change to the c++config hunk mentioned above?
- is it OK even in stage 3?

  Also, PING! Danny/Kai, the windows side of this patch has changed a bit
since you guys OK'd it some time back, can one of you take a look over the new
changes to config/i386/winnt.c w.r.t. i386_find_on_wrapper_list() and #ifdef
CXX_WRAP_SPEC_LIST?

  I've just updated my sandbox to head.  Attached, the final version of the
patch for reference; I'm just rebuilding it now to check nothing has broken.
The ChangeLog is almost the same as last time, but mentions the c++config change:

gcc/ChangeLog:

	* configure.ac (USE_CYGWIN_LIBSTDCXX_WRAPPERS): Define to reflect
	status of AC_CHECK_FUNC for Cygwin DLL libstdc++ support wrappers.
	* configure: Regenerate.
	* config.in: Regenerate.

	* config/i386/cygwin.h (CXX_WRAP_SPEC_LIST): Define list of --wrap
	options for Cygwin DLL libstdc++ support wrappers.
	(CXX_WRAP_SPEC_OPT): Define spec to use wrappers or not by default
	according to defined value of USE_CYGWIN_LIBSTDCXX_WRAPPERS.
	(CXX_WRAP_SPEC): Define entire wrapper spec in or out according to
	whether USE_CYGWIN_LIBSTDCXX_WRAPPERS is even defined or not.
	(LINK_SPEC): Include CXX_WRAP_SPEC.
	* gcc/config/i386/winnt.c (wrapper_strcmp): New qsort helper function.
	(i386_find_on_wrapper_list): Check if a function is found on the list
	of libstdc++ wrapper options.
	(i386_pe_file_end): If we are importing a wrapped function, also emit
	an external declaration for the real version.
	* config/i386/cygming.opt (muse-libstdc-wrappers): New option for
	Cygwin targets. Update copyright year.

	* gcc.c (LINK_COMMAND_SPEC): Allow and ignore -static-libstdc++
	similarly to -static.

gcc/cp/ChangeLog:

	* g++spec.c (SKIPOPT): Delete.
	(lang_specific_driver): Do not skip -static-libstdc++ option.

libstdc++-v3/ChangeLog:

	* libstdc++-v3/acinclude.m4 (GLIBCXX_ENABLE_SYMVERS): Don't disable
	on PE targets.
	* libstdc++-v3/configure: Regenerate.
	* libstdc++-v3/configure.host: Add libtool DLL options for Cygwin
	and MinGW platforms.

	* libstdc++-v3/include/bits/c++config (_GLIBCXX_VISIBILITY_ATTR): On
	platforms that don't support visibility, allow them to declare a macro
	_GLIBCXX_PSEUDO_VISIBILITY that is applied in place of visibility.
	(_GLIBCXX_PSEUDO_VISIBILITY): Supply empty default if not declared by
	CPU- or OS-specific headers.

	* libstdc++-v3/config/os/newlib/os_defines.h
	(_GLIBCXX_PSEUDO_VISIBILITY_default): New macro for dllimport.
	(_GLIBCXX_PSEUDO_VISIBILITY_hidden): New empty macro.
	(_GLIBCXX_PSEUDO_VISIBILITY): Evaluate to one of the above.
	* libstdc++-v3/config/os/mingw32/os_defines.h
	(_GLIBCXX_PSEUDO_VISIBILITY_default,
	_GLIBCXX_PSEUDO_VISIBILITY_hidden,
	_GLIBCXX_PSEUDO_VISIBILITY): Likewise.

    cheers,
      DaveK


[-- Attachment #2: libstdc-dll-vis-final.diff --]
[-- Type: text/x-c, Size: 12797 bytes --]

Index: gcc/gcc.c
===================================================================
--- gcc/gcc.c	(revision 154370)
+++ gcc/gcc.c	(working copy)
@@ -766,7 +766,10 @@ proper position among the other output files.  */
 
 /* -u* was put back because both BSD and SysV seem to support it.  */
 /* %{static:} simply prevents an error message if the target machine
-   doesn't handle -static.  */
+   doesn't handle -static; %{static-libstdc++} is a C++ FE option, 
+   handled entirely in cp/g++spec.c, yet we cannot just remove it
+   from the command-line there or it cannot be tested in target spec
+   processing, so we simply ignore it here similarly.  */
 /* We want %{T*} after %{L*} and %D so that it can be used to specify linker
    scripts which exist in user specified directories, or in standard
    directories.  */
@@ -789,7 +792,7 @@ proper position among the other output files.  */
     %{flto} %{fwhopr} %l " LINK_PIE_SPEC \
    "%X %{o*} %{A} %{d} %{e*} %{m} %{N} %{n} %{r}\
     %{s} %{t} %{u*} %{x} %{z} %{Z} %{!A:%{!nostdlib:%{!nostartfiles:%S}}}\
-    %{static:} %{L*} %(mfwrap) %(link_libgcc) %o\
+    %{static:} %{static-libstdc++:} %{L*} %(mfwrap) %(link_libgcc) %o\
     %{fopenmp|ftree-parallelize-loops=*:%:include(libgomp.spec)%(link_gomp)} %(mflib)\
     %{fprofile-arcs|fprofile-generate*|coverage:-lgcov}\
     %{!nostdlib:%{!nodefaultlibs:%(link_ssp) %(link_gcc_c_sequence)}}\
Index: gcc/cp/g++spec.c
===================================================================
--- gcc/cp/g++spec.c	(revision 154370)
+++ gcc/cp/g++spec.c	(working copy)
@@ -30,8 +30,6 @@ along with GCC; see the file COPYING3.  If not see
 #define MATHLIB		(1<<2)
 /* This bit is set if they did `-lc'.  */
 #define WITHLIBC	(1<<3)
-/* Skip this option.  */
-#define SKIPOPT		(1<<4)
 
 #ifndef MATH_LIBRARY
 #define MATH_LIBRARY "-lm"
@@ -211,10 +209,7 @@ lang_specific_driver (int *in_argc, const char *co
 	  else if (strcmp (argv[i], "-static-libgcc") == 0)
 	    shared_libgcc = 0;
 	  else if (strcmp (argv[i], "-static-libstdc++") == 0)
-	    {
 	      library = library >= 0 ? 2 : library;
-	      args[i] |= SKIPOPT;
-	    }
 	  else if (DEFAULT_WORD_SWITCH_TAKES_ARG (&argv[i][1]))
 	    i++;
 	  else
@@ -324,9 +319,6 @@ lang_specific_driver (int *in_argc, const char *co
 	  arglist[j] = "-xnone";
 	}
 
-      if ((args[i] & SKIPOPT) != 0)
-	--j;
-
       i++;
       j++;
     }
Index: gcc/configure.ac
===================================================================
--- gcc/configure.ac	(revision 154370)
+++ gcc/configure.ac	(working copy)
@@ -3000,6 +3000,25 @@ changequote(,)dnl
   i[34567]86-*-* | x86_64-*-*)
 changequote([,])dnl
     case $target_os in
+      cygwin*)
+	# Full C++ conformance when using a shared libstdc++-v3 requires some
+	# support from the Cygwin DLL, which in more recent versions exports
+	# wrappers to aid in interposing and redirecting operators new, delete,
+	# etc., as per n2800 #17.6.4.6 [replacement.functions].  Check if we
+	# are configuring for a version of Cygwin that exports the wrappers.
+	if test x$host = x$target; then
+	  AC_CHECK_FUNC([__wrap__Znaj],[gcc_ac_cygwin_dll_wrappers=yes],[gcc_ac_cygwin_dll_wrappers=no])
+	else
+	  # Can't check presence of libc functions during cross-compile, so
+	  # we just have to assume we're building for an up-to-date target.
+	  gcc_ac_cygwin_dll_wrappers=yes
+	fi
+	AC_DEFINE_UNQUOTED(USE_CYGWIN_LIBSTDCXX_WRAPPERS,
+	  [`if test $gcc_ac_cygwin_dll_wrappers = yes; then echo 1; else echo 0; fi`],
+	  [Define if you want to generate code by default that assumes that the
+	   Cygwin DLL exports wrappers to support libstdc++ function replacement.])
+    esac
+    case $target_os in
       cygwin* | pe | mingw32*)
 	# Recent binutils allows the three-operand form of ".comm" on PE.  This
 	# definition is used unconditionally to initialise the default state of
Index: gcc/config/i386/cygwin.h
===================================================================
--- gcc/config/i386/cygwin.h	(revision 154370)
+++ gcc/config/i386/cygwin.h	(working copy)
@@ -85,9 +85,41 @@ along with GCC; see the file COPYING3.  If not see
   %{mwindows:-lgdi32 -lcomdlg32} \
   -luser32 -lkernel32 -ladvapi32 -lshell32"
 
+/* To implement C++ function replacement we always wrap the cxx
+   malloc-like operators.  See N2800 #17.6.4.6 [replacement.functions] */
+#define CXX_WRAP_SPEC_LIST "%{!static: %{!static-libstdc++: \
+  --wrap _Znwj \
+  --wrap _Znaj \
+  --wrap _ZdlPv \
+  --wrap _ZdaPv \
+  --wrap _ZnwjRKSt9nothrow_t \
+  --wrap _ZnajRKSt9nothrow_t \
+  --wrap _ZdlPvRKSt9nothrow_t \
+  --wrap _ZdaPvRKSt9nothrow_t \
+  }}"
+
+#if defined (USE_CYGWIN_LIBSTDCXX_WRAPPERS)
+
+#if USE_CYGWIN_LIBSTDCXX_WRAPPERS
+/* Default on, only explict -mno disables.  */
+#define CXX_WRAP_SPEC_OPT "!mno-use-libstdc-wrappers"
+#else
+/* Default off, only explict -m enables.  */
+#define CXX_WRAP_SPEC_OPT "muse-libstdc-wrappers"
+#endif
+
+#define CXX_WRAP_SPEC "%{" CXX_WRAP_SPEC_OPT ":" CXX_WRAP_SPEC_LIST "}"
+
+#else /* !defined (USE_CYGWIN_LIBSTDCXX_WRAPPERS)  */
+
+#define CXX_WRAP_SPEC ""
+
+#endif /* ?defined (USE_CYGWIN_LIBSTDCXX_WRAPPERS) */
+
 #define LINK_SPEC "\
   %{mwindows:--subsystem windows} \
   %{mconsole:--subsystem console} \
+  " CXX_WRAP_SPEC " \
   %{shared: %{mdll: %eshared and mdll are not compatible}} \
   %{shared: --shared} %{mdll:--dll} \
   %{static:-Bstatic} %{!static:-Bdynamic} \
Index: gcc/config/i386/winnt.c
===================================================================
--- gcc/config/i386/winnt.c	(revision 154370)
+++ gcc/config/i386/winnt.c	(working copy)
@@ -603,6 +603,64 @@ i386_pe_maybe_record_exported_symbol (tree decl, c
   export_head = p;
 }
 
+#ifdef CXX_WRAP_SPEC_LIST
+
+/*  Hash table equality helper function.  */
+
+static int
+wrapper_strcmp (const void *x, const void *y)
+{
+  return !strcmp ((const char *) x, (const char *) y);
+}
+
+/* Search for a function named TARGET in the list of library wrappers
+   we are using, returning a pointer to it if found or NULL if not.
+   This function might be called on quite a few symbols, and we only
+   have the list of names of wrapped functions available to us as a
+   spec string, so first time round we lazily initialise a hash table
+   to make things quicker.  */
+
+static const char *
+i386_find_on_wrapper_list (const char *target)
+{
+  static char first_time = 1;
+  static htab_t wrappers;
+
+  if (first_time)
+    {
+      /* Beware that this is not a complicated parser, it assumes
+         that any sequence of non-whitespace beginning with an
+	 underscore is one of the wrapped symbols.  For now that's
+	 adequate to distinguish symbols from spec substitutions
+	 and command-line options.  */
+      static char wrapper_list_buffer[] = CXX_WRAP_SPEC_LIST;
+      char *bufptr;
+      /* Breaks up the char array into separated strings
+         strings and enter them into the hash table.  */
+      wrappers = htab_create_alloc (8, htab_hash_string, wrapper_strcmp,
+	0, xcalloc, free);
+      for (bufptr = wrapper_list_buffer; *bufptr; ++bufptr)
+	{
+	  char *found = NULL;
+	  if (ISSPACE (*bufptr))
+	    continue;
+	  if (*bufptr == '_')
+	    found = bufptr;
+	  while (*bufptr && !ISSPACE (*bufptr))
+	    ++bufptr;
+	  if (*bufptr)
+	    *bufptr = 0;
+	  if (found)
+	    *htab_find_slot (wrappers, found, INSERT) = found;
+	}
+      first_time = 0;
+    }
+
+  return (const char *) htab_find (wrappers, target);
+}
+
+#endif /* CXX_WRAP_SPEC_LIST */
+
 /* This is called at the end of assembly.  For each external function
    which has not been defined, we output a declaration now.  We also
    output the .drectve section.  */
@@ -624,6 +682,15 @@ i386_pe_file_end (void)
       if (! TREE_ASM_WRITTEN (decl)
 	  && TREE_SYMBOL_REFERENCED (DECL_ASSEMBLER_NAME (decl)))
 	{
+#ifdef CXX_WRAP_SPEC_LIST
+	  /* To ensure the DLL that provides the corresponding real
+	     functions is still loaded at runtime, we must reference
+	     the real function so that an (unused) import is created.  */
+	  const char *realsym = i386_find_on_wrapper_list (p->name);
+	  if (realsym)
+	    i386_pe_declare_function_type (asm_out_file,
+		concat ("__real_", realsym, NULL), TREE_PUBLIC (decl));
+#endif /* CXX_WRAP_SPEC_LIST */
 	  TREE_ASM_WRITTEN (decl) = 1;
 	  i386_pe_declare_function_type (asm_out_file, p->name,
 					 TREE_PUBLIC (decl));
Index: gcc/config/i386/cygming.opt
===================================================================
--- gcc/config/i386/cygming.opt	(revision 154370)
+++ gcc/config/i386/cygming.opt	(working copy)
@@ -1,6 +1,6 @@
 ; Cygwin- and MinGW-specific options.
 
-; Copyright (C) 2005, 2007 Free Software Foundation, Inc.
+; Copyright (C) 2005, 2007, 2009 Free Software Foundation, Inc.
 ;
 ; This file is part of GCC.
 ;
@@ -49,3 +49,7 @@ Create GUI application
 mpe-aligned-commons
 Target Var(use_pe_aligned_common) Init(HAVE_GAS_ALIGNED_COMM)
 Use the GNU extension to the PE format for aligned common data
+
+muse-libstdc-wrappers
+Target Condition({defined (USE_CYGWIN_LIBSTDCXX_WRAPPERS)})
+Compile code that relies on Cygwin DLL wrappers to support C++ operator new/delete replacement
Index: libstdc++-v3/configure.host
===================================================================
--- libstdc++-v3/configure.host	(revision 154370)
+++ libstdc++-v3/configure.host	(working copy)
@@ -209,6 +209,7 @@ case "${host_os}" in
     ;;
   cygwin*)
     os_include_dir="os/newlib"
+    OPT_LDFLAGS="${OPT_LDFLAGS} -no-undefined -bindir \$(bindir)"
     ;;
   darwin | darwin[1-7] | darwin[1-7].*)
     # On Darwin, performance is improved if libstdc++ is single-module.
@@ -258,6 +259,7 @@ case "${host_os}" in
   mingw32*)
     os_include_dir="os/mingw32"
     error_constants_dir="os/mingw32"
+    OPT_LDFLAGS="${OPT_LDFLAGS} -no-undefined -bindir \$(bindir)"
     ;;
   netbsd*)
     os_include_dir="os/bsd/netbsd"
Index: libstdc++-v3/include/bits/c++config
===================================================================
--- libstdc++-v3/include/bits/c++config	(revision 154370)
+++ libstdc++-v3/include/bits/c++config	(working copy)
@@ -42,7 +42,9 @@
 #if _GLIBCXX_HAVE_ATTRIBUTE_VISIBILITY
 # define _GLIBCXX_VISIBILITY_ATTR(V) __attribute__ ((__visibility__ (#V)))
 #else
-# define _GLIBCXX_VISIBILITY_ATTR(V) 
+// If this is not supplied by the OS-specific or CPU-specific
+// headers included below, it will be defined to an empty default.
+# define _GLIBCXX_VISIBILITY_ATTR(V) _GLIBCXX_PSEUDO_VISIBILITY(V)
 #endif
 
 // Macros for deprecated.
@@ -275,6 +277,12 @@ namespace std
 // Pick up any CPU-specific definitions.
 #include <bits/cpu_defines.h>
 
+// If platform uses neither visibility nor psuedo-visibility,
+// specify empty default for namespace annotation macros.
+#ifndef _GLIBCXX_PSEUDO_VISIBILITY
+#define _GLIBCXX_PSEUDO_VISIBILITY(V)
+#endif
+
 // Allow use of "export template." This is currently not a feature
 // that g++ supports.
 // #define _GLIBCXX_EXPORT_TEMPLATE 1
Index: libstdc++-v3/config/os/newlib/os_defines.h
===================================================================
--- libstdc++-v3/config/os/newlib/os_defines.h	(revision 154370)
+++ libstdc++-v3/config/os/newlib/os_defines.h	(working copy)
@@ -36,6 +36,15 @@
 #ifdef __CYGWIN__
 #define _GLIBCXX_GTHREAD_USE_WEAK 0
 
+#if defined (_GLIBCXX_DLL)
+#define _GLIBCXX_PSEUDO_VISIBILITY_default __attribute__ ((__dllimport__))
+#else
+#define _GLIBCXX_PSEUDO_VISIBILITY_default
+#endif
+#define _GLIBCXX_PSEUDO_VISIBILITY_hidden
+
+#define _GLIBCXX_PSEUDO_VISIBILITY(V) _GLIBCXX_PSEUDO_VISIBILITY_ ## V
+
 // See libstdc++/20806.
 #define _GLIBCXX_HAVE_DOS_BASED_FILESYSTEM 1
 #endif
Index: libstdc++-v3/config/os/mingw32/os_defines.h
===================================================================
--- libstdc++-v3/config/os/mingw32/os_defines.h	(revision 154370)
+++ libstdc++-v3/config/os/mingw32/os_defines.h	(working copy)
@@ -45,6 +45,15 @@
 #undef NOMINMAX
 #define NOMINMAX 1
 
+#if defined (_GLIBCXX_DLL)
+#define _GLIBCXX_PSEUDO_VISIBILITY_default __attribute__ ((__dllimport__))
+#else
+#define _GLIBCXX_PSEUDO_VISIBILITY_default
+#endif
+#define _GLIBCXX_PSEUDO_VISIBILITY_hidden
+
+#define _GLIBCXX_PSEUDO_VISIBILITY(V) _GLIBCXX_PSEUDO_VISIBILITY_ ## V
+
 // See libstdc++/20806.
 #define _GLIBCXX_HAVE_DOS_BASED_FILESYSTEM 1
 
Index: libstdc++-v3/acinclude.m4
===================================================================
--- libstdc++-v3/acinclude.m4	(revision 154370)
+++ libstdc++-v3/acinclude.m4	(working copy)
@@ -2737,7 +2737,7 @@ if test x$enable_symvers = xyes ; then
   else
     if test $with_gnu_ld = yes ; then
       case ${target_os} in
-        cygwin* | pe | mingw32* | hpux*)
+        hpux*)
           enable_symvers=no ;;
         *)
           enable_symvers=gnu ;;

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

* Re: Libstdc++ as DLL on windows, alternative approach [was Re: cygwin   patch review]
  2009-11-20 16:56     ` Dave Korn
@ 2009-11-21  6:34       ` Danny Smith
  2009-11-23 17:47       ` Benjamin Kosnik
  2009-11-30 20:21       ` Dave Korn
  2 siblings, 0 replies; 10+ messages in thread
From: Danny Smith @ 2009-11-21  6:34 UTC (permalink / raw)
  To: Dave Korn
  Cc: Benjamin Kosnik, libstdc++,
	GCC Patches, Aaron W. LaFramboise (GCC),
	Kai Tietz

On Sat, Nov 21, 2009 at 5:59 AM, Dave Korn  wrote:
> Benjamin Kosnik wrote:
>
>>> However it does sound like the right thing to do, so I've spun a new
>>> version of the patch that applies dllimport to the namespace.  We
>>> could commit this and then open a PR about dllimport not working on
>>> namespaces.
>
>> I like this plan. What's the PR number? And can you affix your
>> patch from here to it:
>> http://gcc.gnu.org/ml/gcc-patches/2009-11/msg00979.html
>
>  This is now PR 42123.  (The WIP patch is on the right lines but not quite
> correct yet, details in the PR.)
>
>> Thanks.
>>
>>>   Initial test results (c and c++ only) are posted(*) and look right,
>>> but I'll do a clean run on the same source revision for comparison
>>> anyway.  Note that I had to patch various testsuite/lib/ files to
>>> avoid the current utf-8 problem(**) that would otherwise have
>>> provoked many spurious 'excess errors' warnings.  That'll be the
>>> subject of a separate patch, I'll do the clean run using them as well
>>> so the comparison is fair.
>>
>> It looks like (in later messages) you have an updated patch.
>
>  I've since made one small change (apart from experimenting with the
> namespace patch).  In testing the namespace patch, I discovered an order of
> include problem, where this code from include/bits/c++config:
>
>>  #if _GLIBCXX_HAVE_ATTRIBUTE_VISIBILITY
>>  # define _GLIBCXX_VISIBILITY_ATTR(V) __attribute__ ((__visibility__ (#V)))
>> +#elif defined (_GLIBCXX_PSEUDO_VISIBILITY)
>> +# define _GLIBCXX_VISIBILITY_ATTR(V) _GLIBCXX_PSEUDO_VISIBILITY(V)
>>  #else
>>  # define _GLIBCXX_VISIBILITY_ATTR(V)
>>  #endif
>
> is before any of the os/cpu-specific headers get #included, so it didn't work.
>  I've rearranged it now so that this site at the top of the file just says
>
>> #if _GLIBCXX_HAVE_ATTRIBUTE_VISIBILITY
>> # define _GLIBCXX_VISIBILITY_ATTR(V) __attribute__ ((__visibility__ (#V)))
>> #else
>> // If this is not supplied by the OS-specific or CPU-specific
>> // headers included below, it will be defined to an empty default.
>> # define _GLIBCXX_VISIBILITY_ATTR(V) _GLIBCXX_PSEUDO_VISIBILITY(V)
>> #endif
>
> .. and then after the #includes I added
>
>> // If platform uses neither visibility nor psuedo-visibility,
>> // specify empty default for namespace annotation macros.
>> #ifndef _GLIBCXX_PSEUDO_VISIBILITY
>> #define _GLIBCXX_PSEUDO_VISIBILITY(V)
>> #endif
>
>  I've verified this change DTRT by diffing pre-processed sources compiled
> with and without -D_GLIBCXX_DLL.
>
>> But your
>> updated results here looks great:
>> http://gcc.gnu.org/ml/gcc-testresults/2009-11/msg01084.html
>
>  Yep, the results are now consistently the same as for static libstdc++.
>
>>>   So, is this any more like OK for head?
>>
>> Yes, exactly. Thanks. This is OK with me.
>
>  :)  Fantastic.  Three small things which need explicit confirmation:
>
> - can your OK cover the c++-related change to LINK_SPEC in gcc/gcc.c or do I
> need to get another maintainer (probably jsm) for that?
> - is it OK even with the change to the c++config hunk mentioned above?
> - is it OK even in stage 3?
>
>  Also, PING! Danny/Kai, the windows side of this patch has changed a bit
> since you guys OK'd it some time back, can one of you take a look over the new
> changes to config/i386/winnt.c w.r.t. i386_find_on_wrapper_list() and #ifdef
> CXX_WRAP_SPEC_LIST?
>
>  I've just updated my sandbox to head.  Attached, the final version of the
> patch for reference; I'm just rebuilding it now to check nothing has broken.
> The ChangeLog is almost the same as last time, but mentions the c++config change:
>

OK. Thanks

Danny

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

* Re: Libstdc++ as DLL on windows, alternative approach [was Re:  cygwin  patch review]
  2009-11-20 16:56     ` Dave Korn
  2009-11-21  6:34       ` Danny Smith
@ 2009-11-23 17:47       ` Benjamin Kosnik
  2009-11-30 20:21       ` Dave Korn
  2 siblings, 0 replies; 10+ messages in thread
From: Benjamin Kosnik @ 2009-11-23 17:47 UTC (permalink / raw)
  To: Dave Korn
  Cc: Dave Korn, libstdc++,
	GCC Patches, Danny Smith, Aaron W. LaFramboise (GCC),
	Kai Tietz


> > I like this plan. What's the PR number? And can you affix your
> > patch from here to it:
> > http://gcc.gnu.org/ml/gcc-patches/2009-11/msg00979.html
> 
>   This is now PR 42123.  (The WIP patch is on the right lines but not
> quite correct yet, details in the PR.)

Thanks for this.

> - can your OK cover the c++-related change to LINK_SPEC in gcc/gcc.c
> or do I need to get another maintainer (probably jsm) for that?

You'll need ok from the appropriate maintainer. Looks ok to me.

> - is it OK even with the change to the c++config hunk mentioned above?

Yes.

> - is it OK even in stage 3?

Yes.

> gcc/ChangeLog:
> 
> 	* configure.ac (USE_CYGWIN_LIBSTDCXX_WRAPPERS): Define to
> reflect status of AC_CHECK_FUNC for Cygwin DLL libstdc++ support
> wrappers.
> 	* configure: Regenerate.
> 	* config.in: Regenerate.
> 
> 	* config/i386/cygwin.h (CXX_WRAP_SPEC_LIST): Define list of
> --wrap options for Cygwin DLL libstdc++ support wrappers.
> 	(CXX_WRAP_SPEC_OPT): Define spec to use wrappers or not by
> default according to defined value of USE_CYGWIN_LIBSTDCXX_WRAPPERS.
> 	(CXX_WRAP_SPEC): Define entire wrapper spec in or out
> according to whether USE_CYGWIN_LIBSTDCXX_WRAPPERS is even defined or
> not. (LINK_SPEC): Include CXX_WRAP_SPEC.
> 	* gcc/config/i386/winnt.c (wrapper_strcmp): New qsort helper
> function. (i386_find_on_wrapper_list): Check if a function is found
> on the list of libstdc++ wrapper options.
> 	(i386_pe_file_end): If we are importing a wrapped function,
> also emit an external declaration for the real version.
> 	* config/i386/cygming.opt (muse-libstdc-wrappers): New option
> for Cygwin targets. Update copyright year.
> 
> 	* gcc.c (LINK_COMMAND_SPEC): Allow and ignore
> -static-libstdc++ similarly to -static.
> 
> gcc/cp/ChangeLog:
> 
> 	* g++spec.c (SKIPOPT): Delete.
> 	(lang_specific_driver): Do not skip -static-libstdc++ option.
> 

All this looks ok to me but I cannot approve.

> libstdc++-v3/ChangeLog:
> 
> 	* libstdc++-v3/acinclude.m4 (GLIBCXX_ENABLE_SYMVERS): Don't
> disable on PE targets.
> 	* libstdc++-v3/configure: Regenerate.
> 	* libstdc++-v3/configure.host: Add libtool DLL options for
> Cygwin and MinGW platforms.
> 
> 	* libstdc++-v3/include/bits/c++config
> (_GLIBCXX_VISIBILITY_ATTR): On platforms that don't support
> visibility, allow them to declare a macro _GLIBCXX_PSEUDO_VISIBILITY
> that is applied in place of visibility. (_GLIBCXX_PSEUDO_VISIBILITY):
> Supply empty default if not declared by CPU- or OS-specific headers.
> 
> 	* libstdc++-v3/config/os/newlib/os_defines.h
> 	(_GLIBCXX_PSEUDO_VISIBILITY_default): New macro for dllimport.
> 	(_GLIBCXX_PSEUDO_VISIBILITY_hidden): New empty macro.
> 	(_GLIBCXX_PSEUDO_VISIBILITY): Evaluate to one of the above.
> 	* libstdc++-v3/config/os/mingw32/os_defines.h
> 	(_GLIBCXX_PSEUDO_VISIBILITY_default,
> 	_GLIBCXX_PSEUDO_VISIBILITY_hidden,
> 	_GLIBCXX_PSEUDO_VISIBILITY): Likewise.

OK.

best,
benjamin

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

* Re: Libstdc++ as DLL on windows, alternative approach [was Re: cygwin  patch review]
  2009-11-20 16:56     ` Dave Korn
  2009-11-21  6:34       ` Danny Smith
  2009-11-23 17:47       ` Benjamin Kosnik
@ 2009-11-30 20:21       ` Dave Korn
  2 siblings, 0 replies; 10+ messages in thread
From: Dave Korn @ 2009-11-30 20:21 UTC (permalink / raw)
  To: Dave Korn
  Cc: Benjamin Kosnik, libstdc++,
	GCC Patches, Danny Smith, Aaron W. LaFramboise (GCC),
	Kai Tietz

Dave Korn wrote:
> [snip]

  YA test results:
      http://gcc.gnu.org/ml/gcc-testresults/2009-11/msg02971.html

  Owing to imminent end of stage I guess I'll just have to check it in without
this part for which I haven't been able to get review in time:

> 	* gcc.c (LINK_COMMAND_SPEC): Allow and ignore -static-libstdc++
> 	similarly to -static.
> 
> gcc/cp/ChangeLog:
> 
> 	* g++spec.c (SKIPOPT): Delete.
> 	(lang_specific_driver): Do not skip -static-libstdc++ option.

  The end result won't be broken, the remaining parts of the patch will stand
alone, but it will lead to statically linked c++ code needlessly indirecting
through the cygwin dll malloc wrappers when it doesn't have to.  That's a
small inefficiency that can be solved whenever the patch can be approved.
It's also eminently suitable to go in as a separate patch anyway, since it
helps fix someone else's PR too.

    cheers,
      DaveK

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

end of thread, other threads:[~2009-11-30 20:12 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20091019092452.2e271791@mcgee.artheist.org>
2009-11-12 22:33 ` Libstdc++ as DLL on windows, alternative approach [was Re: cygwin patch review] Dave Korn
2009-11-16  2:57   ` Dave Korn
2009-11-16  3:34   ` Danny Smith
2009-11-16  3:37     ` Dave Korn
2009-11-19  1:25   ` Dave Korn
2009-11-19 17:39   ` Benjamin Kosnik
2009-11-20 16:56     ` Dave Korn
2009-11-21  6:34       ` Danny Smith
2009-11-23 17:47       ` Benjamin Kosnik
2009-11-30 20:21       ` Dave Korn

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).