Re: Serious bug in auto-import

Re: Serious bug in auto-import

[Paul Sokolovsky]

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

Hello Charles,

Charles Wilson wrote on Tuesday, August 28, 2001:

CW> Paul,

CW> Please take a look at the thread "[aida_s@mx12.freecom.ne.jp: A serious
CW> bug of "ld --enable-auto-import"]" starting here:

CW> http://sources.redhat.com/ml/binutils/2001-08/msg00595.html

CW> This is a confirmed bug.  I've tried to analyze it but I can't really
CW> come up with a fix; I wonder if a fix is possible at all.  If this
CW> problem can't be fixed, I'm going to have to recommend removing the
CW> auto-import stuff. (Yes, it's that serious.)

CW> PLEASE take a look and at least let me know you've seen it; it was
CW> originally reported early Friday morning to the cygwin-apps list, and
CW> moved to the binutils list on Saturday.

    I'm sorry, I was on vacation abroad.

    Yes, the weird stuff. Of course, I though about the issue when
started to work on auto-import, but my reasoning was misgone along the
lines which were evident in this thread too: I confused *relocs* and
*import thunks*. The former support addends, the latter - no. Well, I
never bothered to check issue afterwards.

     How disabling such a misfeature can be? Well, my opinion is that
sample code far-fetched and presents, err, bad software design. The
only sane usage of accessing external array with constant indexes I
can imagine is something like

          extern double world_transform_matrix[3][3];
          ...

     I doubt much code uses such a horror, so losing ability to auto-import
contant-offset array for me is not a big loss. Much more pitiful fact
is that it also applies to accessing global extern struct's - I bet
periodically code which does that will be hit. Well, nothing comes for
free.

     Actions to be taken: well, I don't think it's productive idea,
having hit some particular difficulty, give up entirely thing which
otherwise works well. Imho, the better idea is to make quick surgery
to localize the problem. That's exactly what attached patch does:

====
#include <stdio.h>
extern struct
{
  int a;
  int b;
} st;

int main(void){
        printf("%d\n", st.b);
        printf("%d\n", st.a);
}
====

gcc -s -Wl,--enable-auto-import -o hello.exe hello.o -L. -lhwstr
Warning: resolving _st by linking to __imp__st (auto-import)
hello.o(.text+0x13):hello.c: aggregate 'st' is referenced in direct addressing m
ode with constant offset - auto-import failed. Workarounds: a) mark the symbol w
ith __declspec(dllimport); b) use auxiliary variable
make: *** [hello.exe] Hangup


CW> --Chuck


--
Paul Sokolovsky, IT Specialist
http://www.brainbench.com/transcript.jsp?pid=11135
ld.diff.20010909
ChangeLog.20010909


[-- Attachment #2: ChangeLog.20010909 --]
[-- Type: text/plain, Size: 431 bytes --]

2001-09-09  Paul Sokolovsky  <Paul.Sokolovsky@technologist.com>

	* emultempl/pe.em (make_import_fixup): Receive additional
	argument - section to which reloc belongs. Check that reloc
	doesn't have non-zero inline addend. If it does, fail
	auto-import session, because import thunks do not support
	addends.
	* pe-dll.c, pre-dll.h: Corresponding changes to argument
	passing. Also, minor formatting, naming and warning
	cleanups.

[-- Attachment #3: ld.diff.20010909 --]
[-- Type: text/x-diff, Size: 3706 bytes --]

Index: pe-dll.c
===================================================================
RCS file: /cvs/src/src/ld/pe-dll.c,v
retrieving revision 1.26
diff -u -r1.26 pe-dll.c
--- pe-dll.c	2001/08/21 11:42:57	1.26
+++ pe-dll.c	2001/09/08 20:56:57
@@ -982,10 +982,10 @@
 pe_walk_relocs_of_symbol (info, name, cb)
      struct bfd_link_info *info;
      CONST char *name;
-     int (*cb) (arelent *);
+     int (*cb) (arelent *, asection *);
 {
   bfd *b;
-  struct sec *s;
+  asection *s;
 
   for (b = info->input_bfds; b; b = b->link_next)
     {
@@ -1003,7 +1003,7 @@
 	      && s->output_section == bfd_abs_section_ptr)
 	    continue;
 
-	  current_sec=s;
+	  current_sec = s;
 
 	  symsize = bfd_get_symtab_upper_bound (b);
 	  symbols = (asymbol **) xmalloc (symsize);
@@ -1016,7 +1016,7 @@
 	  for (i = 0; i < nrelocs; i++)
 	    {
 	      struct symbol_cache_entry *sym = *relocs[i]->sym_ptr_ptr;
-	      if (!strcmp(name,sym->name)) cb(relocs[i]);
+	      if (!strcmp(name,sym->name)) cb(relocs[i], s);
 	    }
 	  free (relocs);
 	  /* Warning: the allocated symbols are remembered in BFD and reused
@@ -1908,7 +1908,7 @@
   /* we convert reloc to symbol, for later reference */
   static int counter;
   static char *fixup_name = NULL;
-  static int buffer_len = 0;
+  static unsigned int buffer_len = 0;
   
   struct symbol_cache_entry *sym = *rel->sym_ptr_ptr;
   
Index: pe-dll.h
===================================================================
RCS file: /cvs/src/src/ld/pe-dll.h,v
retrieving revision 1.4
diff -u -r1.4 pe-dll.h
--- pe-dll.h	2001/08/02 23:12:02	1.4
+++ pe-dll.h	2001/09/08 20:56:57
@@ -48,7 +48,7 @@
 
 extern void pe_walk_relocs_of_symbol PARAMS ((struct bfd_link_info * info,
 					     CONST char *name,
-					     int (*cb) (arelent *)));
+					     int (*cb) (arelent *, asection *)));
 
 extern void pe_create_import_fixup PARAMS ((arelent * rel));
 #endif /* PE_DLL_H */
Index: emultempl/pe.em
===================================================================
RCS file: /cvs/src/src/ld/emultempl/pe.em,v
retrieving revision 1.50
diff -u -r1.50 pe.em
--- pe.em	2001/08/31 13:30:12	1.50
+++ pe.em	2001/09/08 20:57:12
@@ -127,7 +127,7 @@
 static boolean pe_undef_cdecl_match
   PARAMS ((struct bfd_link_hash_entry *, PTR));
 static void pe_fixup_stdcalls PARAMS ((void));
-static int make_import_fixup PARAMS ((arelent *));
+static int make_import_fixup PARAMS ((arelent *, asection *));
 static void pe_find_data_imports PARAMS ((void));
 #endif
 
@@ -845,20 +845,36 @@
 }
 
 static int
