[llvm-commits] [llvm] [Patch] Add RuntimeDyld.invalidateEmittedSectionsCache()
Jim Grosbach
grosbach at apple.com
Tue May 15 14:00:56 PDT 2012
Hi Danil,
OK. This is definitely much more consistent with what we're looking to do. It's using features of the primary JIT memory managers that will (probably) go away when we remove the old JIT, but that's fine for now. Moving the logic into this memory manager will be part of that process and will provide a good example for what's expected of other applications in that regard.
More specific comments below. Mostly just style and format nitpicks.
With these changes, this is good for commit. Sorry again for the initial confusion. Entirely my fault.
-Jim
> Index: lib/ExecutionEngine/MCJIT/MCJITMemoryManager.h
> ===================================================================
> --- lib/ExecutionEngine/MCJIT/MCJITMemoryManager.h (revision 156826)
> +++ lib/ExecutionEngine/MCJIT/MCJITMemoryManager.h (working copy)
> @@ -34,12 +34,12 @@
>
> uint8_t *allocateDataSection(uintptr_t Size, unsigned Alignment,
> unsigned SectionID) {
> - return JMM->allocateSpace(Size, Alignment);
> + return JMM->allocateDataSection(Size, Alignment, SectionID);
> }
>
> uint8_t *allocateCodeSection(uintptr_t Size, unsigned Alignment,
> unsigned SectionID) {
> - return JMM->allocateSpace(Size, Alignment);
> + return JMM->allocateCodeSection(Size, Alignment, SectionID);
> }
>
> virtual void *getPointerToNamedFunction(const std::string &Name,
> Index: tools/lli/lli.cpp
> ===================================================================
> --- tools/lli/lli.cpp (revision 156826)
> +++ tools/lli/lli.cpp (working copy)
> @@ -35,8 +35,18 @@
> #include "llvm/Support/Process.h"
> #include "llvm/Support/Signals.h"
> #include "llvm/Support/TargetSelect.h"
> +#include "llvm/Support/DynamicLibrary.h"
> +#include "llvm/Support/Memory.h"
> #include <cerrno>
>
> +#ifdef __linux__
> +#ifdef HAVE_SYS_STAT_H
> +#include <sys/stat.h>
> +#endif
> +#include <fcntl.h>
> +#include <unistd.h>
> +#endif
> +
This is so we get declarations for the specially handled symbols below? Assuming so, a comment to that effect would be good. We generally try to wrap platform specific stuff in llvm/Support/*, but given the very specific nature of this glibc trickery, I'm not sure that works well here, so we just need to comment about it.
> #ifdef __CYGWIN__
> #include <cygwin/version.h>
> #if defined(CYGWIN_VERSION_DLL_MAJOR) && CYGWIN_VERSION_DLL_MAJOR<1007
> @@ -175,6 +185,157 @@
> #endif
> }
>
> +// Memory manager for MCJIT
> +class LLIMCJITMemoryManager : public JITMemoryManager {
> +public:
> + SmallVector<sys::MemoryBlock, 16> AllocatedDataMem;
> + SmallVector<sys::MemoryBlock, 16> AllocatedCodeMem;
> + SmallVector<sys::MemoryBlock, 16> FreeCodeMem;
> +
> + LLIMCJITMemoryManager() { };
> + ~LLIMCJITMemoryManager();
> +
> + virtual uint8_t *allocateCodeSection(uintptr_t Size, unsigned Alignment,
> + unsigned SectionID);
> +
> + virtual uint8_t *allocateDataSection(uintptr_t Size, unsigned Alignment,
> + unsigned SectionID);
> +
> + virtual void *getPointerToNamedFunction(const std::string &Name,
> + bool AbortOnFailure = true);
> +
> + // We need do it before execute on some platforms.
> + virtual void invalidateInstructionCache();
> +
> + // The MCJITMemoryManager doesn't use the following functions, so we don't
> + // need implement them.
> + virtual void setMemoryWritable() {};
> + virtual void setMemoryExecutable() {};
> + virtual void setPoisonMemory(bool poison) {};
> + virtual void AllocateGOT() {};
> + virtual uint8_t *getGOTBase() const { return 0; };
> + virtual uint8_t *startFunctionBody(const Function *F,
> + uintptr_t &ActualSize){ return 0; };
> + virtual uint8_t *allocateStub(const GlobalValue* F, unsigned StubSize,
> + unsigned Alignment) { return 0; };
> + virtual void endFunctionBody(const Function *F, uint8_t *FunctionStart,
> + uint8_t *FunctionEnd) { };
> + virtual uint8_t *allocateSpace(intptr_t Size, unsigned Alignment) { return 0; };
> + virtual uint8_t *allocateGlobal(uintptr_t Size, unsigned Alignment) { return 0; };
> + virtual void deallocateFunctionBody(void *Body) { };
> + virtual uint8_t* startExceptionTable(const Function* F,
> + uintptr_t &ActualSize) { return 0; };
> + virtual void endExceptionTable(const Function *F, uint8_t *TableStart,
> + uint8_t *TableEnd, uint8_t* FrameRegister) { };
> + virtual void deallocateExceptionTable(void *ET){ };
If these are unused, we should put an llvm_unreachable() inside them, or an assert() at least. Since we don't expect them to be called, we want the system to complain loudly if anything breaks our assumptions.
If they're used, but we have nothing useful to do in them, then the comment should be updated to be more clear on that point.
It's not clear to me which is which from what's here. Since lli is often used as a reference point for others developing their own JIT drivers, it's helpful to spell things out as explicitly as we can both what we're doing and why.
> +};
> +
> +uint8_t *LLIMCJITMemoryManager::allocateDataSection(uintptr_t Size,
> + unsigned Alignment,
> + unsigned SectionID) {
Indentation on the following arguments is off by a space for this and the next function.
> + if (!Alignment)
> + Alignment = 1;
We want alignment, for certain. The sections need to be aligned to the greatest alignment of anything contained in them. One would hope that the caller (via whatever created the object file) would know that and would be passing in a non-zero alignment. I'm not sure that's true in all cases, though, so using a generous default alignment is a reasonable fallback. I believe 16-byte alignment is typical for lots of targets, and is larger than those that differ, so that's probably a reasonable default.
> + uint8_t *Addr = (uint8_t*)calloc((Size+Alignment-1)/Alignment,Alignment);
> + AllocatedDataMem.push_back(sys::MemoryBlock(Addr, Size));
> + return Addr;
> +}
> +
> +uint8_t *LLIMCJITMemoryManager::allocateCodeSection(uintptr_t Size,
> + unsigned Alignment,
> + unsigned SectionID) {
> + if (!Alignment)
> + Alignment = 1;
We could probably have a smaller default alignment for code than for data, but there's really not a lot of point to being that picky. I'd just use 16 here as well for simplicity.
> +
> + unsigned NeedAllocate = Alignment*((Size+Alignment-1)/Alignment+1);
> + uintptr_t Addr = 0;
> + // Lookup free memory block with the enough size.
I suggest a bit more verbose comment here. Something like:
"Look in the list of free code memory regions and use a block there if one is available."
> + for (int i = 0, e = FreeCodeMem.size(); i != e; ++i) {
> + sys::MemoryBlock &MB = FreeCodeMem[i];
> + if (MB.size() >= NeedAllocate) {
> + Addr = (uintptr_t)MB.base();
> + uintptr_t EndOfBlock = Addr + MB.size();
> + // Allign the address.
s/Allign/Align/
> + Addr = (Addr + Alignment-1) & ~(uintptr_t)(Alignment-1);
> + // Store cutted free memory block.
> + FreeCodeMem[i] = sys::MemoryBlock((void*)(Addr+Size),
> + EndOfBlock-Addr-Size);
> + break;
Just return the address directly here rather than breaking out of the loop.
> + }
> + }
> + if (!Addr) {
With the early return above when we find a free block, we don't need this conditional at all.
> + // If existed block not found, allocate new block.
I'd expand this a touch. Something like, "No pre-allocated free block was large enough. Allocate a new memory region."
> + sys::MemoryBlock MB = sys::Memory::AllocateRWX(NeedAllocate, 0, 0);
> +
> + AllocatedCodeMem.push_back(MB);
> + Addr = (uintptr_t)MB.base();
> + uintptr_t EndOfBlock = Addr + MB.size();
> + // Allign the address.
> + Addr = (Addr + Alignment-1) & ~(uintptr_t)(Alignment-1);
> + // The AllocateRWX may allocate much more memory than we need. In this
> + // case we will store unused memory as free memory block.
Grammar tweak:
"In this case, we store the unused memory as a free memory block."
> + unsigned FreeSize = EndOfBlock-Addr-Size;
> + if (FreeSize > 16) {
> + FreeCodeMem.push_back(sys::MemoryBlock((void*)(Addr+Size), FreeSize));
> + }
No need for enclosing compound statement braces here since the body is a single statement.
> + }
> + // Return alligned address
> + return (uint8_t*)Addr;
> +}
> +
> +void LLIMCJITMemoryManager::invalidateInstructionCache() {
> + for (int i = 0, e = AllocatedCodeMem.size(); i != e; ++i) {
> + sys::Memory::InvalidateInstructionCache(AllocatedCodeMem[i].base(),
> + AllocatedCodeMem[i].size());
> + }
No need for enclosing compound statement braces.
> +}
> +
> +void *LLIMCJITMemoryManager::getPointerToNamedFunction(const std::string &Name,
> + bool AbortOnFailure) {
> +#if defined(__linux__)
> + //===--------------------------------------------------------------------===//
> + // Function stubs that are invoked instead of certain library calls
> + //
> + // Force the following functions to be linked in to anything that uses the
> + // JIT. This is a hack designed to work around the all-too-clever Glibc
> + // strategy of making these functions work differently when inlined vs. when
> + // not inlined, and hiding their real definitions in a separate archive file
> + // that the dynamic linker can't see. For more info, search for
> + // 'libc_nonshared.a' on Google, or read http://llvm.org/PR274.
> + if (Name == "stat") return (void*)(intptr_t)&stat;
> + if (Name == "fstat") return (void*)(intptr_t)&fstat;
> + if (Name == "lstat") return (void*)(intptr_t)&lstat;
> + if (Name == "stat64") return (void*)(intptr_t)&stat64;
> + if (Name == "fstat64") return (void*)(intptr_t)&fstat64;
> + if (Name == "lstat64") return (void*)(intptr_t)&lstat64;
> + if (Name == "atexit") return (void*)(intptr_t)&atexit;
> + if (Name == "mknod") return (void*)(intptr_t)&mknod;
> +#endif // __linux__
> +
> + const char *NameStr = Name.c_str();
> + void *Ptr = sys::DynamicLibrary::SearchForAddressOfSymbol(NameStr);
> + if (Ptr) return Ptr;
> +
> + // If it wasn't found and if it starts with an underscore ('_') character,
> + // try again without the underscore.
> + if (NameStr[0] == '_') {
> + Ptr = sys::DynamicLibrary::SearchForAddressOfSymbol(NameStr+1);
> + if (Ptr) return Ptr;
> + }
> +
> + if (AbortOnFailure) {
No need for enclosing compound statement braces.
> + report_fatal_error("Program used external function '"+Name+
Spaces around the "+" operators.
> + "' which could not be resolved!");
> + }
> + return 0;
> +}
> +
> +LLIMCJITMemoryManager::~LLIMCJITMemoryManager() {
> + for (unsigned i = 0, e = AllocatedCodeMem.size(); i != e; ++i)
> + sys::Memory::ReleaseRWX(AllocatedCodeMem[i]);
> + for (unsigned i = 0, e = AllocatedDataMem.size(); i != e; ++i)
> + free(AllocatedDataMem[i].base());
> +}
> +
> //===----------------------------------------------------------------------===//
> // main Driver function
> //
> @@ -233,8 +394,11 @@
> Mod->setTargetTriple(Triple::normalize(TargetTriple));
>
> // Enable MCJIT if desired.
> + LLIMCJITMemoryManager *JMM = 0;
> if (UseMCJIT && !ForceInterpreter) {
> builder.setUseMCJIT(true);
> + JMM = new LLIMCJITMemoryManager();
> + builder.setJITMemoryManager(JMM);
> }
>
> CodeGenOpt::Level OLvl = CodeGenOpt::Default;
> @@ -265,6 +429,9 @@
> exit(1);
> }
>
> + if (JMM)
> + JMM->invalidateInstructionCache();
A comment explaining why we need to do this would be good.
> +
> // The following functions have no effect if their respective profiling
> // support wasn't enabled in the build configuration.
> EE->RegisterJITEventListener(
> Index: tools/llvm-rtdyld/llvm-rtdyld.cpp
> ===================================================================
> --- tools/llvm-rtdyld/llvm-rtdyld.cpp (revision 156826)
> +++ tools/llvm-rtdyld/llvm-rtdyld.cpp (working copy)
> @@ -63,20 +63,37 @@
> return 0;
> }
>
> + virtual void clearCache();
A comment explaining why we need to do this would be good.
> +
> };
>
> uint8_t *TrivialMemoryManager::allocateCodeSection(uintptr_t Size,
> unsigned Alignment,
> unsigned SectionID) {
> - return (uint8_t*)sys::Memory::AllocateRWX(Size, 0, 0).base();
> + sys::MemoryBlock MB = sys::Memory::AllocateRWX(Size, 0, 0);
> + FunctionMemory.push_back(MB);
> + return (uint8_t*)MB.base();
> }
>
> uint8_t *TrivialMemoryManager::allocateDataSection(uintptr_t Size,
> unsigned Alignment,
> unsigned SectionID) {
> - return (uint8_t*)sys::Memory::AllocateRWX(Size, 0, 0).base();
> + sys::MemoryBlock MB = sys::Memory::AllocateRWX(Size, 0, 0);
> + DataMemory.push_back(MB);
> + return (uint8_t*)MB.base();
> }
>
> +void TrivialMemoryManager::clearCache() {
> + for (int i = 0, e = FunctionMemory.size(); i != e; ++i) {
> + sys::Memory::InvalidateInstructionCache(FunctionMemory[i].base(),
> + FunctionMemory[i].size());
> + }
> + for (int i = 0, e = DataMemory.size(); i != e; ++i) {
> + sys::Memory::InvalidateInstructionCache(DataMemory[i].base(),
> + DataMemory[i].size());
> + }
No need for enclosing compound statement braces here since the body is a single statement.
> +}
> +
> static const char *ProgramName;
>
> static void Message(const char *Type, const Twine &Msg) {
> @@ -113,6 +130,8 @@
>
> // Resolve all the relocations we can.
> Dyld.resolveRelocations();
> + // Invalidate cache before execute.
> + MemMgr->clearCache();
>
> // FIXME: Error out if there are unresolved relocations.
>
On May 15, 2012, at 11:50 AM, Danil Malyshev <dmalyshev at accesssoftek.com> wrote:
> Hi Jim,
>
> I see, it's should be in the JITMemoryManager instead of the RTDyldMemoryManager.
>
> Please review attached the patch. It's adds LLIMCJITMemoryManager to lli.
> The another way is add the clear cache to the DefaultJITMemoryManager, but attached way is more flexible for MCJIT tasks.
>
>
> Regards,
> Danil
>
> From: Jim Grosbach [grosbach at apple.com]
> Sent: Monday, May 14, 2012 9:55 AM
> To: Danil Malyshev
> Cc: llvm-commits at cs.uiuc.edu
> Subject: Re: [llvm-commits] [llvm] [Patch] Add RuntimeDyld.invalidateEmittedSectionsCache()
>
> Hi Danil,
>
> I'm not sure what additional information you're looking for. The logic you're wanting to add belongs in the memory manager of the client application, not the MCJIT. For example, in the case of lli, it should be part of the TrivialMemoryManager there.
>
> Note that the old JIT did this differently, as it assumed it wasn't ever going to deal with a remote target. It could, and did, make lots of assumptions about that. As such, be wary of using its code paths as a guide for how the MCJIT should work.
>
> -Jim
>
>
> On May 14, 2012, at 9:49 AM, Danil Malyshev <dmalyshev at accesssoftek.com> wrote:
>
>> Ping.
>>
>>
>> Regards,
>> Danil
>>
>>
>> From: llvm-commits-bounces at cs.uiuc.edu [llvm-commits-bounces at cs.uiuc.edu] On Behalf Of Danil Malyshev [dmalyshev at accesssoftek.com]
>> Sent: Tuesday, May 08, 2012 5:50 PM
>> To: Jim Grosbach
>> Cc: llvm-commits at cs.uiuc.edu
>> Subject: Re: [llvm-commits] [llvm] [Patch] Add RuntimeDyld.invalidateEmittedSectionsCache()
>>
>> Hi Jim,
>>
>> Yes, I agree that it's client application job. But the MCJIT hard linked with hosted platform. It loads hosted library, it executes code on the host machine and etc. JIT and MCJIT was hidden from appication by EngineBuilder, their memory managers hidden too. The JIT does it for own code, I believe the MCJIT should do it too.
>> The remote JIT will not be based by MCJIT, it's will be something new, with own memory manager inherited from RTDyldMemoryManager and in this case the remote client application will be cares about their cache.
>>
>> Regards,
>> Danil
>>
>> From: Jim Grosbach [grosbach at apple.com]
>> Sent: Tuesday, May 08, 2012 4:24 PM
>> To: Danil Malyshev
>> Cc: llvm-commits at cs.uiuc.edu
>> Subject: Re: [llvm-commits] [llvm] [Patch] Add RuntimeDyld.invalidateEmittedSectionsCache()
>>
>> Hi Danil,
>>
>> This is closer, but not quite. By the client's memory manager, I mean the application. This functionality shouldn't be in the core MC-JIT at all. For example, it belongs in the TrivialMemoryManager in llvm-rtdyld.cpp. That includes keeping the list of allocations and sections. Consider a remote client. The MemoryBlock map you're creating here will refer to the local copy, not the memory that's actually on the target where it'll be executed. The code that knows how to map between the two is the client's memory manager, so that's where this logic belongs as well.
>>
>> -Jim
>>
>> On May 8, 2012, at 4:09 PM, Danil Malyshev <dmalyshev at accesssoftek.com> wrote:
>>
>>> Hi,
>>>
>>> Please review changed the patch.
>>> This patch adds finalize() to RTDyldMemoryManager, and implements it for MCJITMemoryManager as invalidate instruction cache.
>>>
>>>
>>> Regards,
>>> Danil
>>>
>>>
>>> From: Jim Grosbach [grosbach at apple.com]
>>> Sent: Monday, May 07, 2012 4:16 PM
>>> To: Danil Malyshev
>>> Cc: llvm-commits at cs.uiuc.edu
>>> Subject: Re: [llvm-commits] [llvm] [Patch] Add RuntimeDyld.invalidateEmittedSectionsCache()
>>>
>>> Hi Danil,
>>>
>>> That's not the RuntimeDyld's job. That functionality belongs in elsewhere, probably in the client's memory manager implementation where it's hooked into copying the compiled code into the target's address space. For a hosted platform, something similar just w/o the copying.
>>>
>>> -Jim
>>>
>>> On May 7, 2012, at 2:09 PM, Danil Malyshev <dmalyshev at accesssoftek.com> wrote:
>>>
>>>> Hi everyone,
>>>>
>>>> Please review attached the patch.
>>>> This patch adds invalidateEmmittedSectionsCache() to the RuntimeDyld and uses it in the MCJIT after resolve relocations.
>>>> The MCJIT works unstable on the ARM platforms without it.
>>>> This patch haven't any tests because I want to commit the common ExecutionEngine/MCJIT tests after attached the patch will be committed. These tests will cover it.
>>>> Regards,
>>>> Danil
>>>> <RuntimeDyld_invalidateEmittedSectionsCache-01.patch>_______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>
>>> <RTDyldMemoryManager_finalize-01.patch>
>
> <LLIMCJITMemoryManager.patch>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120515/852f9f20/attachment.html>
More information about the llvm-commits
mailing list