cygwin patch review

[Dave Korn]

[ Pardon the slowness of response, but as you can see there was a lot to
write, and I've got another job on at the moment as well as this. ]

Benjamin Kosnik wrote:

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

  More about the general subject of visibility below.  I'm not sure it's quite
the right concept.

> This should work out of the box... and remove the need for partial
> attribute-ization. I'm concerned with what is and what is not annotated
> with dllimport. What's the criteria here?

  OK, you got me: I'm not entirely sure.  This patch has been kicking around
the windows downstreams for a long time and has been handed down the
generations a bit.  I've Cc'd Danny and Aaron from whom I got the original
version; maybe they know about the motivation behind these exact choices.

> My best guess:
> 1) stuff with _GLIBCXX_EXTERN_TEMPLATE
> 2) exception classes
> 3) io classes

  FWIW, that's what I have inferred by observation as well, and I noticed (and
corrected) a few omissions from that pattern, which showed up as testsuite
"excess errors" failures triggered by ld auto-import warnings.  The pattern
appears to be "everything with data exports that if not marked dllimport,
would cause ld to issue an auto-import warning at link time."

> What about the allocators, containers, TR1 exceptions, C++0x exceptions,
> etc? Can you provide some insight on how the libstdc++ maintainers will
> choose how to deploy _GLIBCXX_IMPORT?

  My vague plan was for the libstdc++ maintainers not to have to worry about
it much, and leave it to the windows maintainers to update.  The minimal
criteria is this: every compound data object exported from the DLL, array, POD
or otherwise, needs to be marked up, as the linker cannot use auto-import to
resolve a reference to an embedded member of a structure (i.e., at a non-zero
offset from the base).  Marking up data scalars and functions is not
essential, as they can always be resolved by auto-import.  However auto-import
is inefficient, and it would be desirable to try and mark up the whole library
so that auto-import need not be used.

> Looks like darwin started using this around 2006-12-02, which may be
> after the design for cywin was established.

  By "using this", I think you mean visibility attributes on the namespace?

>> Only on ELF
>> platforms you'd define _GLIBCXX_IMPORT to __attribute__ ((visibility
>> ("default"))) while compiling libstdc++ and use -fvisibility=hidden?  
> 
> No. You're just using -fvisibility=hidden and maybe
> -fvisibility-inlines-hidden, although I'm not sure what the status is on
> this flag.

  I'll explain in a minute why I don't think visibility can solve the whole
problem.

> Everything else is done via attributes on namespace std. I don't know if
> attribute (dllimport) works on namespace std. Does it? That might be
> another option.
> 
> After talking to Jason for a bit it looks like dllimport on namespace
> std may not work right now. I think the visibility attributes are the
> only things that work on namespaces. There is certainly no reason why
> other attributes couldn't work.

  Well, I have verified this.  dllimport does nothing on namespaces, it has to
be applied to the actual item.  I think it might be a good idea to make it
work in that way, but that might be too risky for stage 3.

>> Were you
>> planning to make -fvisibility work in some way on windows, or just use
>> #definery to use dllimport/dllexport there?  
> 
> Look at what Darwin is doing here.
> 
> Does visibility not work on cygwin? More details about this option on PE
> hosts please. It is supposed to work.

  As far as I know.... symbol visibility is an entirely ELF thing.  Maybe
Darwin supports it too, I don't know, but there's no corresponding concept in
COFF and PE, no bits defined in the file format anywhere to store the
visibility of the symbols, no handling for it in linkers and loaders.  I don't
know, does the compiler do anything with visibility internally?  Or does it
just pass it down the toolchain by setting symbol flags in the generated
assembler source?  I thought it was an entirely lower-toolchain feature.

> My other questions for you, after some poking around:
> 
> 1) What would it take to map the baseline_symbols.txt file to a suitable
> .def file to control exports on cygwin? Are there known issues and if
> so, what?

  Actually, this one is easy.  The PE versions of LD accept ELF version
