kuilpd 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;
----------------
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.
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
