bulbazord added a comment.
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.
Sorry for the churn btw, I really appreciate your patience here.
================
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:
> > 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;
}
```
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D149262/new/
https://reviews.llvm.org/D149262
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
