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
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
