VModuleKey K not valid here

Dibyendu_Majumdar · August 13, 2019, 8:49pm

Hi,

I am getting following assertion failure when attempting to remove a module.

/llvm/ExecutionEngine/Orc/CompileOnDemandLayer.h:311: llvm::Error
llvm::orc::LegacyCompileOnDemandLayer<BaseLayerT, CompileCallbackMgrT,

::removeModule(llvm::orc::VModuleKey) [with

BaseLayerT = llvm::orc::LegacyIRTransformLayer<llvm::orc::LegacyIRCompileLayer<llvm::orc::LegacyRTDyldObjectLinkingLayer,
llvm::orc::SimpleCompiler>,
std::function<std::unique_ptr<llvm::Module>(std::unique_ptr<llvm::Module>)>

; CompileCallbackMgrT = llvm::orc::JITCompileCallbackManager;

IndirectStubsMgrT = llvm::orc::IndirectStubsManager;
llvm::orc::VModuleKey = long unsigned int]: Assertion `I !=
LogicalDylibs.end() && "VModuleKey K not valid here"' failed.

1) Can 0 ever be a valid VModuleKey? How can one reliably detect an
invalid VModuleKey?
2) Secondly it seems to me that following the assertion there should
be a check so that the code dosn't continue? It is causing
segmentation fault in release builds.

Regards
Dibyendu

lhames · August 13, 2019, 9:14pm

Hi Dibyendu,

Can 0 ever be a valid VModuleKey? How can one reliably detect an invalid VModuleKey?

I believe 0 was a valid VModuleKey in ORCv1. The assertion is checking the the VModuleKey is present in the LogicalDylibs map. That means that you have to have used that key in an addModule call, e.g.:

CODLayer.addModule(K, std::move(M));

and not subsequently removed the key with a call to removeModule.

Secondly it seems to me that following the assertion there should be a check so that the code dosn’t continue? It is causing segmentation fault in release builds.

There should not be any check other than the assertion. Assertions aren’t for recoverable errors or logging, they’re only for verifying that code is being used according to contract. In this case the contract is that removeModule is only called with valid keys. If the assertion is triggering then either the key you’re using is invalid, or my implementation of CompileOnDemandLayer (or the assert itself) is invalid. We just need to fix the offending code.

Out of interest: What are your plans for removeModule? It’s currently unimplemented in ORCv2. Understanding people’s use cases will help with design and prioritization.

Cheers,
Lang.

Dibyendu_Majumdar · August 13, 2019, 9:33pm

Hi Lang,

1) Can 0 ever be a valid VModuleKey? How can one reliably detect an invalid VModuleKey?

I believe 0 was a valid VModuleKey in ORCv1. The assertion is checking the the VModuleKey is present in the LogicalDylibs map. That means that you have to have used that key in an addModule call, e.g.:

CODLayer.addModule(K, std::move(M));

and not subsequently removed the key with a call to removeModule.

Yes indeed that is what happened. So to prevent that I need to check
if VModuleKey was ever initialized ... how do I do that? I would have
to have to add another flag to track the initialized state.
The reason for it not being initialized is that sometimes I cannot
generate JIT code because some bytecode is not yet supported.

2) Secondly it seems to me that following the assertion there should be a check so that the code dosn't continue? It is causing segmentation fault in release builds.

There should not be any check other than the assertion. Assertions aren't for recoverable errors or logging, they're only for verifying that code is being used according to contract. In this case the contract is that removeModule is only called with valid keys. If the assertion is triggering then either the key you're using is invalid, or my implementation of CompileOnDemandLayer (or the assert itself) is invalid. We just need to fix the offending code.

Sure - in that case there ought be some value of VModuleKey that is
invalid. Example NULL for pointers. Otherwise users have to track
separately whether it is initialized or not.
Having said that just as free(NULL) doesn't do anything, and
fclose(NULL) doesn't either, it seems better to not continue to try to
access 'I' if it doesn't exist. Or the assertion should trigger even
in release builds.

Out of interest: What are your plans for removeModule? It's currently unimplemented in ORCv2. Understanding people's use cases will help with design and prioritization.

Ideally it should be possible to delete modules after the code is compiled.
And also delete compiled functions if they are no longer needed.
In my case, the language is Lua based, functions are garbage
collected. So any associated stuff should be deleted when the function
is collected. Sometimes each function is in its own module, but
sometimes several functions are in one module. Either way when the
last function in a module is deleted, I invoke removeModule().

I think in ORCv2 stuff never gets deleted?

Regards
Dibyendu

lhames · August 13, 2019, 10:04pm

Hi Dibyendu,

Yes indeed that is what happened. So to prevent that I need to check
if VModuleKey was ever initialized … how do I do that? I would have
to have to add another flag to track the initialized state.
The reason for it not being initialized is that sometimes I cannot
generate JIT code because some bytecode is not yet supported.

I’m not sure I follow. In ORCv1 VModuleKeys are just integers: initialization is up to you, and the standard way to get a unique key is to always initialize keys by calling ExecutionSession::allocateVModuleKey(). The intent is that you should use a unique key for each module that you add, and keep a copy of the key if it is associated with a module if you want to be able to remove that module later. E.g.

// Module that I will never want to remove:
CODLayer.addModule(ES.createVModuleKey(), std::move(M)); // unique throwaway key.

// Module that I want to be able to remove later:
auto TemporaryModuleKey = ES.createVModuleKey();

CODLayer.addModule(TemporaryModuleKey, std::move(M));
// Do stuff.
CODLayer.removeModule(TemporaryModuleKey);

The main use cases of removable modules that I’m aware of are: (1) temporary expressions in REPLs, and (2) lower-optimized modules in re-optimizing JITs. In the former case you can usually name the key individually (as above). In the latter case you usually want to stash it in a map or context object somewhere and remove it after you’ve replaced the low-optimization-level code with a higher optimization level.

There should not be any check other than the assertion. Assertions aren’t for recoverable errors or logging, they’re only for verifying that code is being used according to contract. In this case the contract is that removeModule is only called with valid keys. If the assertion is triggering then either the key you’re using is invalid, or my implementation of CompileOnDemandLayer (or the assert itself) is invalid. We just need to fix the offending code.

Sure - in that case there ought be some value of VModuleKey that is
invalid. Example NULL for pointers. Otherwise users have to track
separately whether it is initialized or not.

In ORCv1 the solution is to always use ES.createVModuleKey(). Users should only have to keep track keys that they want to be able to remove.

Having said that just as free(NULL) doesn’t do anything, and
fclose(NULL) doesn’t either, it seems better to not continue to try to
access ‘I’ if it doesn’t exist. Or the assertion should trigger even
in release builds.

I don’t have strong opinions about this idiom when applied to free or fclose, but I definitely wouldn’t want it to apply here. What is a programmer who calls “removeModule()” trying to do? Whatever it is, it’s definitely a mistake. The contract is: Only call removeModule with a valid key that is associated with a module that you want to remove. Tracking that is definitely up to the client, but they’d have to do that anyway, otherwise how would you know what needed removing?

Ideally it should be possible to delete modules after the code is compiled.
And also delete compiled functions if they are no longer needed.
In my case, the language is Lua based, functions are garbage
collected. So any associated stuff should be deleted when the function
is collected. Sometimes each function is in its own module, but
sometimes several functions are in one module. Either way when the
last function in a module is deleted, I invoke removeModule().

Yep. That makes perfect sense. You just want to create a unique key for each module and track it in your garbage collection context somewhere.

– Lang.

Dibyendu_Majumdar · August 13, 2019, 10:17pm

Hi Lang,

Yes indeed that is what happened. So to prevent that I need to check
if VModuleKey was ever initialized ... how do I do that? I would have
to have to add another flag to track the initialized state.
The reason for it not being initialized is that sometimes I cannot
generate JIT code because some bytecode is not yet supported.

I'm not sure I follow. In ORCv1 VModuleKeys are just integers: initialization is up to you, and the standard way to get a unique key is to always initialize keys by calling ExecutionSession::allocateVModuleKey(). The intent is that you should use a unique key for each module that you add, and keep a copy of the key if it is associated with a module if you want to be able to remove that module later. E.g.

// Module that I will never want to remove:
CODLayer.addModule(ES.createVModuleKey(), std::move(M)); // unique throwaway key.

// Module that I want to be able to remove later:
auto TemporaryModuleKey = ES.createVModuleKey();
CODLayer.addModule(TemporaryModuleKey, std::move(M));
// Do stuff.
CODLayer.removeModule(TemporaryModuleKey);

The main use cases of removable modules that I'm aware of are: (1) temporary expressions in REPLs, and (2) lower-optimized modules in re-optimizing JITs. In the former case you can usually name the key individually (as above). In the latter case you usually want to stash it in a map or context object somewhere and remove it after you've replaced the low-optimization-level code with a higher optimization level.

Okay maybe I am doing something wrong. I am using:

llvm::orc::VModuleKey K = ES->allocateVModule();

My point was that suppose whether I added the module or not in the way
you suggest, I still need to know which value of they VModuleKey is
'invalid'. In the example assume that CODLayer.addModule() is never
called sometimes. So how do you know whether to call removeModule()?

Regards
Dibyendu

lhames · August 13, 2019, 11:14pm

Hi Dibyendu,

My point was that suppose whether I added the module or not in the way
you suggest, I still need to know which value of they VModuleKey is
‘invalid’. In the example assume that CODLayer.addModule() is never
called sometimes. So how do you know whether to call removeModule()?

That’s definitely up to the client to track. E.g. if you want to make removal generic.

enum class RemoveFrom { None, OptimizeLayer, CODLayer };

struct ModuleInfo {
VModuleKey Key;
RemoveFrom ToRemove;
};

When adding to the CODLayer:

ModuleInfo MInfo;
MInfo.Key = ES.allocateVModule();
MInfo.ToRemove = RemoveFrom::CODLayer;
CODLayer.add(MInfo.Key, std::move(M));

When removing code:

void removeModule(ModuleInfo &MInfo) {
switch (MInfo.ToRemove) {
case RemoveFrom::None:
break;
case RemoveFrom::OptimizeLayer:
OptLayer.removeModule(MInfo.Key);
break;
case RemovFrom::CODLayer:
CODLayer.removeModule(MInfo.Key);
break;
}
}

At the moment removal is simpler in my prototype for removable code in ORCv2: You still create and hold a VModuleKey yourself, but it’s an object that knows how to remove the code for you, so removal looks more like:

auto Key = ES.allocateVModule();
CODLayer.add(Key, std::move(M));
// Do stuff.

// Later.
Key.remove();

Hopefully I can keep it that simple, but there’s still a lot of work to do on this prototype.

Cheers,
Lang.