scripts directly and use them to filter down the list of exported symbols in
auto-export mode (it doesn't do symbol versioning, just pays attention to the
public/private declarations).  We just need to fix the configury to enable the
version script on cygwin even though versioning itself isn't supported.  I
tried this:

Index: libstdc++-v3/acinclude.m4
===================================================================
--- libstdc++-v3/acinclude.m4	(revision 152966)
+++ 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 ;;

but it looks like I'll need to take further steps as there's no sign of a .ver
script being used in the build logs.

> 2) operator new/delete replaceable signature stuff looks cool. However,
> isn't this different behavior than with the microsoft runtimes, which
> don't support this? FWIW, I thought intrapostion didn't work on that
> platform. To me, it looks like you've solved this for the GNU
> toolchain. Yay. This part of your patch is ok with me for sure. 

  It may well be different from the MS runtimes, although I suspect they
support it when using static linking at the very least.  Cygwin doesn't care
about behaving the same as the MS runtimes, we want to be as Linx/POSIX
compatible as possible, so we want to implement this feature.  MinGW uses the
MSVC runtime, and on that target the feature is disabled.  It doesn't
implement genuine intraposition; there is no way round the fact that all
symbols references have to be fully resolved at link time on PE platforms.
Instead it relies on ld to redirect references to wrapper functions, which
have been implemented by the Cygwin DLL.  This is all highly Cygwin- and
Gnu-specific.

> But where to note this kind of usage stuff so that when people go mad
> trying to debug some subtle thing, we can point to a web page and claim
> not guilty? I am not even quite sure that there are new/delete
> intraposition tests in the testsuite.

  The test suite contains two tests that verify function replacement with full
interposition into library calls:

g++.old-deja/g++.abi/cxa_vec.C execution test
g++.old-deja/g++.brendan/new3.C execution test

  They both begin with this comment:

> // This test fails on VxWorks in kernel mode because it depends on the
> // library version of "::operator new[]" calling the "::operator new"
> // defined in this module.  This doesn't work because the library version
> // of "::operator new[]" is built into the kernel itself; library relocations
> // are resolved when the kernel is linked.

and they both PASS on Cygwin when linking statically, or with this patch, but
FAIL if you build a DLL without the redirection features implemented here and
supported by wrappers in recent versions of the Cygwin DLL.  I'm not sure what
kind of "usage stuff" you mean exactly here; I thought this was just the
specified behaviour defined by the C++ standard.  Still, if you need someone
to blame, point at the Cygwin mailing list and claim not guilty; any problems
that crop up can be triaged and diagnosed there.

> 3) How are you documenting the static/shared libgcc/libstdc++ issues? It
> seems that there is some subtlety involved with these flags and the DLL
> version of libstdc++. (No ideas on where to document, but this is
> getting confusing enough that it should be done.)

  The only subtlety is that we don't want to pass --wrap options to ld for
static linking because the whole mechanism is superfluous overhead in that
case; function replacement and intraposition has always worked just fine in
that case.



  Now, about visibility.  I think we're somewhat at cross purposes here, but
I'm not an ELF-head, so it's possible I've got the wrong end of the stick.  My
understanding is that visibility is a flag that we set on symbols when
compiling the library, which is used later down the toolchain, when
subsequently linking an executable against that library, to control which
symbols the executable can actually see; the purpose being to avoid namespace
pollution and prevent executables from attempting to call private internal
APIs of the library or otherwise messing around "inside the black box".

  On a Windows platform, the choice of which symbols are linkable from a DLL