-make_import_fixup (rel)
+make_import_fixup (rel, s)
   arelent *rel;
+  asection *s;
 {
   struct symbol_cache_entry *sym = *rel->sym_ptr_ptr;
-/*
-  bfd *b;
-*/
 
   if (pe_dll_extra_pe_debug)
     {
       printf ("arelent: %s@%#x: add=%li\n", sym->name,
               (int) rel->address, rel->addend);
     }
-  pe_create_import_fixup (rel);
+
+  {
+    int addend = 0;
+    if (!bfd_get_section_contents(s->owner, s, &addend, rel->address, sizeof(addend)))
+      {
+        einfo (_("%C: Cannot get section contents - auto-import exception\n"),
+               s->owner, s, rel->address);
+      }
+
+    if (addend == 0)
+      pe_create_import_fixup (rel);
+    else
+      {
+        einfo (_("%C: aggregate '%T' is referenced in direct addressing mode with constant offset - auto-import failed. Workarounds: a) mark the symbol with __declspec(dllimport); b) use auxiliary variable\n"),
+               s->owner, s, rel->address, sym->name);
+        einfo ("%X");
+      }
+  }
+
   return 1;
 }
 
@@ -931,6 +947,8 @@
   struct bfd_hash_entry *h;
   PTR string ATTRIBUTE_UNUSED;
 {
+  (void)string;
+
   if (pe_dll_extra_pe_debug)
     {
       printf("+%s\n",h->string);

Re: Serious bug in auto-import

[Charles Wilson]

Paul Sokolovsky wrote:

> CW> Please take a look at the thread "[aida_s@mx12.freecom.ne.jp: A serious
> CW> bug of "ld --enable-auto-import"]" starting here:
> 
> CW> http://sources.redhat.com/ml/binutils/2001-08/msg00595.html
> 
> CW> This is a confirmed bug.  I've tried to analyze it but I can't really
> CW> come up with a fix; I wonder if a fix is possible at all.  If this
> CW> problem can't be fixed, I'm going to have to recommend removing the
> CW> auto-import stuff. (Yes, it's that serious.)
> 
> CW> PLEASE take a look and at least let me know you've seen it; it was
> CW> originally reported early Friday morning to the cygwin-apps list, and
> CW> moved to the binutils list on Saturday.
> 
>     I'm sorry, I was on vacation abroad.


Oh -- this bug cropped up at a "bad" time for you. :-)   Thanks for 
responding.


>     Yes, the weird stuff. Of course, I though about the issue when
> started to work on auto-import, but my reasoning was misgone along the
> lines which were evident in this thread too: I confused *relocs* and
> *import thunks*. The former support addends, the latter - no. Well, I
> never bothered to check issue afterwards.
> 
>      How disabling such a misfeature can be? Well, my opinion is that
> sample code far-fetched and presents, err, bad software design. The
> only sane usage of accessing external array with constant indexes I
> can imagine is something like
> 
>           extern double world_transform_matrix[3][3];
>           ...
> 
>      I doubt much code uses such a horror, so losing ability to auto-import
> contant-offset array for me is not a big loss. 


Agree.  (NOTE: following paragraph was written before I read the rest of 
your message.  You've already implemented the following! :-)

But, if code DOES use this (assuming we can't "fix" it) -- then the poor 
unsuspecting user should be warned "Attempt to access array via constant 
offset" or some such, with a pointer to some documentation somewhere 
that says "The following idiom is not supported in client code for array 
variables exported from DLLs on pei-386:
    "array_exported_from_dll[constant_index] = foo"
or "foo = array_exported_from_dll[constant_index]"
Please replace with the following idiom:
    <type> *local_copy = array_exported_from_dll;
    local_copy[constant_index] = foo;
or foo = local_copy[constant_index];"
(yeah, yeah, I know.  pointers, arrays.  the above is meant as a 
starting point; not to be taken literally)

> Much more pitiful fact
> is that it also applies to accessing global extern struct's - I bet
> periodically code which does that will be hit. 


Oooh.  I hadn't thought of that.

> Well, nothing comes for
> free.
> 
>      Actions to be taken: well, I don't think it's productive idea,
> having hit some particular difficulty, give up entirely thing which
> otherwise works well.


True.  I panicked.  (At least it was private mail; I hadn't (and still 
have not)  made that suggestion onlist.

> Imho, the better idea is to make quick surgery
> to localize the problem. That's exactly what attached patch does:
> 
> ====
> #include <stdio.h>
> extern struct
> {
>   int a;
>   int b;
> } st;
> 
> int main(void){
>         printf("%d\n", st.b);
>         printf("%d\n", st.a);
> }
> ====
> 
> gcc -s -Wl,--enable-auto-import -o hello.exe hello.o -L. -lhwstr
> Warning: resolving _st by linking to __imp__st (auto-import)
> hello.o(.text+0x13):hello.c: aggregate 'st' is referenced in direct addressing m
> ode with constant offset - auto-import failed. Workarounds: a) mark the symbol w
> ith __declspec(dllimport); b) use auxiliary variable
> make: *** [hello.exe] Hangup
> 


AHA!  Exactly what I suggested above -- but I didn't know how to make ld 
do that.  Very cool.  Good work, Paul.  Warning when something fails and 
identifying the spot where the failure occurs is MUCH better than 
silently allowing buggy code. :-)

