[llvm-commits] [PATCH] Review request - MCJIT runtime loader improvements

[Eric Christopher]

The patches seem reasonable (especially since you've fixed the 32-bit issue as well). One thought is that we may want to load debug sections eventually so an option or some sort to load more sections might be a good idea.

Please remove the include of iostream. :)

Otherwise thanks :)

-eric

On Apr 9, 2012, at 1:38 PM, "Kaylor, Andrew" <andrew.kaylor at intel.com> wrote:

> No problem.  Here are the patches.  Here was my accompanying description:
> 
> ----------
> Here is the first incremental chunk of the patch I submitted a few days ago (now based on the current trunk code!).
> 
> I have separated the patch into two files.  The first file contains the actual implementation details.  The second file makes a minor change to existing test files to eliminate 'XFAIL' lines for things that should now pass.
> 
> The primary patch implements new handling of zero-initialized sections, virtual sections and common symbols and prevents the loading of sections which are not required for execution such as debug information.  By "loading" here I mean the process of allocating memory for the section, copying its contents into that memory and performing applicable relocations.  These improvements required the addition of new functions in the ObjectFile class and its subclasses to test various attributes of a section.  Because I am not familiar with all the details of the MachO and COFF formats, I have used default implementations in some places.  This default code should produce correct results, but it may not be the most optimized behavior possible.  I believe I am correctly handling common symbols for both ELF and MachO formats.
> 
> I've tested this code on 32- and 64-bit Linux systems and on a 64-bit Mac system.  In my testing, all execution tests pass with the MCJIT engine.
> ----------
> 
> Danil is seeing problems with 32-bit MCJIT, which I'm currently investigating.
> 
> -Andy
> 
> -----Original Message-----
> From: Eric Christopher [mailto:echristo at apple.com] 
> Sent: Monday, April 09, 2012 1:32 PM
> To: Kaylor, Andrew
> Cc: Jim Grosbach
> Subject: Re: [PATCH] Review request - MCJIT runtime loader improvements
> 
> Must've missed it. Want to resend to Jim and I?
> 
> -eric
> 
> 
> On Apr 9, 2012, at 1:31 PM, "Kaylor, Andrew" <andrew.kaylor at intel.com> wrote:
> 
>> I did split it up.
>> 
>> The patch I've got out there now has just the runtime improvements.  A lot of the diffs are the result of blocks of code being moved inside an 'if' statement.  I don't think this set of changes is overly large.
>> 
>> -Andy
>> 
>> -----Original Message-----
>> From: Eric Christopher [mailto:echristo at apple.com] 
>> Sent: Monday, April 09, 2012 1:24 PM
>> To: Kaylor, Andrew
>> Cc: Jim Grosbach
>> Subject: Re: [PATCH] Review request - MCJIT runtime loader improvements
>> 
>> I'd like one of us to review it. I thought you were going to split it up for us? At least a little?
>> 
>> -eric
>> 
>> On Apr 9, 2012, at 1:16 PM, "Kaylor, Andrew" <andrew.kaylor at intel.com> wrote:
>> 
>>> Hi Jim and Eric,
>>> 
>>> Is this sufficient review for a commit, or do one of you still want to review my patch?
>>> 
>>> Thanks,
>>> Andy
>>> 
>>> 
>>> From: Danil Malyshev [mailto:dmalyshev at accesssoftek.com] 
>>> Sent: Wednesday, April 04, 2012 6:20 PM
>>> To: Kaylor, Andrew; llvm-commits at cs.uiuc.edu
>>> Subject: RE: [PATCH] Review request - MCJIT runtime loader improvements
>>> 
>>> Hi Andrew,
>>> 
>>> It looks good for me, except using of "if ((bool)(err = ..." instead of "Check()", thank you!
>>> But too fast growing number of isSectionXXX() in SectionRef. It would be better convert it to getFlags() as it was done in SymbolRef.
>>> 
>>> 
>>> Regards,
>>> Danil
>> 
> 
> <01-runtimedyld-improvments.patch><02-runtimedyld-tests.patch>