is controlled by dllexport, which can be applied either by annotating the
sources in the compiler, or by writing a .DEF file for the linker that names
the symbols to be exported.  In the GNU toolchain we have provided an extra
pathway, since it is our goal on Cygwin to be able to compile Linux (and other
POSIX) applications without having to add #ifdef WINDOWS everywhere and mark
up everything with DLL attributes: if you don't specify any symbols at all as
dllexports, then when LD is generating a DLL, it will export all the
externally visible symbols.  This technique obviously does pollute the
namespace (although it doesn't cause problems often in practice), but we can
now address it with a version script, which if I understand correctly will
work to the same end as applying the visibility attributes does (on darwin?)

  What visibility doesn't address however is the other side of the equation,
when importing a DLL symbol into a user application.  The normal way of
working in the windows world is that all the symbols referenced from a DLL by
an exe are gathered together into an "import table" (a separate section
containing just a pointer to each imported symbol) at link time, and the
compiler generates code to access dllimported symbols by indirecting a
reference through this table, rather than directly.

  This avoids the whole text-segment-full-of-relocs problem that can arise on
ELF platforms, but the compiler does need to be aware of it when generating
the code that references a symbol if it is going to be imported from a DLL,
and that is the purpose of adding the _GLIBCXX_IMPORT annotations.

  Now, similarly to how we have auto-export features in the toolchain to
assist users in recompiling standard Linux applications without needing to
annotate them, we have an auto-import feature to the same purpose.  In this
scheme, the linker does indeed fill the text section full of 'relocs', or to
be precise, single one-entry four-byte import tables (that just happen, as if
by coincidence, to overlap the immediate address operand field of an assembly
instruction such as a jump or call...), with the same implications as on ELF
for security and memory overheads from not being able to share COW-modified
pages of supposed-to-be-read-only text section between separate simultaneous
running instances of the same program.

  We could just suck it up and live with the inefficiencies on Windows, except
unfortunately we can't.  It is a fundamental limitation of using import tables
rather than genuine relocs (which is necessary because the windows OS runtime
linker only knows about imports, not relocs) that you can't apply offsets, you
can only get an operand "relocated" to point to the actual address of an
export from a DLL.  So, while auto-import works fine for simple function calls
and loads and stores of scalar variables, it just cannot be made to work when
the compiler generates a complex offsetted reference to an array or struct
member.  There is an example in the LD manual: see the auto-import(*)
documentation, and there is a bit of summary information in the "automatic
data imports" section of the win32-specific notes(**), but the long and the
short of it is, if you want to export complex data structures from a DLL on
windows, you have to use the annotations.  (There's also a *third* custom
mechanism implemented in LD, "runtime pseudo-relocs"; these overcome the
limitation that you can't have an offsetted imported address, but can't
resolve the other problems of writeable text segments, and are a recent
development that isn't yet fully supported.)

  Anyway, I tried annotating the namespaces, and as Jason expected, it didn't
work.  I'd like to take that more neat approach in the long run, but we'd need
to make the dllimport attribute inheritable from the namespace in a way that
most likely (I haven't actually scoped it out yet, this is a guesstimate)
would need changes to the C++ FE of a sort that aren't desirable for stage 3.

  So, here's a possible way forward: let's add these annotations for now, so
that 4.5.0 can get out the door and Windows users won't have to wait yet
another half year or more for proper C++ support on their platform.  They may
be ugly, but they're only simple little macros and we can be confident they
won't cause any harm on any of the other platforms where they just get defined
away to nothing.  Then, during next stage 1, I'll make the dllimport attribute
work on namespaces how we want it to, and once that's done we'll rip the whole
lot straight out again and just define the visibility macros to add dllimport
annotation on windows platforms.

  I believe that should reduce the potential maintenance problem to near zero,
because we're not going to be doing ABI-changing things like adding and
removing exports on the 4.5 branch, so the set of annotations there should
suffice, and then before they can become a problem on mainline we'll have
ripped them all out in favour of modified visibility macros that simply and
easily annotate the entire namespace at one time.

  How does it sound to you?


    cheers,
      DaveK
-- 
(*)  - http://tinyurl.com/yglqhta aka
http://sourceware.org/binutils/docs-2.20/ld/Options.html#index-g_t_002d_002denable_002dauto_002dimport-296
(**) - http://tinyurl.com/yzqwcn8 aka
http://sourceware.org/binutils/docs-2.20/ld/WIN32.html#index-automatic-data-imports-651