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

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.


================
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;
----------------
kuilpd wrote:
> 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?
> 
How do the tests fail? If the process is wrong that sounds like a pretty bad bug :(

I would think that we could return `false` with some logging or an error would be appropriate if we don't have an ABI. I may be misunderstanding something but I would think that the tests should still pass when we `return false` there... I hope.


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

https://reviews.llvm.org/D149262