Re: [PATCH v3] Add dll trampoline code handling for windows 64bit

[Roland Schwingel <roland@onevision.com>]

Hi Joel,

gdb-patches-owner@sourceware.org wrote on 29.03.2012 20:21:02:
 > > 2012-03-16  Roland Schwingel <roland.schwingel@onevision.com>
 > >
 > >         * amd64-windows-tdep.c: #include "frame.h".
 > >         (amd64_windows_skip_trampoline_code): New function.
 > >         (amd64_windows_init_abi): Add trampoline registration.
 >
 > No one reviewed þhe patch as far as I can tell, I am sorry about that.
 > As a general guideline, it's OK to ping us every week or two...
Thanks for taking the time and looking at it.

 > I think the code looks pretty good, and you can commit, with a couple
 > of very minor almost-nitpicky little comments (a apologize for asking
 > for your forgiveness)...
No problem.

 > Hmmm, I actually cannot find any copyright assignment for you on
 > file. Do you have one? Or is this done on behalf of your employer
 > who might have one? Please let me know... We can give you write-
 > after-approval priviledges as soon as we have confirmed that you
 > do have an assignment on file.
Tom Tromey sent me the necessary informations on march 15th. One day 
later I sent out the mail to fsf-records@gnu.org but yet have not 
received *any* response. I asked Tom some days ago and he told me to 
wait for some more days before I send in a ping. I already got a CVS 
account on sourceware and I am just awaiting my okays ...

 > > +      /* Get address of function pointer at end of pc.  */
 > > +      CORE_ADDR indirect_addr = pc + offset + 6;
 >
 > I think it would be useful to explain where the magic constant "6"
 > comes from... I'd almost write the expression "pc + 6 + offset".
PC relative instructions always count starting from the end of the 
instruction itself. pc in this situation points to the beginning of the 
instruction. The instruction itself is (with operands) 6 bytes long. If 
you like to I will change the order of addition on commit.

 > > +      struct minimal_symbol *indsym =
 > > +        indirect_addr ? lookup_minimal_symbol_by_pc 
(indirect_addr) : 0;
 > > +      const char *symname = indsym ? SYMBOL_LINKAGE_NAME (indsym) : 0;
 >
 > I'd rather you used NULL instead of 0, even if we're pretty much
 > guaranteed that NULL will always be zero. That's really nit-picky,
 > but it seems clearer that way, at least for me.
Sure I can do that (on commit). I did it the same way as the 32bit code 
in i386_pe_skip_trampoline_code() does hoping it would be easier for the 
approver if he sees familiar code ...

The other 2 formatting I will also correct on commit. But if you want I 
am doing a 4th version of my patch with your suggested changes and send 
it in. Elsewise I will sit and wait until my assignment stuff gets done 
and commit it later on and avoid unneccessary noise.

Roland