This is the mail archive of the
cygwin-apps@cygwin.com
mailing list for the Cygwin project.
Re: [RFA] pei386 dll: auto-import patch
- To: DJ Delorie <dj at delorie dot com>
- Subject: Re: [RFA] pei386 dll: auto-import patch
- From: Charles Wilson <cwilson at ece dot gatech dot edu>
- Date: Wed, 01 Aug 2001 14:13:38 -0400
- CC: binutils at sources dot redhat dot com, cygwin-apps at cygwin dot com
- References: <3B670087.7090102@ece.gatech.edu> <200108011735.NAA32231@envy.delorie.com>
DJ Delorie wrote:
> A couple of comments:
Thanks! I will attempt to address them unless Paul or Robert beats me
to it. I do want to point out one thing: the ONLY changes in "my" patch
that are different from Robert's pre-existing version are the ld.texinfo
and gen-doc.texi changes. That's it. (I'm not sure how much Robert's
version differs from Paul's original, or where the various pieces
originated).
>
> Please use something more neutral than "gory debug". How about
> "--enable-extra-pe-debug"?
OK.
> I don't like having a global in bfd/cofflink.c that's used by
> bfd/linker.c and ld.
Yeah, that looked a little fishy to me -- but I did not want to modify
actual code (docs, ok) for fear of obsoleting all of Roberts' and my
testing of the current version. A "danger" you allude to, below...
> Please either encapsulate that logic in the
> pe-specific files in the linker, or make that variable somehow not a
> global (i.e. find some structure, say link_info, where we can pass
> that flag). In the very least, having linker.c depend on cofflink.c
> will adversely affect every non-coff target.
Very good point. I hadn't thought of that.
> You should have matching --enable and --disable options.
OK.
> The default for auto-import should be disabled, until it has been well
> tested (I mean *this* version, not a pre-existing version in a
> different distribution). Especially since you rely on violating the
> Win32 spec to accomplish this.
</begin relutant agreement/>
There are really only two ways this code gets activated. #1: building a
DLL, AND --export-all-symbols is used -- either explicitly, or
implicitly because of lack of .def / declspec() directives. This logic
for turning on the --export-all-symbols behavior is pre-existing; the
auto-import support (e.g. adding the DATA thunking symbols) hooks into
that). #2. linking to a dll that already possesses the DATA thunking
symbols -- if no thunking symbols, then code is not "activated".
In either case, #1 or #2, it's not really "on by default", is it? It
only activates in certain cases -- which require user intervention to
create...
BUT, your point is taken. I'd rather worry about turning on the
"--enable-auto-import" flag for a while, than not have the capability at
all.
Ultimately, I believe the default should be "on"...eventually. Mainly
because it adds desirable behavior, yet does no harm to code/libs that
currently build and link properly. And it gets rid of this ugly junk:
#if building_dll
# define DLIMPORT __declspec(dllexport)
#else if linking_to_dll
# define DLIMPORT __declspec(dllimport)
# else
# define DLIMPORT
# endif
#endif
Of course, even if auto-import defaults to off, just moving the DLL
"problem" to a purely link-time flag, rather than a combinataion
compile-time AND link-time flag, is an improvement. Just so we can get
rid of the compile/link "flag-matching" requirements: "gcc -c
-DZLIB_STATIC + ld -Bstatic" or "-DZLIB_DLL + ld -Bdynamic"
</end reluctant agreement>
> The ChangeLog formatting is wrong. Among other things, you only need
> list each file name once for each set of related changes.
OK. I'll work on it. (I know there are other problems, but in my
defense, something weird happened with the spacing in the email...I'm
not sure what. Probably some sort of tab/space conversion.
> The code style is wrong (binutils follows the GNU coding standards).
????
I'm not sure about this; I'll go thru the code and try to make the
spacing line up like the surrounding code. Is there something else I'm
missing?
> The documentation about how auto-import works probably should go in
> the ld internals doc, not the user's guide.
Oh. OK.
> Please post the ldlang.c patch separately; it is not pe-specific.
Actually, Danny told me that this part of the patch should be removed
completely. See
http://sources.redhat.com/ml/binutils/2001-04/msg00367.html for
Nick Clifton's comments
> Do not use C++ comments in C code.
I will convert these.
> Do not use the term "bugger" in comments. Please stick to technical
> names, not derogatory ones.
??? Where's THAT? I'll find it and remove it.
> Perhaps that long list of strcmp's should be replaced with a table?
That's probably a good idea. (My suspicion is this list is only going
to get longer...) I'll take a look at "table-izing" it.
DJ, *thanks* for your comments. I've been hoping someone more closely
allied with the main binutils development would take a look and comment
on this change. I had felt that "we" have been operating in a vacuum...
--Chuck