Page MenuHomePhabricator

[ExecutionEngine] Add to the C API possibility to create custom SectionMemoryManager
Needs ReviewPublic

Authored by jauhien on Sep 3 2015, 1:41 PM.

Details

Summary

New set of functions in ExecutionEngine C API allows to create custom memory manager
based on the SectionMemoryManager with some of methods overridden by user.
Supprot for LLVM RTTI was added to memory managers hierarchy as this is necessary for
easy implementation of functions that call parent methods.

This change is useful e.g. for implementing custom JIT with the need of symbol resolution
similar to the one from MCJIT variant of the Kaleidoscope tutorial. SectionMemoryManager
can be reused in this case with only getSymbolAddress overriden with the appropriate callback.

This patch is taken with minor changes from my Rust LLVM bindings (https://github.com/jauhien/iron-llvm) used at the moment in the Rust LLVM Kaleidoscope tutorial (https://github.com/jauhien/iron-kaleidoscope). There a relevant example of real use can be found (https://github.com/jauhien/iron-llvm/blob/master/src/execution_engine/{memory_manager.rs,wrappers.rs} and https://github.com/jauhien/iron-kaleidoscope/blob/master/src/jitter.rs).

Diff Detail

Event Timeline

jauhien updated this revision to Diff 33965.Sep 3 2015, 1:41 PM
jauhien retitled this revision from to [ExecutionEngine] Add to the C API possibility to create custom SectionMemoryManager.
jauhien updated this object.
jauhien added a subscriber: llvm-commits.
DiamondLovesYou added a subscriber: DiamondLovesYou.
DiamondLovesYou added inline comments.
include/llvm-c/ExecutionEngine.h
214

What will ErrMsg's lifetime be (or who owns it after the callback returns)? Also, how would use of alloc_jemalloc affect this (w.r.t. Rust)?

223

Typos

225

*to the client object

lib/ExecutionEngine/ExecutionEngineBindings.cpp
450

Keep this line.

lhames edited edge metadata.Sep 6 2015, 10:59 PM

Hi Jauhien,

I don't think this is a good fit for the stable C API. To remain stable, we need to minimize the dependency on implementation details. This patch would add a lot of dependence on the behavior of SectionMemoryManager.

Are there any aspects that can't be satisfied by re-implementing SectionMemoryManager in C? (I know that may seem inconvenient, but it allows the client to remain stable, which is something SectionMemoryManager doesn't guarantee).

I think the big missing piece at the moment is the symbol lookup support (getSymbolAddress and getSymbolAddressInDylib). If that's what you need I would prefer to add a new variant of LLVMCreateSimpleMCJITMemoryManager that also supports overloading those functions.

jauhien added inline comments.Sep 8 2015, 10:55 AM
include/llvm-c/ExecutionEngine.h
214

This is the same situation as in already existing LLVMMemoryManagerFinalizeMemoryCallback. C API binding code freez it. With alloc_jemalloc -- a good question, but something like this already exists in the C API, so it is not in the scope of this patch.

jauhien marked 2 inline comments as done.Sep 8 2015, 10:56 AM
jauhien updated this revision to Diff 34268.Sep 8 2015, 2:52 PM
jauhien edited edge metadata.

Fix typos.

jauhien marked 3 inline comments as done.Sep 8 2015, 2:53 PM

Hi Lang,

thanks for reply.

Hi Jauhien,

I don't think this is a good fit for the stable C API. To remain stable, we need to minimize the dependency on implementation details. This patch would add a lot of dependence on the behavior of SectionMemoryManager.

This patch relies on these things:

  1. Methods needed for memory management (already used in create mcjit)
  2. Methods for symbol resolution
  3. Existence of section MM and the fact that it implements mentioned methods

Are there any aspects that can't be satisfied by re-implementing SectionMemoryManager in C? (I know that may seem inconvenient, but it allows the client to remain stable, which is something SectionMemoryManager doesn't guarantee).

I think the big missing piece at the moment is the symbol lookup support (getSymbolAddress and getSymbolAddressInDylib). If that's what you need I would prefer to add a new variant of LLVMCreateSimpleMCJITMemoryManager that also supports overloading those functions.

Yes, this is the main reason why I needed this patch. But if we will introduce something for fixing this, we will have a dependency on the point 2 in the list above. And the fact of existence of section MM and implementation of all listed methods by it looks quite stable for me (no much of dependency of its behavior really). Or am I wrong somewhere?

lhames added a comment.Sep 8 2015, 3:15 PM

Hi Jauhien,

This patch relies on these things:

  1. Methods needed for memory management (already used in create mcjit)
  2. Methods for symbol resolution
  3. Existence of section MM and the fact that it implements mentioned methods

Point 3 is the problem: This would lock SectionMemoryManager and its behavior in as part of the stable C API. That's convenient from an implementation standpoint, but it's a problem for maintenance. We want to be able to redesign the memory management behavior in the future.

Are there any aspects that can't be satisfied by re-implementing SectionMemoryManager in C? (I know that may seem inconvenient, but it allows the client to remain stable, which is something SectionMemoryManager doesn't guarantee).

I think the big missing piece at the moment is the symbol lookup support (getSymbolAddress and getSymbolAddressInDylib). If that's what you need I would prefer to add a new variant of LLVMCreateSimpleMCJITMemoryManager that also supports overloading those functions.

