[Lldb-commits] [PATCH] D113498: [lldb] Constant-resolve operands to `getelementptr`

[Andy Yankovsky via Phabricator via lldb-commits]

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