[llvm-commits] MachOObjectFile fix functions

[Owen Anderson]

Danil,

On Nov 7, 2011, at 2:40 PM, Danil Malyshev wrote: 
> Please find attached the changed patch which addresses the following 2 things:
> 1. Functions in ELFObjectFile, MachOObjectFile and COFFObjectFile should returns same result.

This is fine in general, but we need to make sure we're standardizing on the right behaviors.

> 2. We don't need have several functions with similar functionality which can be obtained by other means.

I have many more issues with this one.  Comments inline.

> getSymbolAddress - removed.

I don't like this.  Getting symbol virtual addresses is one of the most common operations we do in, say, llvm-objdump.cpp or MachODump.cpp.  I think this will make that code needlessly complex and non-obvious.

> getRelocationAddress - removed.

This can't be removed.  MachO scattered relocations relocate on a virtual address, rather than a symbol, so getSymbol().getAddress() doesn't work for them.

> getSymbolOffset - returns the offset from the beginning of the section.

I don't particularly mind standardizing on this as the definition for "getOffset" methods, but it doesn't seem particularly useful to me either.  Why require a two-stage lookup (section offset + symbol offset from section) rather than just defining it to return a file offset?

> If we need the virtual address we can use something like symbol.getOffset() + symbol.getSection().getAddress()

See my above comments about how common this lookup is and why I don't think we should make it more complex.
 
> If we need the pointer to the first byte of symbol we can use something like getOffset() + getSection().getSectionContents().begin()
> getRelocationOffset - returns the offset from the beginning of the section.
> If we need the virtual address we can use somethings like reloc.getOffset() + reloc.getSection().getAddress()

This does not match my understanding of what getRelocationAddress() computes.  My understanding is that it returns the virtual address of the value being relocated upon, in which case this is not the same as what you are proposing.  See my comments above about why this value is necessary on MachO rather than just using getSymbol().getAddress().

--Owen
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20111107/c3d85df4/attachment.html>