Yes, this is the main reason why I needed this patch. But if we will introduce something for fixing this, we will have a dependency on the point 2 in the list above. And the fact of existence of section MM and implementation of all listed methods by it looks quite stable for me (no much of dependency of its behavior really). Or am I wrong somewhere?

I'm happy to add symbol resolution methods to the abstract C memory manager interface. The stability of SectionMemoryManager is accidental though: it's just that nobody has found time to work on it. As I said - we want to reserve the right to change that behavior in the future, so we can't expose it via the stable API.

Hi Lang,

Hi Jauhien,

I'm happy to add symbol resolution methods to the abstract C memory manager interface. The stability of SectionMemoryManager is accidental though: it's just that nobody has found time to work on it. As I said - we want to reserve the right to change that behavior in the future, so we can't expose it via the stable API.

I'll work on the support for symbol resolution methods then. But before it I want to figure out one thing that I do not understand. This thing is how APIs that I've added need SectionMemoryManager implementation to be stable.

Three facts that I rely on (memory management API, symbol resolution API, existence of some MM) will be exposed by execution engine API anyway. If some of MM methods will change implementation, client that uses it correctly will not see it, as it sees only quite common interface. Even SectionMemoryManager itself can be removed and replaced by something that implements necessary functionality. Could you, please, point me to the exact place where this patch (API, not realization) demands SectionMemoryManager to be stable?

lhames added a comment.Sep 9 2015, 1:45 PM

Hi Jauhien,

I'll work on the support for symbol resolution methods then. But before it I want to figure out one thing that I do not understand. This thing is how APIs that I've added need SectionMemoryManager implementation to be stable.

Three facts that I rely on (memory management API, symbol resolution API, existence of some MM) will be exposed by execution engine API anyway. If some of MM methods will change implementation, client that uses it correctly will not see it, as it sees only quite common interface. Even SectionMemoryManager itself can be removed and replaced by something that implements necessary functionality. Could you, please, point me to the exact place where this patch (API, not realization) demands SectionMemoryManager to be stable?

The problem is the fall-back to SectionMemoryManager for unimplemented callbacks, which seems to be a central aim for this patch.

As an example: If an API takes a callback called 'allocateCodeSection' it should document some contract on it. E.g. 'allocateCodeSection should allocate memory for text sections, the memory will be aligned to at least Align bytes (but could be higher). Memory allocated via this method must be marked executable when memory is finalized, etc.'. Once that's documented people can reason about the guarantees provided by the contract.

In this patch, the contract is 'If you do not provide an allocateCodeSection method you will get whatever SectionMemoryManager does', but there's no contract for SectionMemoryManager's behavior. Since the C API is stable, people are going to assume that the implied contract is "Whatever SectionMemoryManager does Today", and they may come to rely on implementation details of SectionMemoryManager that aren't actually guaranteed to be stable.

If we were to provide a C API for obtaining a default memory manager implementation, it would have to have a very restricted contract* (i.e. the default implementation would only be guaranteed to meet the same bare-minimum requirements that we place on user-supplied memory managers). The question is whether a default implementation with so few guarantees would be useful to clients. I can imagine it being useful as a stop-gap solution for people bringing up a new JIT, but anyone building a robust solution would want to roll their own versions of all methods. (As a straw man example of the problem: the lack of contract means we might choose to slow every method down by 1000000x and nobody would have the right to complain, because the contract makes no guarantees on performance).

If all you're looking for is a bare-bones default memory manager implementation I think this patch could serve as a base for that, but I have to stress: This would *not* be a matter of exposing access to SectionMemoryManager through the C API, even if that's how it happened to be implemented today. Tomorrow we might replace SectionMemoryManager with something that still met the restricted contact, but behaved totally different to SectionMemoryManager.

So - the question is whether you're just looking for a default memory manager, or for SectionMemoryManager in particular. The former is doable, the latter is not.

  • There's nothing stopping us from providing more guarantees on a default memory manager in time, but the more guarantees we provide the higher the maintenance burden going forward, so the amount of consideration and commitment needed rises a lot.

Hi Lang,

thank you for in-depth answer. Now I clearly see what are your reasons.

Hi Jauhien,

If all you're looking for is a bare-bones default memory manager implementation I think this patch could serve as a base for that, but I have to stress: This would *not* be a matter of exposing access to SectionMemoryManager through the C API, even if that's how it happened to be implemented today. Tomorrow we might replace SectionMemoryManager with something that still met the restricted contact, but behaved totally different to SectionMemoryManager.

So - the question is whether you're just looking for a default memory manager, or for SectionMemoryManager in particular. The former is doable, the latter is not.

What I'm looking for is (starting from the most important):

  1. possibility to have MM with custom symbol resolution
  2. have default MM that is ok for simplest uses

If we could use this patch as a base for solving both problems, it would be really great. What should I change to have this patch accepted as implementation of a bare-bones default MM? The most obvious thing that I see is renaming (so no section MM is mentioned) and warning for users in comments about only base guaranties.

What I would like to have as result is:

  1. Possibility to implement completely custom MM with custom symbol resolution
  2. Possibility to define not all callbacks, for undefined callbacks some base implementation is used. No guaranties apart from those most basic.
  3. Possibility to call custom implementation from user implemented callbacks (specially usefull for symbol resolution).