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

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

I haven't actually *built* the code with the changes below; I wanted to 
get this email off, start a build, and go to bed.  We'll see if I made 
no stupid syntax errors in the morning.

DJ Delorie wrote:

 > You should only need to do the cindex for the first of each set, yes?

Okay.  Fixed.

 >> +typedef struct { +  char *name; +  int len; +}
 >> autofilter_entry_type;
 >>
 >
 > I wonder if we don't need the length, because it can be computed on the
 >  fly?  Not a big deal, though - it would only really help maintainers.
 >

no, this saves execution time rather than calling strlen OVER and OVER
on the same constant strings.  We could compute it once and then cache
those values, but geez, that's ugly.


 >> +#if 0 +      /* Don't export any 'reserved' symbols */ +      if
 >> (*n && *n == '_' && n[1] == '_') 	return 0; +#endif
 >>
 >
 > Rather than leave in commented out code, you should simply add a textual
 >  comment explaining what to avoid (if such a comment is warranted)
 > or simply remove the code.  Not a big deal, though.


Ok. Replaced with textual comment.


 >
 >> +char * +make_import_fixup_mark (rel)
 >>
 >
 > This should be static, or be named pe_*


static.  Done.  Also, there really was no reason for add_bfd_to_link to
be made non-static, so I reverted it to static.


 >> +  static char fixup_name[300];
 >>
 >
 > 300 is a wonderful number, but dynamically allocating space is
 > better. Compromise - keep a static pointer to a malloc'd buffer
 > that grows as needed.


Okay.  See code snippet below.


 >> +  if (pe_dll_extra_pe_debug)  +    { +      printf
 >> (__FUNCTION__"\n"); +    }
 >>
 >
 > Hmmm... what happens when the function name has a percent in it?
 > (not likely in C, but I'm the paranoid type...)


Is printf ("%s\n", __FUNCTION__); okay ?  If so, done.


 >> +char *data_import_dll;
 >>
 >
 > Should this be static?

No, it's used in both pe.em and pe-dll.c.  Although, I'm not really sure 
why.  Paul? Robert?  If it really needs to be public, I suppose we 
should rename it pe_data_import_dll.


Code snippet for fixup_name buffer dynamic sizing:


----------------
   static char *fixup_name = NULL;
   static int buffer_len = 0;
   int ret;

   if (!fixup_name)
     {
       fixup_name = (char *) xmalloc (300);
       buffer_len = 300;
     }

   struct symbol_cache_entry *sym = *rel->sym_ptr_ptr;

   bfd *abfd = bfd_asymbol_bfd (sym);
   struct coff_link_hash_entry *myh = NULL;

   ret = snprintf (fixup_name, buffer_len, "__fu%d_%s",
                   counter++, sym->name);
   if (ret < 0)
     {
       free (fixup_name);
       fixup_name = (char *) xmalloc (buffer_len + strlen (sym->name));
       /* Surely 300 + strlen will be big enough for this symbol.  If
          counter is more than 295 digits long, we have other problems... */
       buffer_len += strlen (sym->name);
       ret = snprintf (fixup_name, buffer_len, "__fu%d_%s",
                       counter++, sym->name);
     }

   ...

----------------------

--Chuck