Inconsistent use of `#ifdef J9VM_INTERP_NATIVE_SUPPORT`

[janvrany]

While working on CMake support for RISC-V, I noticed that some code use #ifdef J9VM_INTERP_NATIVE_SUPPORT inconsistently, causing compilation to fail if J9VM_INTERP_NATIVE_SUPPORT is not #define'd.

For example, definition of newJitVTable and newJitVTable is guarded

https://github.com/eclipse/openj9/blob/3caadd433c8157b403f337ecd7a057ae188d8297/runtime/util/hshelp.c#L1345-L1355

but (some of) their use not:

https://github.com/eclipse/openj9/blob/3caadd433c8157b403f337ecd7a057ae188d8297/runtime/util/hshelp.c#L1389-L1393

I spotted similar problem JVMTI component and possibly elsewhere.

[pshipton]

@ChengJin01 how does this work for RISC-V normally?

[janvrany]

@pshipton: when building RISC-V using UMA and autotools, J9VM_INTERP_NATIVE_SUPPORT is defined in j9cfg.h and so everything's fine.

[ChengJin01]

Hi @keithc-ca , I think we used to talk about this a couple of months ago. Did you already change things related to J9VM_INTERP_NATIVE_SUPPORT?

[gacholio]

This flag (and so many others) need to be removed - we do not support any builds without JIT. There are also runtime checks for JIT enablement, so this flag should be defined when bringing up a new platform, even if it has no JIT.

Changes to any flags in the UMA core.feature haven't been tested in many years, and I for one stopped using them long ago, so there is unifdeffed code out there that assumes the flags are set properly.

[janvrany]

@gacholio : I see. This means that we may need to use different / introduce a new CMake option to include / exclude JIT since as of now, J9VM_INTERP_NATIVE_SUPPORT is used for that, see:

https://github.com/eclipse/openj9/blob/e5e0324a0f509bfbcfe085c6c0080451e5adb22c/runtime/CMakeLists.txt#L533-L535

and

https://github.com/eclipse/openj9/blob/e5e0324a0f509bfbcfe085c6c0080451e5adb22c/runtime/include/j9cfg.h.in#L170

[gacholio]

I believe that unless you explicitly add -Xjit on the command line, having no JIT library is a soft failure (message printed to the console). I think nothing should be done (other than removing the useless ifdefs).

[keithc-ca]

Hi @keithc-ca Did you already change things related to J9VM_INTERP_NATIVE_SUPPORT?

No, I haven't done anything in that area.

[keithc-ca]

I believe @janvrany was pointing out that for cmake, that flag controls whether the JIT is even compiled. When a new platform is first introduced, it may not be possible to compile the JIT code (lots of missing pieces, I expect), so separate from J9VM_INTERP_NATIVE_SUPPORT we may want a way to handle that: to tell cmake to not even bother trying to compile the JIT code. At worst, the condition could be

if(NOT NEW_PLATFORM_WITH_NO_JIT) 
    add_subdirectory(compiler) 
 endif()

[janvrany]

Yes, @keithc-ca, this is pretty much how I "fixed" it, see #10915

[dnakamura]

At worst, the condition could be

if(NOT NEW_PLATFORM_WITH_NO_JIT) 
  add_subdirectory(compiler) 
 endif()

this is essentially how it works in the uma builds at the moment

<artifact type="target" name="j9jitlauncher">
        <include-if condition="spec.flags.interp_nativeSupport"/>
        <exclude-if condition="spec.linux_riscv.*" />
...
        <commands>
            <command line="$(MAKE) -C compiler -f makefile clean" type="clean"/>
            <command line="$(MAKE) -C compiler -f makefile" type="all"/>
        </commands>
</artifact>

however, this is a rather unpleasant hack. I think the better solution would be to either a) modify the code so that we can build properly with J9VM_INTERP_NATIVE_SUPPORT disabled or, if that turn out to be not possible / not feasible b) Introduce a MODULE_JIT flag to control building, like we do with the other components

[pshipton]

Given https://github.com/eclipse/openj9/issues/10914#issuecomment-710609219, adding MODULE_JIT seems the better approach. There isn't any real need for J9VM_INTERP_NATIVE_SUPPORT any more, so it's a waste of time to fix it and maintain it.

[0xdaryl]

Given #10914 (comment), adding MODULE_JIT seems the better approach.

For whatever my opinion is worth here, I agree with this solution.