I'm going to test this; assuming it works as advertised (and I have no 
reason to think it will not) I recommend immediate inclusion.

THEN we can think about a "real" fix for this array-index problem (if 
possible; it may not be). :-)

--Chuck

P.S. Paul: also, please take a look here:
ld-auto-import memory bug fixing
http://sources.redhat.com/ml/binutils/2001-09/msg00062.html

Ralf has been trying (without success) to track down a memory leak 
problem that makes it difficult (impossible?) to link KDE apps to KDE 
DLL's via auto-import.  Too many dll's, too many symbols + memory leak 
== out of memory. The memory leak *may* be related to this snippet of 
code in pe-dll.c(pe_walk_relocs_of_symbol):

+ /* Warning: the allocated symbols are remembered in BFD and reused
+ 
      later, so don't free them! */
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
!!! Where do they be used later ??? Can this not be done in another way ?
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
+ 
   /* free (symbols); */

And here, in pe.em(pe_find_data_imports)

+            int nsyms, symsize, i;
+
+            symsize = bfd_get_symtab_upper_bound (b);
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
symbols will never be freed, only overwritten
!!!!!
+            symbols = (asymbol **) xmalloc (symsize);

Original bug description is here:
WG: ld --auto-import for cygwin and libtool
http://sources.redhat.com/ml/binutils/2001-06/msg00742.html

Re: Serious bug in auto-import

[Charles Wilson]

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

Charles Wilson wrote:

> 
> True.  I panicked.  (At least it was private mail; I hadn't (and still 
> have not)  made that suggestion onlist.


(of course, since Paul posted that excerpt onlist, my panic is now 
archived for the world to see.  But that's okay, Paul. :-)

 
>> Imho, the better idea is to make quick surgery
>> to localize the problem. That's exactly what attached patch does:


> I'm going to test this; assuming it works as advertised (and I have no 
> reason to think it will not) I recommend immediate inclusion.


Yep.  Tested.  ld with Paul's patch accurately detects, prints out a 
warning message, and exits-with-error whenever "direct addressing" is 
used in the .o file with a DLL auto-imported variable.  It doesn't 
"interfere" when the client code explicitly requests the import thunk 
(e.g. client uses __declspec(dllimport)) -- that continues to work as 
normal.

With arrays, this is good (and ld links without complaint):

extern <type> array_var_auto_imported_from_DLL;
<type> p[];
   p = array_var_auto_imported_from_DLL;
   p[12] = ....

Also, this is good:
   extern __declspec(dllimport) <type> array_var_from_DLL;
   array_var_from_DLL[12] = ....

