Add DOTNET_MODIFIABLE_ASSEMBLIES env var for hot reload support

[mikem8361]

Issue #47274

[jkotas]

We are adding more condition to disable EnC. What is actually going to enable EnC when this env variable is set?

[jkotas]

I think this logic can be simplified:

• Delete // If the program has the "ForceEnc" env variable set we ensure every eligible // module has EnC turned on. if (g_pConfig->ForceEnc() && IsEditAndContinueCapable()) EnableEditAndContinue(); in Module::Initialize
• Delete DisableEditAndContinue method
• Enable EnC here for things that need EnC to be enabled.

[mikem8361]

SetDebuggerInfoBits is called from a lot of places including the ICorDebugModule API. So doesn't the debugger need to set or clear the ENC flag (EnableEditAndContinue and DisableEditAndContinue) depending on the DACF_ENC_ENABLED flag? What I think you are describing will only allow the debugger to enable ENC not disable it.

Something like this in SetDebuggerInfoBits?

 BOOL setEnC = IsEditAndContinueCapable() &&
        ((newBits & DACF_ENC_ENABLED) != 0 ||
          g_pConfig->ForceEnc() ||
         (g_pConfig->DebugAssembliesModifiable() && CORDisableJITOptimizations(GetDebuggerInfoBits())));

    if (setEnC)
    {
        EnableEditAndContinue();
    }

[mikem8361]

Or should there be the else clause?

    if (setEnC)
    {
        EnableEditAndContinue();
    }
    else
    {
        DisableEditAndContinue();
    }

[mikem8361]

In the end, I guess you are assuming that debuggers never enable then disable ENC on a module.

[jkotas]

Yes, that would be odd. I think enabling EnC should be one way operation - once it is enable, it is cannot be disabled.

[mikem8361]

This would change the semantics of a public debugger API if it doesn't allow disabling. Are we ok with that even though it would be old?

/cc: @noahfalk

[jkotas]

Add EXTERNAL_DOTNET_MODIFIABLE_ASSEMBLIES env var

Just to make sure - I assume that the env var does not actually have EXTERNAL_ prefix. It is just the internal C++ id that has it.

[jkotas]

The code depends on IsEditAndContinueCapable to be invariant, but it may actually change because of it depends on GetDebuggerInfoBits that the debugger can change. There is commented out assert in BOOL IsEditAndContinueEnabled() that suggest it may actually happen in some cases. I think that this can interact poorly with this change, depending on how the debugger decides to change the bits.

I think we should:

• Compute IsEditAndContinueCapable just once in Module::Create
• Use immutable cached value of IsEditAndContinueCapable everywhere else. It can be achieve by changing BOOL IsEditAndContinueCapable() to virtual method that returns false in class Module and returns true in class EditAndContinueModule.

[jkotas]

Also, enable the assert in IsEditAndContinueEnabled and change it to just return ((m_dwTransientFlags & IS_EDIT_AND_CONTINUE) != 0);

[mikem8361]

Are we sure that after Module::Create the DACF_ALLOW_JIT_OPTS in the Assembly will not change? What about the debugger API (via DacDbiInterfaceImpl::SetCompilerFlags)? Isn't the module already created by then? It is clear to me and it is confusing because Assembly, DomainAssembly and Module all have flags.

To double check what you are saying about IsEditAndContinueCapable():

Module:
  virtual BOOL IsEditAndContinueCapable() const { return FALSE; }
EditAndContinueModule:
  virtual BOOL IsEditAndContinueCapable() const { return TRUE; }

Module::Create() doesn't need any changes because it already creates an
EditAndContinueModule instance if IsEditAndContinueCapable(pAssembly, file) returns true.

[jkotas]

I agree that it is confusing and hard to reason about.

Module::Create() doesn't need any changes

Correct.

[jkotas]

Note that IsEditAndContinueCapable returns false when the JIT optimizations are enabled for module.

It suggests that CORDisableJITOptimizations is unnecessary here and this new setting behaves same as the existing ForceEnC switch for all practical purposes. It may be worth capturing this as a comment at least since it is not obvious by looking at the code.

[mikem8361]

I assumed up to this point that when the ForceEnc env var was set all release and debug built binaries are ENC'able (except system, dynamic, resource and ready to run). If what you implied above that DACF_ALLOW_JIT_OPTS is already set when the Module or ENC module is created, then yes CORDisableJITOptimizations will always return the same thing (except something about profiling or instrumentation). But I may be confusing between the flags set in the Assembly with the ones set in the Module/ENC Module. I need to dig deeper into this. This is a lot more messy and complicated than I was hoping.

[jkotas]

Thanks!