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