[PATCH] tidy the MCJIT tests now applyPermissions will invalidate cache

[Filip Pizlo]

On May 13, 2013, at 9:00 AM, David Tweed <david.tweed at arm.com> wrote:

> Hi,
>  
> Thanks for the review.
>  
> | Wouldn't it be better if the tests did the "right thing" and called TheJIT->finalizeObject()?
>  
> I think you're probably right. It would also be good if the finalizeObject() declaration had a comment mentioning that's what it's intended use is. AFAICS the only comment is the oblique reference in SectionMemoryManager.h.
>  
> |I'm not sure how much I like SectionMemoryManager::applyPermissions() being specified to also mean flushing cache, but I don't have a strong opinion on that.
>  
> I think that it may be that applyPermissions might not be quite the right name, but my basic reasoning is that it does do the cache invalidation inside the body, so there should be commentary documenting this fact (particularly since it's allowed for alternative implementations of the memory manager to be used). As you can probably tell, in addition to the professional interest in having MCJIT work well on ARM, I'm interested in having LLVM's JITing facilities be documented enough to be usable by "casual users" without them needing to become JIT implementation experts.

I'm totally with you on this!  And I agree with that being documented. I just think that calling finalizeObject() is the more canonical way of doing this, even if applyPermissions() also works - and the tests should do the canonical thing unless there is good reason not to.

I do like the idea of adding more regression tests that cover even the non-canonical ways of doing things just so we can have a way of finding out if they break. But the generic tests should still do the canonical thing since they are not meant to cover the more exotic behaviors. 

>  
> I'll wait a bit for other comments, then respin a patch.

Cool!

>  
> Cheers,
> Dave
>  
> -Filip
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130513/67bd08b3/attachment.html>