This is bad (and Paul's ld detects, warns, and exits):
extern <type> array_var_auto_imported_from_DLL;
   array_var_auto_imported_from_DLL[12] = ....

Now, with structs, it's a little tricky.  Assume the following definition:
typedef struct {
   int a;
   int b;
} ST_;

This is bad:
extern ST_ st;
   st.a = ...

This is also bad (but is an analogous form to the "array" version that 
works)...
extern ST_ st;
ST_ p;
   p = st;
   p.a = ...

This is good:
extern ST_ st;
ST_ * p;
   p = &st;
   p->a = ...

Of course, that's almost identical to what happens when you do this 
(which is also good, of course):
extern __declspec(dllimport) ST_ st;
   st.a = ...

The good news is, Paul's patch will always detect, warn, and fail when 
the "bad" forms are used -- because it's triggered by the "bad" assember 
instructions directly, regardless of what C code produced it.  (That is, 
no weird list of special cases.)  The only question is, when a user gets 
Paul's error message, how will the user know HOW to fix the problem (in 
their client code)?  *That* requires a long list of special cases in 
some documentation somewhere...for extern auto-imported array access do 
this, for extern auto-imported struct access do that...

Unless of course:


> THEN we can think about a "real" fix for this array-index problem (if 
> possible; it may not be). :-)


I've attached a tar.gz of some test code (arrays, structs) that I used 
to verify Paul's patch.  Also, I've put a cygwin binary version of 
20010909 binutils (with Paul's patch) here:

http://www.neuro.gatech.edu/users/cwilson/cygutils/robert-collins/latest/

Since I built this binutils against a snapshot version of cygwin, you 
*may* need to also download cygwin-1.3.2-10.tar.bz2 from the same 
location.  (Warning: both were built with -g and are unstripped; the 
tarballs are 10 and 5 Meg, respectively)

--Chuck

[-- Attachment #2: auto-import-test.tar.gz --]
[-- Type: application/x-gzip, Size: 919 bytes --]

RE: Serious bug in auto-import

[Ralf Habacker]

> Paul Sokolovsky wrote:
>
> > CW> Please take a look at the thread "[aida_s@mx12.freecom.ne.jp: A serious
> > CW> bug of "ld --enable-auto-import"]" starting here:
> >
> > CW> http://sources.redhat.com/ml/binutils/2001-08/msg00595.html
> >
> > CW> This is a confirmed bug.  I've tried to analyze it but I can't really
> > CW> come up with a fix; I wonder if a fix is possible at all.  If this
> > CW> problem can't be fixed, I'm going to have to recommend removing the
> > CW> auto-import stuff. (Yes, it's that serious.)
> >
> > CW> PLEASE take a look and at least let me know you've seen it; it was
> > CW> originally reported early Friday morning to the cygwin-apps list, and
> > CW> moved to the binutils list on Saturday.
> >
> >     I'm sorry, I was on vacation abroad.
>
>
> Oh -- this bug cropped up at a "bad" time for you. :-)   Thanks for
> responding.
>
>
> >     Yes, the weird stuff. Of course, I though about the issue when
> > started to work on auto-import, but my reasoning was misgone along the
> > lines which were evident in this thread too: I confused *relocs* and
> > *import thunks*. The former support addends, the latter - no. Well, I
> > never bothered to check issue afterwards.
> >
> >      How disabling such a misfeature can be? Well, my opinion is that
> > sample code far-fetched and presents, err, bad software design. The
> > only sane usage of accessing external array with constant indexes I
> > can imagine is something like
> >
> >           extern double world_transform_matrix[3][3];
> >           ...
> >
> >      I doubt much code uses such a horror, so losing ability to auto-import
> > contant-offset array for me is not a big loss.
>
>
> Agree.  (NOTE: following paragraph was written before I read the rest of
> your message.  You've already implemented the following! :-)
>
> But, if code DOES use this (assuming we can't "fix" it) -- then the poor
> unsuspecting user should be warned "Attempt to access array via constant
> offset" or some such, with a pointer to some documentation somewhere
> that says "The following idiom is not supported in client code for array
> variables exported from DLLs on pei-386:
>     "array_exported_from_dll[constant_index] = foo"
> or "foo = array_exported_from_dll[constant_index]"
> Please replace with the following idiom:
>     <type> *local_copy = array_exported_from_dll;
>     local_copy[constant_index] = foo;
> or foo = local_copy[constant_index];"
> (yeah, yeah, I know.  pointers, arrays.  the above is meant as a
> starting point; not to be taken literally)
>
> > Much more pitiful fact
> > is that it also applies to accessing global extern struct's - I bet
> > periodically code which does that will be hit.
>
>
> Oooh.  I hadn't thought of that.
>
> > Well, nothing comes for
> > free.
> >
> >      Actions to be taken: well, I don't think it's productive idea,
> > having hit some particular difficulty, give up entirely thing which
> > otherwise works well.
>
>
> True.  I panicked.  (At least it was private mail; I hadn't (and still
> have not)  made that suggestion onlist.
>
> > Imho, the better idea is to make quick surgery
> > to localize the problem. That's exactly what attached patch does:
> >
> > ====
> > #include <stdio.h>
> > extern struct
> > {
> >   int a;
> >   int b;
> > } st;
> >
> > int main(void){
> >         printf("%d\n", st.b);
> >         printf("%d\n", st.a);
> > }
> > ====
> >
> > gcc -s -Wl,--enable-auto-import -o hello.exe hello.o -L. -lhwstr
> > Warning: resolving _st by linking to __imp__st (auto-import)
> > hello.o(.text+0x13):hello.c: aggregate 'st' is referenced in direct
> addressing m
> > ode with constant offset - auto-import failed. Workarounds: a) mark
> the symbol w
> > ith __declspec(dllimport); b) use auxiliary variable
> > make: *** [hello.exe] Hangup
> >
>
>
> AHA!  Exactly what I suggested above -- but I didn't know how to make ld
> do that.  Very cool.  Good work, Paul.  Warning when something fails and
> identifying the spot where the failure occurs is MUCH better than
> silently allowing buggy code. :-)
>
> I'm going to test this; assuming it works as advertised (and I have no
> reason to think it will not) I recommend immediate inclusion.
>
> THEN we can think about a "real" fix for this array-index problem (if
> possible; it may not be). :-)
>
> --Chuck
>
> P.S. Paul: also, please take a look here:
> ld-auto-import memory bug fixing
> http://sources.redhat.com/ml/binutils/2001-09/msg00062.html
>
> Ralf has been trying (without success) to track down a memory leak
> problem that makes it difficult (impossible?) to link KDE apps to KDE
> DLL's via auto-import.  Too many dll's, too many symbols + memory leak
> == out of memory. The memory leak *may* be related to this snippet of
> code in pe-dll.c(pe_walk_relocs_of_symbol):
>
> + /* Warning: the allocated symbols are remembered in BFD and reused
> +
>       later, so don't free them! */
> !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
> !!! Where do they be used later ??? Can this not be done in another way ?
> !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
> +
>    /* free (symbols); */
>
> And here, in pe.em(pe_find_data_imports)
>
> +            int nsyms, symsize, i;
> +
> +            symsize = bfd_get_symtab_upper_bound (b);
> !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
> symbols will never be freed, only overwritten
> !!!!!
> +            symbols = (asymbol **) xmalloc (symsize);
>
> Original bug description is here:
> WG: ld --auto-import for cygwin and libtool
> http://sources.redhat.com/ml/binutils/2001-06/msg00742.html
>
I have looked into the relating stuff and found, that for any section of any
input bfd a symbol tab is allocated.
For this I have two questions:

