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

[Raphael Isemann via Phabricator via lldb-commits]

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