This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]