werat marked 4 inline comments as done. werat added a comment. In D113498#3124525 <https://reviews.llvm.org/D113498#3124525>, @teemperor wrote:
> I really like the solution, but I think by fixing the `CanInterpret` you also > made the test case no longer reach the actual changed interpreting logic? > > So, `CanInterpret` says "we can't interpret this" (which is correct), but > then the interpreter won't run it and your change to `ResolveConstantValue` > isn't actually tested. There is no other test that touches that logic from > what I can see. You could try throwing in some other expression at this that > tests that new code? Maybe some kind of pointer arithmetic on a variable > defined in your expression itself (so it can be constant evaluated). I think you're right, the interpreter now doesn't get to evaluating the operands of `GetElementPtr`. However, I've failed to construct an expression that would have a constant expression with `getelementptr` instruction. So far I've only been able to reproduce it with the example from the test case (which is being rejected during `CanInterpret`). I've tried expressions like `const int x = 1; (int*)100 + (long long)(&x)` and similar (also with `x` being a global variable), but they're are being compiled in a way that `getelementptr` is not a constant expression anymore. In D113498#3124525 <https://reviews.llvm.org/D113498#3124525>, @teemperor wrote: > We could also split out the interpreting change and then this is good to go. I think the `Interpret` and `CanInterpret` should really be in-sync with each other, otherwise more bugs can follow. Given that we can't get an expression for the logic I've implemented, I can make the check more strict -- just verify that all indexes are `ConstantInt`. What do you think? ================ Comment at: lldb/source/Expression/IRInterpreter.cpp:490 + constant_expr->op_end()); + for (Value *op : operands) { + Constant *constant_op = dyn_cast<Constant>(op); ---------------- teemperor wrote: > `for (Value *op : constant_expr->ops())` ? `ConstantExpr` doesn't have `ops()` accessor, only `op_begin/op_end`. Am I missing something? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113498/new/ https://reviews.llvm.org/D113498 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