1. I recognized, that for any input file read by bfd one symboltab is present,
not one for any section. Isn't it 	?
If this is true (and it seems so, because I have applied a patch and have
compiled about 20 dll's with it), I have
found a way to solve this problem.  (moving symboltable allocating in the first
for loop)

(This patch contains some debugging stuff, which needs other files patch. It is
for dicsussion only)

pe-dll.c
   for (b = info->input_bfds; b; b = b->link_next)
     {
-      arelent **relocs;
-      int relsize, nrelocs, i;
+         int nsyms, symsize;
+         asymbol **symbols;

+         symsize = bfd_get_symtab_upper_bound (b);
+         symbols = (asymbol **) xmalloc (symsize);
+         nsyms = bfd_canonicalize_symtab (b, symbols);
+
       for (s = b->sections; s; s = s->next)
        {
-         asymbol **symbols;
-         int nsyms, symsize;
+      int relsize, nrelocs, i;
+           arelent **relocs;
          int flags = bfd_get_section_flags (b, s);

          /* Skip discarded linkonce sections */
          if (flags & SEC_LINK_ONCE
-             && s->output_section == bfd_abs_section_ptr)
+                     && s->output_section == bfd_abs_section_ptr) {
+                               if (pe_dll_extra_pe_debug & 4)
+                                       printf("\t section->name=%s -->
ignored\n",s->name);
            continue;
-
+                       }
          current_sec=s;

-         symsize = bfd_get_symtab_upper_bound (b);
-         symbols = (asymbol **) xmalloc (symsize);
-         nsyms = bfd_canonicalize_symtab (b, symbols);
-
          relsize = bfd_get_reloc_upper_bound (b, s);
          relocs = (arelent **) xmalloc ((size_t) relsize);
          nrelocs = bfd_canonicalize_reloc (b, s, relocs, symbols);

2. Relating to this bug, I have found that allocating symbol tables is done
several times in ld. Using a symbol
   cache would be save much memory.

			pe_find_data_imports ()
	ei386pe.c:         symsize = bfd_get_symtab_upper_bound (b);

			gld_i386pe_after_open ()
	ei386pe.c:         symsize = bfd_get_symtab_upper_bound (is->the_bfd);

			pe_walk_relocs_of_symbol (info, name, cb)
	pe-dll.c:         symsize = bfd_get_symtab_upper_bound (b);

			process_def_file (abfd, info)
	pe-dll.c:         symsize = bfd_get_symtab_upper_bound (b);

			generate_reloc (abfd, info)
	pe-dll.c:         symsize = bfd_get_symtab_upper_bound (b);

	ldcref.c:      symsize = bfd_get_symtab_upper_bound (abfd);
	ldmain.c:         symsize = bfd_get_symtab_upper_bound (abfd);
	ldmisc.c:                   symsize = bfd_get_symtab_upper_bound (abfd);

What about creating a symbol cache list in ld using pointer to loaded symbols
tables or by extending bfd structure with a pointer to a "symbol cache entry",
which can be filled by client code and later reused by other code.
Extending bfd would minimize the overhead to handle this cache, so I think this
would be the best way.
I have to say one limitation to this. Such cached symbol tables should not be
manipulated. They should be used only read only. Is this real ?

I you wish, I can look deeper into this.

Regards Ralf

Re[2]: Serious bug in auto-import

[Paul Sokolovsky]

Hello Charles,

Charles Wilson wrote on Sunday, September 09, 2001:

[]

CW> Yep.  Tested.  ld with Paul's patch accurately detects, prints out a
CW> warning message, and exits-with-error whenever "direct addressing" is

    Yep, if I didn't confused it and used right English term (I
learned that long ago in Russian). Well, for sure it must right. In
Intel's terms, it is "direct addressing with [non-zero] displacement"
(vs for example with indirect addressing when [part of] address is held
in register). I used term "offset" instead of "displacement" though
(Intel used to mark offset within 8086 64k segements).

[]

CW> Now, with structs, it's a little tricky.  Assume the following definition:
CW> typedef struct {
CW>    int a;
CW>    int b;
CW> } ST_;

CW> This is bad:
CW> extern ST_ st;
CW>    st.a = ...

       Well, in fact *this* exact code is good. It's possible to
access the very first member of struct ;-)

CW> This is also bad (but is an analogous form to the "array" version that
CW> works)...
CW> extern ST_ st;
CW> ST_ p;
CW>    p = st;
CW>    p.a = ...

       Here's the trick: it should be working in general (get the
address of st, memcpy()  it), but gcc optimizes 8-byte copy, into two
consecutive stores, hence problem.

       Nevermind. It will tell when it's bad, and what is needed is to
