[PATCH] D130456: [ORC][COFF] Introduce COFFVCRuntimeBootstrapper.

[Lang Hames via Phabricator via llvm-commits]

lhames accepted this revision.
lhames added a comment.
This revision is now accepted and ready to land.

Otherwise LGTM!



================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/COFFVCRuntimeSupport.h:52
+  /// Adds symbol definitions of static version of msvc runtime libraries.
+  Error loadStaticVCRuntime(JITDylib &JD, std::vector<std::string>& ImportedLibraries, bool DebugVersion = false);
+
----------------
`ImportedLibraries` looks like an out-parameter. Could this method return an `Expected<std::vector<std::string>>` instead?

(Similarly for `loadDynamicVCRuntime` below)


================
Comment at: llvm/include/llvm/ExecutionEngine/Orc/ExecutorProcessControl.h:262-267
+  /// Run function with a int (*)(void) signature.
+  virtual Expected<int32_t> runAsVoidFunction(ExecutorAddr VoidFnAddr) = 0;
+
+  /// Run function with a int (*)(int) signature.
+  virtual Expected<int32_t> runAsIntFunction(ExecutorAddr IntFnAddr,
+                                             int Arg) = 0;
----------------
We should move these into the ORC runtime, but we can do that in a follow-up patch.

Could you amend the comment to mention that these methods are temporary, and not to be used?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130456/new/

https://reviews.llvm.org/D130456