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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