know how to workaround such case.

[]

CW> Of course, that's almost identical to what happens when you do this
CW> (which is also good, of course):
CW> extern __declspec(dllimport) ST_ st;
CW>    st.a = ...

CW> The good news is, Paul's patch will always detect, warn, and fail when
CW> the "bad" forms are used -- because it's triggered by the "bad" assember
CW> instructions directly, regardless of what C code produced it.  (That is,
CW> no weird list of special cases.)  The only question is, when a user gets
CW> Paul's error message, how will the user know HOW to fix the problem (in
CW> their client code)?  *That* requires a long list of special cases in
CW> some documentation somewhere...

    As you see, I tried to hammer in howto into error message ;-)
(yes, it hardly will make sense for outsider). I think, there should
be standard recommendation: mark offending symbol with
__declspec(dllimport) (and error message should state only this
alternative). If you care, in ld you can add more elaborated
discussion, along the lines of:

=====
The problem happens when some (sub)expression accessing address
ultimately given by the sum of two constants. The solution is to make
it use one variable instead. In case of array, we have two
possibilities: a) make indexee (array) variable, b) make index
variable, i.e.:

extent type extern_array[];

extern_array[1] => { volatile type *t=extern_array; t[1] }

or

extern_array[1] => { volatile int t=1; extern_array[t] }

For struct, the only option is to make struct itself variable:

extent struct s extern_struct;

extern_struct.field => { volatile struct s *t=extern_struct; t->field }

Which workaround (__declspec(dllimport) marking or rewriting rules
above) you should use? Obviously, __declspec(dllimport) marking,
unless you have the aim not to introduce system-dependent features to
your code (but note that alternative is to introduce obscure
constructs into your code, so you will need to comment why you did
this anyway). In short, consider typical real-world usage of both
methods and decide yourself:

Original :
--foo.h
extern int arr[];
--foo.c
#include "foo.h"
main()
{
  printf(arr[1]);
}
--

1:
--foo.h
/* Note: auto-export is assumed (no __declspec(dllexport)) */
#if defined(_WIN32) && !defined(FOO_BUILD)
#define FOO_IMPORT __declspec(dllimport)
#else
#define FOO_IMPORT
#endif
extern FOO_IMPORT int arr[];
--foo.c
#include "foo.h"
main()
{
  printf(arr[1]);
}
--

2:
--foo.h
extern int arr[];
--foo.c
#include "foo.h"
main()
{
  /* This stupid workaround is for win32 - do not "optimize" */
  volatile int *parr = arr;
  printf(parr[1]);
}
--


=====

[]

CW> Unless of course:

>> THEN we can think about a "real" fix for this array-index problem (if
>> possible; it may not be). :-)

   Well, my guess is that occurrences of the issue will be so rare,
that "real fix" will never pay for itself...


[]

CW> --Chuck

--
Paul Sokolovsky, IT Specialist
http://www.brainbench.com/transcript.jsp?pid=11135

Re: Serious bug in auto-import

