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

Joel Brobecker
2012-11-05T17:11:42Z

> Oh boy, you scared me there... But definitively, no, I didn't
> commit this change yet!

Sorry about the scare. I was looking at the code, and saw identical
code, so I figured it got checked in. That's perhaps a clue for us
to do more code factoring...

> I still didn't get a correct way of checking my formatting...

No problemo, I don't think it's a major issue.

> > > + printf ("%s is a forward to %s\n", funcname, forward_name);
> >
> > I don't think a printf is appropriate, here. Is that meant to be
> > a warning?
>
> I would have like any suggestion about an idea about how to
> implement such a forward... Something like a function called
> add_pe_forward_symbol Would it require the definition of a new
> minimal symbol type? If yes, I wouldn't even know where I should
> add this new type...

Are you saying, remove the need for the printf/warning by implementing
the missing feature? If complicated, I'd start with the
warning/complaint first, get that checked in, and then think about
how to lift the limitation.

> I basically reused the existing code in the function called
> read_pe_exported_syms, and, specifically, I kept most of the local
> variables and used the same names...

Let's keep it consistent for now, then...

> > > + unsigned char *expdata, *erva;
> >
> > Should we be using gdb_byte *, in this case? I'm wondering if we
> > should be adjusting the pe_get* & pe_as* routines as well...
>
> The problem is that it is used both for 'char *'
> for all the names and for RVA (relative virtual addresses)
> which are more something like a 32-bit unsigned offset.

I'm not opposed to keeping the consistency, but I think we're going
to have to re-evaluate the use of "char *" vs "gdb_byte *" at some
point. I think that if you have a buffer of bytes, it's better to
use gdb_bytes, as the type was created to deal with this sort of
data. Usually, you can't rely on what the buffer contains, because
host and target can be different.

> > > + if (!is_pe32 && !is_pe64)
> > > + {
> > > + /* This is not a recognized PE format file. Abort now, because
> > > + the code is untested on anything else. *FIXME* test on
> > > + further architectures and loosen or remove this test. */
> > > + return 0;
> > > + }
> >
> > I think a complaint would be appropriate, here. And I'm wondering
> > if there might be a better way to check which PE format it is other
> > than looking at a string...
>
> Again, this is a plain copy of the code in read_pe_exported_syms.

OK, you can keep it that way, at least for now. "plain copy" -> factorize?

--
Joel