Re: bfd function to read ELF file image from memory

[Nick Clifton <nickc at redhat dot com>]

Hi Roland,

>> Well since your function is creating a new bfd, one obvious place for
>> it would be in opncls.c.  Have you considered using an interface like
>> the one provided by BFD_IN_MEMORY ?  You could make the in-memory
>> interface generic and provide different memory accessing functions
>> (for local memory, remote memory, mmap'ed files, etc).
>>
>> I do not think that treating this in-memory bfd as a new target is
>> appropriate, since it is not really a new file format or instruction
>> set architecture, but a new method of loading (part of) a binary file
>> into a normal bfd structure.
>
> I think you have misunderstood what the code does, or else I am just
> very confused about what your comments mean.  The terminology in my
> remarks about the code was surely sloppy, since I was mostly
> expecting folks to look at the code itself.
>
> This is a new function that bfd_malloc's a buffer, fills in its contents,
> and makes that into a bfd with BFD_IN_MEMORY set.  It has nothing to do
> with adding a new bfd backend.

Agreed - hence treating it as a new target would be inappropriate.

> The function's implementation is format-specific, but its calling
> interface is generic.  So it seems like it ought to be accessed
> through a new function pointer in struct bfd_target, which we would
> at least so far only have implemented in the ELF backend.

Right - my theory was that you could extend and clean up the current
BFD_IN_MEMORY implementation so that the memory was either local (as
it currently is), remote (as your patch implements) or even an mmap'ed
file (something that people have been asking for in bfd for ages, and
is partially implemented in bfdwin.c).

This would allow for lazy reading in of remote contents (ie only when
they are needed) and, in theory, would mean that it could be used by
almost any bfd backend. Any remote-memory-bfd would just appear to the
rest of the bfd code as a normal bfd structure, and when one of the
standard bfd functions calls was used to read some of the contents, a
remote fetch would be performed and that region would then be marked
as being held in local memory.

Which is probably more than you wanted to implement, but it seemed
like the right way to handle remote memory reading, at least in the
long run.  Your patch as it stands, looks fine to me, although I would
probably rename the functions to ..._create_bfd_from_remote_memory().
I would also recommend adding a couple of prototypes to elf-bfd.h.


>> I am all for keeping things simple, so if it is easier to put it in
>> elfcode.h then that is probably where it should live.  Given the
>> probably limited utility of this function I feel that it ought to
>> be only conditionally defined, based on the particular configured
>> target or maybe a configure time switch.
>
> I am not aware of functions in libbfd that are conditionalized this
> way.  Can you point me to an example that would be a model of what
> you are suggesting?

Certainly.  If you have the code in a separate file, then you could do
the same thing as the peXXigen.c file, which is conditionally compiled
and linked into the bfd library based on the configuration target.
(See bfd/configure.in).  Alternatively you could use a system like the
definition of COFF_WITH_PE, which is defined in a variety of different
source files (eg bfd/pe-arm.c) and then tested for in various other
files (eg bfd/coffcode.h).


> I guess I can figure out what compile-time check to use in gdb
> for an ELF target and call bfd_elf_bfd_from_memory from libbfd under
> that #ifdef, but it doesn't seem very BFDish.

Hmm, I take your point - it would be easier if the function were
always defined in the bfd library, even if it was not used/not
intended to be used by a particular bfd target.  (I think it would be
OK if it were only defined in ELF targeted versions of the library
though).

Given that the code is not target specific itself, (just the
availability of the ELF headers/file contents in remote memory), I
withdraw my suggestion that the code be conditionalised.

  
> Still, I would just to have one calling interface that handles
> 32-bit and 64-bit ELF.  To implement that front-end function I need
> a compile-time check I can use in elfcode.h that tells me whether
> 32-bit and/or 64-bit targets are being built.  What works?

You can check "#if ARCH_SIZE == 64" or "#if ARCH_SIZE == 32".


> It so happens that I rewrote the function in the fashion I was
> describing.  So here's the new version.  This one should be robustly
> generally useful to extract the runtime-loaded portions of any ELF
> DSO from inferior memory.  Please take a look at the code.

> +/* Create a new BFD as if by bfd_openr.  Rather than opening a file,
> +   reconstruct an ELF file by reading the segments out of remote memory
> +   based on the ELF file header at EHDR_VMA and the ELF program headers it
> +   points to.  The function TARGET_READ_MEMORY is called to read remote
> +   memory regions by VMA.  TEMPL must be a BFD for a target with the word
> +   size and byte order found in the remote memory.  */

I would recommend including a description of how TARGET_READ_MEMORY is
supposed to work - what parameters it takes, what values it returns,
whether it is possible for it to timeout, etc.

Also - please document the purpose of the LOADBASEP parameter.


> +  Elf_External_Ehdr x_ehdr;	/* Elf file header, external form */
> +  Elf_Internal_Ehdr ehdr;	/* Elf file header, internal form */
> +  Elf_External_Phdr *x_phdrs;
> +  Elf_Internal_Phdr *i_phdrs;

I think that the choice of name of the name for the internal Ehdr is
slightly confusing.  I would suggest that you change it to "i_ehdr",


> +  contents_size = ehdr.e_phoff + ehdr.e_phnum;
> +  if ((bfd_vma) contents_size < ehdr.e_ehsize)
> +    {
> +      bfd_set_error (bfd_error_wrong_format);
> +      return NULL;
> +    }

This bit of code has me confused.  I assume that 'contents_size' is
being used to reserve space in the (to be allocated) 'contents'
buffer for the ELF header, but why are e_phoff and e_phnum being
used ? 


> +  /* Back up to the last PT_LOAD header.  */
> +  do
> +    --i;
> +  while (i > 0 && i_phdrs[i].p_type != PT_LOAD);

Couldn't you just remember the value of 'i' each time you encounter a
PT_LOAD section in the for() loop, so saving yourself this do...while
loop ?  It would also be a good idea to check that there actually *is*
a PT_LOAD section.


> +  /* Now we know the size of the whole image we want read in.  */
> +  contents = (char *) bfd_zmalloc ((bfd_size_type) contents_size);

Hmmm, what about files with disjoint segments ?  ie files which load
some segments to low addresses and some to high addresses ?  With the
current code 'contents' will be some huge number and so this malloc
will fail.  Wouldn't it be better to load each segment contiguously
into the 'contents' buffer and adjust the p_vaddr values in the
program header appropriately ?

Cheers
        Nick