[llvm-commits] ELFObject

[Jason Kim]

Hi Dani,


On Fri, Aug 5, 2011 at 11:58 AM, Danil Malyshev
<dmalyshev at accesssoftek.com>wrote:

>  Hello,****
>
> ** **
>
> > 1. Which architecture is to be supported first?****
>
> ** **
>
> Currently I'm working on the armv7 support.****
>
> **
>

Great!



>  **
>
> > 2. How will you split off architecture specific details, such as ..****
>
> > 3. Relocations during JIT?****
>
> ** **
>
> This class provides an access to symbol and relocation data, which is the
> same for all architectures.****
>
> Architecture specific details will be handled by the dynamic linker.****
>
> ** **
>
> > Is it intended to ever support 64 bit elf files directly? If so, you'll
> have to do something about the 32bit offset helpers below (for starters)
> and/or the class name (for clarity) If not, then should this class be called
> ELF32Object?****
>
> ** **
>
> I have changed the proposed class to be a template with 2 instances:
> ELF32Object and ELF64Object.****
>
> Please find the patch attached.****
>
> **
>

Overall, it looks pretty good.

I'd feel better if there was at least some unification between ELFObject and
ELFObjectFile -- if only for the representation of the ELF blob's layout.
I don't see why we need to have a total bifurcation here. Whether its a file
or an in-memory object, the representation is the same, right? The latter
should have the file-io specific routines that the in-memory does not
need....

Some specific nits:

The getOffset() routine is misnamed - perhaps it should be getData()?
(consolation: it agrees with the existing MachOObject.h  :-)

Also, it still uses a 32bit offset field even in the 64 bit case - can you
please fix this somehow? (Perhaps look at ELFObjectFile.cpp for
inspiration?)

Thanks!



> **
>
> ** **
>
> ** **
>
> Regards,****
>
> Danil****
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20110805/db7f8880/attachment.html>