Re: [RFC-v4] Fix .text section offset for windows DLL (was Calling __stdcall functions in the inferior)

[Joel Brobecker <brobecker at adacore dot com>]

> I used alloca here...  And tried  to use if for the qualified_name
> too, but that turned out to be a bad idea...
>
> The name was apparently still accessed after the function returned,
> despite the fact that the symbol_name is copied...  (This is probably
> in the hash table, but I didn't completely understand how that
> works...)

I think you might have unearthed a bug, here. This is the code you
propose:

    +  qualified_name = xstrprintf ("%s!%s", dll_name, sym_name);
    +  prim_record_minimal_symbol (qualified_name, vma, msymtype, objfile);

Following the code:

   prim_record_minimal_symbol
   -> prim_record_minimal_symbol_full with copy_name=1
   -> SYMBOL_SET_NAMES (msymbol, name, name_len, copy_name, objfile);

And symbol_set_names does:

|  else
|    {
|      lookup_len = len;
|      lookup_name = linkage_name;
|      linkage_name_copy = linkage_name;
|    }
|
|  entry.mangled = (char *) lookup_name;
|  slot = ((struct demangled_name_entry **)
|          htab_find_slot (objfile->demangled_names_hash,
|                          &entry, INSERT));

Does it looks like a reference to the string might actually
get inserted in the htab if not found, thus defeating the
mechanism above? Tom might be better suited to answer that
question more authoritatively.

In the meantime, one approach that might be equally suitable, but
without the bug, is if allocate the name on the objfile obstack,
and then call prim_record_minimal_symbol_full directly (I am
assuming that objfile can never be null, which seems to be the
case by my reading of your patch).

> > Are we missing a cleanup/xfree?
> 
> I added some, please check that part, as I have no experience at all
> with using make_cleanup  related functions...  In particular, I didn't
> really get if it is OK to call do_cleanups with a possibly NULL
> argument...

It seems like you're going to have to make some other changes to
address the crash that was reported, and Pedro sent you a link to
the documentation. So I'll wait for that version before reviewing.
Hope that's OK.

> > > +  section_data[PE_SECTION_INDEX_DATA].section_name = ".data";
> > > +  section_data[PE_SECTION_INDEX_BSS].ms_type = mst_bss;
> > > +  section_data[PE_SECTION_INDEX_BSS].section_name = ".bss";
> > 
> > Also, I think it makes it harder to determine what the contents of the
> > table is. I suggest you go back to the static definition above, but
> > updated with the extra field.
> 
>   Sorry, but here my C knowledge is not sufficent:
> section_data is now a pointer allocated on heap,
> how can I reconcile this with the old static definition?

I cannot remember what I was thinking (or inhaling) at the time.
Probably something good (thinking or inhaling), but now gone.
Suggestion withdrawn...

> > Can you make prim_record_minimal_symbol sym_name parameter a "const",
> > and then declare...
> 
>   I hope you meant changing add_pe_exported_sym only...
> I don't want to touch prim_record_minimal_symbol function which is also
> used in other C sources...
>  
>   I removed the null_char by handling NULL also in add_pe_exportd_sym
> instead of only supporting a pointer to '\0'.

Yes, I meant add_pe_exported_sym, nowing that the only potential issue
was with the call to prim_record_minimal_symbol - but not an issue
since the name parameter is already a const.

Your solution is good, although making the sym_name parameter a const
is also an improvement.

-- 
Joel