[llvm-commits] RuntimeDyld RelocationResolver

[Jim Grosbach]

On Oct 11, 2011, at 2:29 PM, Danil Malyshev wrote:

> Hello Jim,
> 
> Thank you for review. The changed patch is attached.
> 
> 
>> Target names for classes like this are generally done as a prefix, not 
>> a suffix. It would be better to do that here for consistency.
> 
> Fixed.
> 
> 
>> x86 vs. x86_64?
> 
> Both. I have added the explanation in the attached patch.
> 
> 
>> This seems odd? Placeholder? If so, a FIXME to that effect would be good.
> 
> Comment added.
> 
> 
>> LLVM doesn't generally use right-justified comments like this. Not a 
>> big deal, but it looks a bit odd in context.
> 
> Fixed.
> 
> 
>> For now, though, it'd be good to split the target bits into separate 
>> files rather than keeping it all together in one.
> 
> Fixed.
> 
> 
>> This seems odd. At minimum, copious comments are needed explaining 
>> what's going on here.
> 
> Comment added.
> 
> 
>> We shouldn't need to do by-name lookup here. Mapping names should have 
>> been handled by the loader when the symbol table was processed.
>> Everything at this level should be able to use symbol table indices.
> 
> Fixed.
> 
> 
>> No braces when there's just one statement.
> 
> Fixed.
> 
> 
>> Not all relocations are on functions. How are global values handled, 
>> for example?
> 
> This part of code runs only for branch relocations and will not be executed for global values.
> 
> 
>> Is this target memory? The local memory where our copy is stored? In 
>> any case, addr/length is probably better than start/end so we can 
>> handle symbols w/o any data associated (aliases).
> 
> I have added the explanation in the attached patch.
> 
> 
>> Relocations are inherently target and platform specific. ARM MachO vs. 
>> ARM ELF are very different, for example. This appears to be trying for 
>> a one-size-fits-all solution. I don't think that's going to work out.
>> This is the biggest concern I have with this patch. The relocations 
>> should use the definitions in the Object and ELF/MachO file format 
>> headers and handle them distinctly.
> 
> 
> You are right. Originally, relocations are inherently target and platform specific.
> 
> Though, emitter emits the relocations to the target memory on some unified manner, so once in the target memory they are target specific only (i.e. ARM specific, since ARM MachO vs. ARM ELF differences are handled).
> 

The code itself looks better. Thanks for the changes. I disagree fairly strongly with this approach, however. Relocations are target and platform specific. They should be handled as such directly rather than trying to map to a super-set representation that tries to do everything. Can you elaborate on why you find this approach compelling, perhaps?

-Jim


> Relocation resolver dials with already emitted relocations, and depdends on target only because of that.
> 
> 
> 
> Regards,
> Danil
> <RuntimeDyld_RelocationResolver-02.patch>