[Charles Wilson]

Paul Sokolovsky wrote:

> Hello Charles,
> 
> Charles Wilson wrote on Sunday, September 09, 2001:
> 
> []
> 
> CW> Yep.  Tested.  ld with Paul's patch accurately detects, prints out a
> CW> warning message, and exits-with-error whenever "direct addressing" is
> 
>     Yep, if I didn't confused it and used right English term (I
> learned that long ago in Russian). Well, for sure it must right. In
> Intel's terms, it is "direct addressing with [non-zero] displacement"
> (vs for example with indirect addressing when [part of] address is held
> in register). I used term "offset" instead of "displacement" though
> (Intel used to mark offset within 8086 64k segements).


Your wording:

aggregate 'st' is referenced in direct addressing mode with constant 
offset - auto-import failed. Workarounds: a) mark the symbol with 
__declspec(dllimport); b) use auxiliary variable

Seems good to me.  I think both options should be mentioned (briefly, as 
you have done), because recommending only __declspec(dllimport) leads us 
*right* back to the nasty "on cygwin, if static...else if building 
dll...else if linking to dll..." garbage.  (more on this below).

> 
> CW> Now, with structs, it's a little tricky.  Assume the following definition:
> CW> typedef struct {
> CW>    int a;
> CW>    int b;
> CW> } ST_;
> 
> CW> This is bad:
> CW> extern ST_ st;
> CW>    st.a = ...
> 
>        Well, in fact *this* exact code is good. It's possible to
> access the very first member of struct ;-)


Yeah, I forgot that my test program had the .a and .b references 
transposed (next time, I should cut-n-paste, not retype by hand)


> 
> CW> This is also bad (but is an analogous form to the "array" version that
> CW> works)...
> CW> extern ST_ st;
> CW> ST_ p;
> CW>    p = st;
> CW>    p.a = ...
> 
>        Here's the trick: it should be working in general (get the
> address of st, memcpy()  it), but gcc optimizes 8-byte copy, into two
> consecutive stores, hence problem.
> 
>        Nevermind. It will tell when it's bad, and what is needed is to
> know how to workaround such case.


exactly my point.  A brief explanation in the error message (as you 
have), coupled with more explanation in the .info file.  I'll try to 
work up a patch for ld.texinfo tonight.

>     As you see, I tried to hammer in howto into error message ;-)
> (yes, it hardly will make sense for outsider). I think, there should
> be standard recommendation: mark offending symbol with
> __declspec(dllimport) (and error message should state only this
> alternative).


I disagree.  While the auto-export option means that you don't need to 
specify __declspec(dllexport), you still need to distinguish (on cygwin) 
between "I'm building client code that I will link to the DLL" vs. "I'm 
building client code that will link to the static lib"

This means that you can't just decide at link time to say "ld 
-Bdynamic"(default gcc behavior) or "ld -Bstatic"(gcc -static).  You 
ALSO have to -DFOO_DLL_LINK or -UFOO_DLL_LINK (conversely, 
-DFOO_STATIC_LINK and -UFOO_STATIC_LINK) when compiling, so that the 
decoration is #defined appropriately.  But, you only have to do that if 
your package accesses "aggregates" from its dependencies, if it uses 
direct accessing with constant offset....

Blech.  This gets really messy.  Joe Schmoe ought to be able to build a 
client package without knowing those internal details of the library. 
And convincing the maintainers of that client code to uglify their 
headers with all those "extern STUPID_CYGWIN_DECORATOR int foo" is a 
real challenge -- trust me, I've tried.

Therefore, I *much* prefer recommending the second of your alternatives 
-- I wasn't aware that volatile would "fix" it for us.  (And you can use 
volatile for ALL THREE cases on cygwin; static, DLL(build), and 
DLL(linkto) -- I think. I will test this tonight.).  One question, 
though:  you say:

 > /* This stupid workaround is for win32 - do not "optimize" */
 > volatile int *parr = arr;

Does your comment mean
   "Future programmer editing this client code, do not replace 
references to 'parr' with 'arr'."
or
   "When compiling this source code, do not specify -O{n}"
or both?

Will -O2 override an explicit "volatile" declaration?  I didn't think it 
would do that...I will verify.

Therefore, ld's error message should at least HINT at the "use a local 
variable" possibility.  Your original wording does that.  Then, more 
detailed stuff would go in the .info file (I'll base my ld.texinfo patch 
on your explanation below).

