[Lldb-commits] [PATCH] D149262: [lldb] Add settings for expression evaluation memory allocations.

[Ilia Kuklin via Phabricator via lldb-commits]

kuilpd added inline comments.


================
Comment at: lldb/source/Expression/LLVMUserExpression.cpp:38
 
+using namespace lldb;
 using namespace lldb_private;
----------------
bulbazord wrote:
> What in this file uses something from the lldb namespace now?
ABI class


================
Comment at: lldb/source/Expression/LLVMUserExpression.cpp:340-348
+        Process *process_sp;
+        ABISP abi_sp;
+        if ((process_sp = exe_ctx.GetProcessPtr()) &&
+            (abi_sp = process_sp->GetABI())) {
+          stack_frame_size = abi_sp->GetStackFrameSize();
+        } else {
+          stack_frame_size = 512 * 1024;
----------------
bulbazord wrote:
> A few things:
> - I don't think you need to re-extract process from `exe_ctx`, the variable `process` at the beginning of this function should have a valid Process pointer in it already (see: `LockAndCheckContext` at the beginning of this function). We already assume `target` is valid and use it in the same way.
> - If we can't get an ABI from the Process, do we want to assume a stack frame size of `512 * 1024`? Maybe there is a use case I'm missing, but if we don't have an ABI I'm not convinced that `PrepareToExecuteJITExpression` should succeed. LLDB will need some kind of ABI information to correctly set up the register context to call the JITed code anyway.
- I tried that, but a lot of tests fail on `GetABI()` after that, so had to re-extract it. Not sure why.
- `512 * 1024` is what was hardcoded there by default. It makes sense that ABI has to exist, but leaving no default value in case if it's not retreived is weird as well. Or should we return an error in that case?



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

https://reviews.llvm.org/D149262