teemperor added a comment. In D113498#3131336 <https://reviews.llvm.org/D113498#3131336>, @werat wrote:
> 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. Not sure how flexible your fuzzer setup is regarding downstream patches, but did you try putting some kind of `assert("coverage" && false);` in that new code and try fuzzing until you reach it? > 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? Sure, that would also work for me. FWIW, I'm OOO for an undefined amount of time so I am not sure when I can take a look at this again. Feel free to ping in case you don't find another reviewer. ================ Comment at: lldb/source/Expression/IRInterpreter.cpp:490 + constant_expr->op_end()); + for (Value *op : operands) { + Constant *constant_op = dyn_cast<Constant>(op); ---------------- werat wrote: > teemperor wrote: > > `for (Value *op : constant_expr->ops())` ? > `ConstantExpr` doesn't have `ops()` accessor, only `op_begin/op_end`. Am I > missing something? My bad, the function name was `operands()` 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