> If you care, in ld you can add more elaborated
> discussion, along the lines of:
> 
> =====
> The problem happens when some (sub)expression accessing address
> ultimately given by the sum of two constants. The solution is to make
> it use one variable instead. In case of array, we have two
> possibilities: a) make indexee (array) variable, b) make index
> variable, i.e.:
> 
> extent type extern_array[];
> 
> extern_array[1] => { volatile type *t=extern_array; t[1] }
> 
> or
> 
> extern_array[1] => { volatile int t=1; extern_array[t] }
> 
> For struct, the only option is to make struct itself variable:
> 
> extent struct s extern_struct;
> 
> extern_struct.field => { volatile struct s *t=extern_struct; t->field }
> 
> Which workaround (__declspec(dllimport) marking or rewriting rules
> above) you should use? Obviously, __declspec(dllimport) marking,
> unless you have the aim not to introduce system-dependent features to
> your code (but note that alternative is to introduce obscure
> constructs into your code, so you will need to comment why you did
> this anyway). In short, consider typical real-world usage of both
> methods and decide yourself:
> 
> Original :
> --foo.h
> extern int arr[];
> --foo.c
> #include "foo.h"
> main()
> {
>   printf(arr[1]);
> }
> --
> 
> 1:
> --foo.h
> /* Note: auto-export is assumed (no __declspec(dllexport)) */
> #if defined(_WIN32) && !defined(FOO_BUILD)
> #define FOO_IMPORT __declspec(dllimport)
> #else
> #define FOO_IMPORT
> #endif
> extern FOO_IMPORT int arr[];
> --foo.c
> #include "foo.h"
> main()
> {
>   printf(arr[1]);
> }
> --
> 
> 2:
> --foo.h
> extern int arr[];
> --foo.c
> #include "foo.h"
> main()
> {
>   /* This stupid workaround is for win32 - do not "optimize" */
>   volatile int *parr = arr;
>   printf(parr[1]);
> }
> --
> 
> 
> =====
> 
> []
> 
> CW> Unless of course:
> 
> 
>>>THEN we can think about a "real" fix for this array-index problem (if
>>>possible; it may not be). :-)
>>>
> 
>    Well, my guess is that occurrences of the issue will be so rare,
> that "real fix" will never pay for itself...


Actually, given the "volatile" fix, I like it.  This usage seems to be 
pretty uncommon, so the bug (and error message) will rarely be hit. 
Recommending "volatile" markers means when only need to #ifdef on 
__CYGWIN__ , not a bunch of FOO_STATIC/FOO_DLL_BUILD/FOO_DLL_LINKTO 
variables.

Getting rid of THAT garbage was my primary goal in advocating your 
auto-import stuff in the first place!

So: we have a rarely triggered bug.  Your patch warns the user and 
prevents buggy code from being generated. My forthcoming documentation 
patch (based on your stuff, above) will explain how a user hit by this 
bug can work around it.  And we still don't need 
FOO_STATIC/FOO_DLL_BUILD/etc.

I'm happy.  Now, about that memory leak.... :-)

--Chuck

Re: Serious bug in auto-import

[Charles Wilson]

Charles Wilson wrote:


> 
> Therefore, I *much* prefer recommending the second of your alternatives 
> -- I wasn't aware that volatile would "fix" it for us. 


To clarify, 'volatile' is not a magic bullet.  It just prevents gcc from 
optimizing our workaround out of existence.  In the absence of 
optimization, this works just as well:
   extern_array[1] --> { type *t=extern_array; t[1] }
   extern_struct.field --> { struct s *t=&extern_struct; t->field }

> (And you can use 
> volatile for ALL THREE cases on cygwin; static, DLL(build), and 
> DLL(linkto) -- I think. I will test this tonight.).  


Yep.  Works fine.  (I know, I know, should be obvious -- but at this 
point, I'm in the "trust nothing unless you've tested it" mode.)

> One question, 
> though:  you say:
> 
>  > /* This stupid workaround is for win32 - do not "optimize" */
>  > volatile int *parr = arr;
> 
> Does your comment mean
>   "Future programmer editing this client code, do not replace references 
> to 'parr' with 'arr'."


After testing with various optimization settings, it is obvious you mean 
this ^^^^

> or
>   "When compiling this source code, do not specify -O{n}"


and not this ^^^^^^^^^

> Therefore, ld's error message should at least HINT at the "use a local 
> variable" possibility.  Your original wording does that.  Then, more 
> detailed stuff would go in the .info file (I'll base my ld.texinfo patch 
> on your explanation below).


In the patch I will soon be posting, I've reversed the order of the two 
solutions, to present the 'auxilliary variable' first, the __declspec() 
second.

 
> 
> Actually, given the "volatile" fix, I like it.  This usage seems to be 
> pretty uncommon, so the bug (and error message) will rarely be hit. 
> Recommending "volatile" markers means when only need to #ifdef on 
> __CYGWIN__ , not a bunch of FOO_STATIC/FOO_DLL_BUILD/FOO_DLL_LINKTO 
> variables.
> 
> Getting rid of THAT garbage was my primary goal in advocating your 
> auto-import stuff in the first place!
> 
> So: we have a rarely triggered bug.  Your patch warns the user and 
> prevents buggy code from being generated. My forthcoming documentation 
> patch (based on your stuff, above) will explain how a user hit by this 
> bug can work around it.  And we still don't need 
> FOO_STATIC/FOO_DLL_BUILD/etc.
> 
> I'm happy.


Okay, combined patch (with ld.texinfo stuff) coming up...

--Chuck