[Lldb-commits] [PATCH] D149262: [lldb] Add settings for expression evaluation memory allocations.
Ilia Kuklin via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
Fri Apr 28 16:24:53 PDT 2023
kuilpd added a comment.
In D149262#4306820 <https://reviews.llvm.org/D149262#4306820>, @bulbazord wrote:
> Sorry I should have brought this up earlier but I've noticed you don't have any tests with this change. Is it possible you could add something there? Something to make sure that these settings work as expected.
Should I do it also without a target?
> Sorry for the churn btw, I really appreciate your patience here.
No worries, these are all valid points you raised :)
================
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:
> kuilpd wrote:
> > bulbazord wrote:
> > > 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.
> > Tried returning false, even more tests failed.
> > Did some digging, turns out expression can work without any target, right after starting LLDB.
> > So tests like [[ https://github.com/llvm/llvm-project/blob/main/lldb/test/API/commands/expression/calculator_mode/TestCalculatorMode.py | this one ]] fail because there is no target or process.
> Ah, right, I forgot! A given expression, if simple enough, might be compiled to LLVM IR and then interpreted instead of being compiled to machine code and run in the inferior... The fact that a stack frame size is even relevant in that case is strange but there's not much we can do about it without further refactors. Does something like this work then? If not, this should be fine.
>
> ```
> if (stack_frame_size == 0) {
> ABISP abi_sp;
> if (process && (abi_sp = process->GetABI()))
> stack_frame_size = abi_sp->GetStackFrameSize();
> else
> stack_frame_size = 512 * 1024;
> }
> ```
This worked, thanks!
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D149262/new/
https://reviews.llvm.org/D149262
More information about the lldb-commits
mailing list