[llvm-commits] [llvm] r56303 - in /llvm/trunk: include/llvm/System/Memory.h lib/ExecutionEngine/ExecutionEngine.cpp lib/ExecutionEngine/JIT/JITEmitter.cpp lib/ExecutionEngine/JIT/JITMemoryManager.cpp lib/System/Memory.cpp lib/System/Unix/Memory.inc

[Dan Gohman]

On Thu, September 18, 2008 12:54 am, Evan Cheng wrote:

> +  // Mark code region readable and executable if it's not so already.
> +  sys::Memory::SetRXPrivilege(FnStart, FnEnd-FnStart);

Shouldn't this code check SetRXPrivilege's return value? It may
not be able to do much if SetRXPrivilege fails, but printing to
cerr and calling abort, as this code does to handle other errors,
at least seems friendlier than the alternative, letting the code
take a SIGSEGV later on.

>  DefaultJITMemoryManager::DefaultJITMemoryManager() {
>    // Allocate a 16M block of memory for functions.
> +#if defined(__APPLE__) && defined(__arm__)
> +  sys::MemoryBlock MemBlock = getNewMemoryBlock(4 << 20);
> +#else
>    sys::MemoryBlock MemBlock = getNewMemoryBlock(16 << 20);
> +#endif

This introduces a platform-specific magic number in
platform-independent code, with no comment and no mention in
the commit message.

> +#if defined(__APPLE__) && defined(__arm__)
> +  void *pa = ::mmap(start, pageSize*NumPages, PROT_READ|PROT_EXEC,
> +                    flags, fd, 0);
> +#else
>    void *pa = ::mmap(start, pageSize*NumPages,
> PROT_READ|PROT_WRITE|PROT_EXEC,
>                      flags, fd, 0);
> +#endif
[...]
> +#if defined(__APPLE__) && defined(__arm__)
> +  kern_return_t kr = vm_protect(mach_task_self(), (vm_address_t)pa,
> +                                (vm_size_t)(pageSize*NumPages), 0,
> +                                VM_PROT_READ | VM_PROT_EXECUTE |
> VM_PROT_COPY);
> +  if (KERN_SUCCESS != kr) {
> +    MakeErrMsg(ErrMsg, "vm_protect max RWX failed\n");
> +    return sys::MemoryBlock();
> +  }
> +
> +  kr = vm_protect(mach_task_self(), (vm_address_t)pa,
> +                  (vm_size_t)(pageSize*NumPages), 0,
> +                  VM_PROT_READ | VM_PROT_WRITE);
> +  if (KERN_SUCCESS != kr) {
> +    MakeErrMsg(ErrMsg, "vm_protect RW failed\n");
> +    return sys::MemoryBlock();
> +  }
> +#endif

The mmap change and the first call to vm_protect here suggest
that something highly non-obvious is happening here, but this
is lib/System and perhaps that's what this system needs.

Looking at the second vm_protect call though, and the fact that
this patch introduces a SetRXPrivilege API call, it looks like
the intention might be to allocate the memory as RW for the JIT
to write to it, and then to set it to RX when it's ready to be
executed. Perhaps AllocateRWX should be renamed to AllocateRW?
Even if it allocates memory RWX on systems with SetRXPrivilege
as a no-op, it would be good to make the API consistent.

Dan