[llvm-commits] [PATCH] unit tests for MCJIT

[Jim Grosbach]

On Sep 25, 2012, at 8:22 AM, "Malea, Daniel" <daniel.malea at intel.com> wrote:

> On 2012-09-24, at 5:51 PM, Jim Grosbach wrote:
> 
> 
> On Sep 24, 2012, at 2:20 PM, "Du Toit, Stefanus" <stefanus.du.toit at intel.com<mailto:stefanus.du.toit at intel.com>> wrote:
> 
> -----Original Message-----
> From: llvm-commits-bounces at cs.uiuc.edu<mailto:llvm-commits-bounces at cs.uiuc.edu> [mailto:llvm-commits-
> bounces at cs.uiuc.edu] On Behalf Of Jim Grosbach
> Sent: Monday, September 24, 2012 1:38 PM
> To: Malea, Daniel
> Cc: llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>
> Subject: Re: [llvm-commits] [PATCH] unit tests for MCJIT
> 
> The most substantive comment is that we shouldn't move the memory
> manager out of lli.cpp. That's very much not a general purpose memory
> manager, but rather is intended as a (trivial) example of what a client
> application may need to implement in order to utilize the MCJIT. It's not
> part of a general LLVM library, but rather part of the lli tool.
> 
> What's the downside of moving the memory manager out of lli? If it's sufficient for a simple client (like lli's) needs, why not have it be reusable?
> 
> My understanding is that the tests do need *some* memory manager implementation. If it's not moved here, wouldn't it need to be included from inside of the lli sources (awkward) or duplicated somewhere in the test sources (even more awkward)?
> 
> It's a division of responsibility and layering thing. The MCJIT library defines the interface to the memory manager, but that's it. The client is responsible for supplying an implementation. We want the implementation to make this distinction very starkly at least for now, and moving the memory manager into the library rather than part of lli obfuscates that purpose. It's possible we'll create another library later that supplies, for example, a simple default hosted JIT environment complete with simple memory manager and refactor lli to reference that. That's an interesting thing and I'm open to it as a design direction, but it's orthogonal to getting unit tests for other parts of the MCJIT and I'd prefer it be dealt with separately.
> 
> -Jim
> 
> 
> Thanks for the review Jim! I will address the review comments and post an updated patch once I get through my post-vacation email backlog...
> 
> Are you ok with the approach of using the lli memory manager in the unit tests after factoring it out to a separate (but non-public) header. Although this would make the tests depend on a detail of lli, I would prefer that approach over the alternative of duplicating the memory manager in the tests.

Where would such a header go?

Note, because the two managers are the same now doesn't mean they will be in the future. The lli memory manager is not unlikely to grow a bit, but that may well not be required for the unit tests. The unit tests should be as standalone as possible except for the specific bit of functionality they're testing.



> 
> You mention that you're open to having a memory manager that is compatible with MCJIT in the library (i.e. public headers.) What requirements do you envision for such a memory manager, beyond the ability to protect memory sections via correct read/write/executable flags?
> 
> 
> Cheers,
> Dan