Re: [RFA] pei386 dll: auto-import patch

[Charles Wilson <cwilson at ece dot gatech dot edu>]

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