[PATCH] D141215: [clang-repl] Introduce Value to capture expression results

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz added a comment.

This looks a lot better already than the implementation I know from Cling. Well done!

OTOH it's a challenge to review since the patch is really big. Is there no way to break it up and test in isolation? Or strip away details that could be added in iterations? I had a short look and some comments, but I won't find the time in the coming weeks to give it the attention it deserves.

My biggest conceptual concern is that `Value` is part of the Interpreter library. This is going to be a showstopper for out-of-process execution. Conceptually, it should move into a runtime library (like the ORC runtime in compiler-rt) and this won't get easier the more it is entangled with the interpreter. I do see that this adds significant complexity and I guess it's ok to keep this for later, but I do wonder what would be the best way to prepare for it. Essentially, all communication with the interpreter will be asynchronous. Is this something we can do already?



================
Comment at: clang/include/clang/Interpreter/Interpreter.h:83
   /// mangled name.
   llvm::Expected<llvm::JITTargetAddress> getSymbolAddress(GlobalDecl GD) const;
 
----------------
Most of the Orc and JITLink moved away from `JITTargetAddress` already and uses `ExecutorAddr` nowadays. I think we should use `ExecutorAddr` from the beginning in this patch.


================
Comment at: clang/include/clang/Interpreter/Value.h:98
+  QualType getType() const;
+  Interpreter &getInterpreter() const;
+
----------------
Can we make this private and try not to introduce more dependencies on the interpreter instance?

The inherent problem is that we will have to wire these through Orc RPC in the future once we want to support out-of-process execution.


================
Comment at: clang/lib/Interpreter/Interpreter.cpp:406
+    "__InterpreterSetValueNoAlloc", "__InterpreterSetValueWithAlloc",
+    "__InterpreterSetValueCopyArr"};
+
----------------
In Orc we are usually prefixing such functions with the namespace and class name from where they are used. While it can impact readability, it gives these names a structure and does prevent collisions, e.g.:
`__clang_Interpreter_SetValueNoAlloc`.


================
Comment at: clang/lib/Interpreter/Interpreter.cpp:534
+    }
+      llvm_unreachable("Unknown runtime interface kind");
+    }
----------------
Is this a leftover from an old `default` label?


================
Comment at: clang/lib/Interpreter/Value.cpp:91
+  static constexpr unsigned char Canary[8] = {0x4c, 0x37, 0xad, 0x8f,
+                                              0x2d, 0x23, 0x95, 0x91};
+};
----------------
Can we add a comment with an explanation of the magic numbers please?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141215