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

[Andy Yankovsky via Phabricator via lldb-commits]

werat added a comment.

In D113498#3314729 , @shafik wrote:

> I was hoping you would add a positive test as well like the one I came up 
> with that covered the new code as well.

Oh, whoops, sorry, I don’t know how I missed your point :)
I will add a test, thanks for a code sample!


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

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

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added a comment.

I was hoping you would add a positive test as well like the one I came up with 
that covered the new code as well.


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

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

[Andy Yankovsky via Phabricator via lldb-commits]

werat added a comment.

In D113498#3308499 , 
@stella.stamenova wrote:

> It looks like the new test is failing on the Windows bot:
>
> https://lab.llvm.org/buildbot/#/builders/83/builds/15049

Whoops, static lookup seem to be broken on Windows (other tests in this file 
skip Windows altogether). I've disabled this test in 
https://github.com/llvm/llvm-project/commit/e0f2375b5262d0dd778ecaf0628f905d241da733
 for now.


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

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

[Stella Stamenova via Phabricator via lldb-commits]

stella.stamenova added a comment.

It looks like the new test is failing on the Windows bot:

https://lab.llvm.org/buildbot/#/builders/83/builds/15049


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

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

[Andy Yankovsky via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGafb446e8a61d: [lldb] Constant-resolve operands to 
`getelementptr` (authored by werat).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113498/new/

https://reviews.llvm.org/D113498

Files:
  lldb/source/Expression/IRInterpreter.cpp
  lldb/test/API/lang/cpp/static_members/TestCPPStaticMembers.py


Index: lldb/test/API/lang/cpp/static_members/TestCPPStaticMembers.py
===
--- lldb/test/API/lang/cpp/static_members/TestCPPStaticMembers.py
+++ lldb/test/API/lang/cpp/static_members/TestCPPStaticMembers.py
@@ -41,3 +41,31 @@
 self.createTestTarget()
 self.expect("expression s_c", error=True,
 startstr="error: use of undeclared identifier 's_d'")
+
+def test_no_crash_in_IR_arithmetic(self):
+"""
+Test that LLDB doesn't crash on evaluating specific expression 
involving
+pointer arithmetic and taking the address of a static class member.
+See https://bugs.llvm.org/show_bug.cgi?id=52449
+"""
+self.build()
+lldbutil.run_to_source_breakpoint(self, "// stop in main", 
lldb.SBFileSpec("main.cpp"))
+
+# This expression contains the following IR code:
+# ... i64 ptrtoint (i32* @_ZN1A3s_cE to i64)) ...
+expr = "(int*)100 + (long long)(&A::s_c)"
+
+# The IR interpreter doesn't support non-const operands to the
+# `GetElementPtr` IR instruction, so verify that it correctly fails to
+# evaluate expression.
+opts = lldb.SBExpressionOptions()
+opts.SetAllowJIT(False)
+value = self.target().EvaluateExpression(expr, opts)
+self.assertTrue(value.GetError().Fail())
+self.assertIn(
+"Can't evaluate the expression without a running target",
+value.GetError().GetCString())
+
+# Evaluating the expression via JIT should work fine.
+value = self.target().EvaluateExpression(expr)
+self.assertSuccess(value.GetError())
Index: lldb/source/Expression/IRInterpreter.cpp
===
--- lldb/source/Expression/IRInterpreter.cpp
+++ lldb/source/Expression/IRInterpreter.cpp
@@ -285,9 +285,11 @@
 return true; // no offset to apply!
 
   SmallVector indices(op_cursor, op_end);
-
   Type *src_elem_ty =
   cast(constant_expr)->getSourceElementType();
+
+  // DataLayout::getIndexedOffsetInType assumes the indices are
+  // instances of ConstantInt.
   uint64_t offset =
   m_target_data.getIndexedOffsetInType(src_elem_ty, indices);
 
@@ -466,12 +468,20 @@
   case Instruction::BitCast:
 return CanResolveConstant(constant_expr->getOperand(0));
   case Instruction::GetElementPtr: {
+// Check that the base can be constant-resolved.
 ConstantExpr::const_op_iterator op_cursor = constant_expr->op_begin();
 Constant *base = dyn_cast(*op_cursor);
-if (!base)
+if (!base || !CanResolveConstant(base))
   return false;
 
-return CanResolveConstant(base);
+// Check that all other operands are just ConstantInt.
+for (Value *op : make_range(constant_expr->op_begin() + 1,
+constant_expr->op_end())) {
+  ConstantInt *constant_int = dyn_cast(op);
+  if (!constant_int)
+return false;
+}
+return true;
   }
   }
 } else {


Index: lldb/test/API/lang/cpp/static_members/TestCPPStaticMembers.py
===
--- lldb/test/API/lang/cpp/static_members/TestCPPStaticMembers.py
+++ lldb/test/API/lang/cpp/static_members/TestCPPStaticMembers.py
@@ -41,3 +41,31 @@
 self.createTestTarget()
 self.expect("expression s_c", error=True,
 startstr="error: use of undeclared identifier 's_d'")
+
+def test_no_crash_in_IR_arithmetic(self):
+"""
+Test that LLDB doesn't crash on evaluating specific expression involving
+pointer arithmetic and taking the address of a static class member.
+See https://bugs.llvm.org/show_bug.cgi?id=52449
+"""
+self.build()
+lldbutil.run_to_source_breakpoint(self, "// stop in main", lldb.SBFileSpec("main.cpp"))
+
+# This expression contains the following IR code:
+# ... i64 ptrtoint (i32* @_ZN1A3s_cE to i64)) ...
+expr = "(int*)100 + (long long)(&A::s_c)"
+
+# The IR interpreter doesn't support non-const operands to the
+# `GetElementPtr` IR instruction, so verify that it correctly fails to
+# evaluate expression.
+opts = lldb.SBExpressionOptions()
+opts.SetAllowJIT(False)
+value = self.target().EvaluateExpression(expr, 

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

[Andy Yankovsky via Phabricator via lldb-commits]

werat added a comment.

In D113498#3305973 , @shafik wrote:

> I believe this a program like this
>
>   int main() {
> int arr[2]{0};
>   
> return arr[1];
>   } 
>
> and an expression like this `expr arr[0]` will give us the constant 
> expression `getelementptr` at least my testing seems to show that.

This example produces the following code for me:

  %4 = load [2 x i32]*, [2 x i32]** %1, align 8
  %arrayidx = getelementptr inbounds [2 x i32], [2 x i32]* %4, i64 0, i64 0
  store i32* %arrayidx, i32** %3, align 8
  br label %init.end

This is successfully interpreted via IRInterpreter with the current patch (i.e. 
all arguments seem to be `ConstantInt`).


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

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

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik accepted this revision.
shafik added a comment.
This revision is now accepted and ready to land.

I believe this a program like this

  int main() {
int arr[2]{0};
  
return arr[1];
  } 

and an expression like this `expr arr[0]` will give us the constant expression 
`getelementptr` at least my testing seems to show that.

Otherwise this looks good.


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

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

[Andy Yankovsky via Phabricator via lldb-commits]

werat added a comment.

@JDevlieghere @shafik friendly ping :)


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

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

[Andy Yankovsky via Phabricator via lldb-commits]

werat added a comment.

@shafik ping :)


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

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

[Andy Yankovsky via Phabricator via lldb-commits]

werat added a comment.

In D113498#3139112 , @shafik wrote:

> On the other hand this is a well-formed constant expression:
>
>   static const int x =0;
>   constexpr const int* ip1 = &x + 1;
>
> Does that get you the test case you need?

This expression seems to be similar to what I have in the test case now.

  (lldb) p static const int x = 0; constexpr const int* ip1 = &x + 1;
  ...
  ; Function Attrs: convergent noinline nounwind optnone
  define dso_local void @"?$__lldb_expr@@YAXPEAX@Z"(i8* %"$__lldb_arg") #0 {
  entry:
%"$__lldb_arg.addr" = alloca i8*, align 8, !clang.decl.ptr !9
%ip1 = alloca i32*, align 8, !clang.decl.ptr !10
store i8* %"$__lldb_arg", i8** %"$__lldb_arg.addr", align 8
store i32* bitcast (i8* getelementptr (i8, i8* bitcast (i32* 
@"?x@?1??$__lldb_expr@@YAXPEAX@Z@4HB" to i8*), i64 4) to i32*), i32** %ip1, 
align 8
ret void
  }

As far as I understand, `getelementptr (i8, i8* bitcast (i32* 
@"?x@?1??$__lldb_expr@@YAXPEAX@Z@4HB" to i8*), i64 4)` is a well-formed 
constant expression, however it has `GlobalVariableVal` as one of it's operand. 
Even though `GlobalVariableVal` is `Constant`, I think it's not always possible 
to resolve its value (please correct me if that's not true, not very familiar 
with IR). Even if there's a way, it's not currently implemented in the 
IRInterpreter -- `CanResolveConstant` will return false when it sees 
`GlobalVariableVal`.


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

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

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added a comment.

I am OOO for a bit but this makes sense and LGTM, let me think about it a bit 
more.


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

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

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added a comment.

So `(int*)100` can't be a constant expression b/c it is basically a 
`reinterpret_cast` and that is forbidden in a constant expression e.g.:

  constexpr const int* ip2 = (int*)100;

is ill-formed.

On the other hand this is a well-formed constant expression:

  static const int x =0;
  constexpr const int* ip1 = &x + 1;

Does that get you the test case you need?


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

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

[Jim Ingham via Phabricator via lldb-commits]

jingham resigned from this revision.
jingham added a comment.

I have also never looked at the IR Interpreter code, and am not particularly 
familiar with llvm IR, sorry.  Maybe Shafik Yaghmour?


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

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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

FYI: I am not familiar enough with the expression parser and IR interpreter 
logic to be able to be able to give the ok here. I hope other expression parser 
experts will continue to give good feedback on this patch!


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

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

[Andy Yankovsky via Phabricator via lldb-commits]

werat marked 2 inline comments as done.
werat added a comment.

In D113498#3137196 , @teemperor wrote:

> 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?

It's not that flexible, it runs against the prebuilt version of LLVM 12 at the 
moment -- https://github.com/google/lldb-eval/releases/tag/oss-fuzz-ubuntu-20.04

> 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.

Sure, I'll try to find someone else for now. Will ping if nobody comes along :)


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

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

[Andy Yankovsky via Phabricator via lldb-commits]

werat updated this revision to Diff 387911.
werat added a comment.

Make the check more strict, only verify that the indexes are ConstantInt.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113498/new/

https://reviews.llvm.org/D113498

Files:
  lldb/source/Expression/IRInterpreter.cpp
  lldb/test/API/lang/cpp/static_members/TestCPPStaticMembers.py


Index: lldb/test/API/lang/cpp/static_members/TestCPPStaticMembers.py
===
--- lldb/test/API/lang/cpp/static_members/TestCPPStaticMembers.py
+++ lldb/test/API/lang/cpp/static_members/TestCPPStaticMembers.py
@@ -41,3 +41,31 @@
 self.createTestTarget()
 self.expect("expression s_c", error=True,
 startstr="error: use of undeclared identifier 's_d'")
+
+def test_no_crash_in_IR_arithmetic(self):
+"""
+Test that LLDB doesn't crash on evaluating specific expression 
involving
+pointer arithmetic and taking the address of a static class member.
+See https://bugs.llvm.org/show_bug.cgi?id=52449
+"""
+self.build()
+lldbutil.run_to_source_breakpoint(self, "// stop in main", 
lldb.SBFileSpec("main.cpp"))
+
+# This expression contains the following IR code:
+# ... i64 ptrtoint (i32* @_ZN1A3s_cE to i64)) ...
+expr = "(int*)100 + (long long)(&A::s_c)"
+
+# The IR interpreter doesn't support non-const operands to the
+# `GetElementPtr` IR instruction, so verify that it correctly fails to
+# evaluate expression.
+opts = lldb.SBExpressionOptions()
+opts.SetAllowJIT(False)
+value = self.target().EvaluateExpression(expr, opts)
+self.assertTrue(value.GetError().Fail())
+self.assertIn(
+"Can't evaluate the expression without a running target",
+value.GetError().GetCString())
+
+# Evaluating the expression via JIT should work fine.
+value = self.target().EvaluateExpression(expr)
+self.assertSuccess(value.GetError())
Index: lldb/source/Expression/IRInterpreter.cpp
===
--- lldb/source/Expression/IRInterpreter.cpp
+++ lldb/source/Expression/IRInterpreter.cpp
@@ -283,9 +283,11 @@
 return true; // no offset to apply!
 
   SmallVector indices(op_cursor, op_end);
-
   Type *src_elem_ty =
   cast(constant_expr)->getSourceElementType();
+
+  // DataLayout::getIndexedOffsetInType assumes the indices are
+  // instances of ConstantInt.
   uint64_t offset =
   m_target_data.getIndexedOffsetInType(src_elem_ty, indices);
 
@@ -465,12 +467,20 @@
   case Instruction::BitCast:
 return CanResolveConstant(constant_expr->getOperand(0));
   case Instruction::GetElementPtr: {
+// Check that the base can be constant-resolved.
 ConstantExpr::const_op_iterator op_cursor = constant_expr->op_begin();
 Constant *base = dyn_cast(*op_cursor);
-if (!base)
+if (!base || !CanResolveConstant(base))
   return false;
 
-return CanResolveConstant(base);
+// Check that all other operands are just ConstantInt.
+for (Value *op : make_range(constant_expr->op_begin() + 1,
+constant_expr->op_end())) {
+  ConstantInt *constant_int = dyn_cast(op);
+  if (!constant_int)
+return false;
+}
+return true;
   }
   }
 } else {


Index: lldb/test/API/lang/cpp/static_members/TestCPPStaticMembers.py
===
--- lldb/test/API/lang/cpp/static_members/TestCPPStaticMembers.py
+++ lldb/test/API/lang/cpp/static_members/TestCPPStaticMembers.py
@@ -41,3 +41,31 @@
 self.createTestTarget()
 self.expect("expression s_c", error=True,
 startstr="error: use of undeclared identifier 's_d'")
+
+def test_no_crash_in_IR_arithmetic(self):
+"""
+Test that LLDB doesn't crash on evaluating specific expression involving
+pointer arithmetic and taking the address of a static class member.
+See https://bugs.llvm.org/show_bug.cgi?id=52449
+"""
+self.build()
+lldbutil.run_to_source_breakpoint(self, "// stop in main", lldb.SBFileSpec("main.cpp"))
+
+# This expression contains the following IR code:
+# ... i64 ptrtoint (i32* @_ZN1A3s_cE to i64)) ...
+expr = "(int*)100 + (long long)(&A::s_c)"
+
+# The IR interpreter doesn't support non-const operands to the
+# `GetElementPtr` IR instruction, so verify that it correctly fails to
+# evaluate expression.
+opts = lldb.SBExpressionOptions()
+opts.SetAllowJIT(False)
+value = self.target().EvaluateExpression(expr, opts)
+self.assertTrue(value.G

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

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

In D113498#3131336 , @werat wrote:

> In 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 , @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(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

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

[Andy Yankovsky via Phabricator via lldb-commits]

werat added a comment.

Btw, this crash in `llvm::DataLayout::getIndexedOffsetInType` was discovered by 
lldb-eval fuzzer a while ago -- 
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=36738


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

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

[Andy Yankovsky via Phabricator via lldb-commits]

werat updated this revision to Diff 387236.
werat added a comment.

Address review comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113498/new/

https://reviews.llvm.org/D113498

Files:
  lldb/source/Expression/IRInterpreter.cpp
  lldb/test/API/lang/cpp/static_members/TestCPPStaticMembers.py


Index: lldb/test/API/lang/cpp/static_members/TestCPPStaticMembers.py
===
--- lldb/test/API/lang/cpp/static_members/TestCPPStaticMembers.py
+++ lldb/test/API/lang/cpp/static_members/TestCPPStaticMembers.py
@@ -41,3 +41,31 @@
 self.createTestTarget()
 self.expect("expression s_c", error=True,
 startstr="error: use of undeclared identifier 's_d'")
+
+def test_no_crash_in_IR_arithmetic(self):
+"""
+Test that LLDB doesn't crash on evaluating specific expression 
involving
+pointer arithmetic and taking the address of a static class member.
+See https://bugs.llvm.org/show_bug.cgi?id=52449
+"""
+self.build()
+lldbutil.run_to_source_breakpoint(self, "// stop in main", 
lldb.SBFileSpec("main.cpp"))
+
+# This expression contains the following IR code:
+# ... i64 ptrtoint (i32* @_ZN1A3s_cE to i64)) ...
+expr = "(int*)100 + (long long)(&A::s_c)"
+
+# The IR interpreter doesn't support non-const operands to the
+# `GetElementPtr` IR instruction, so verify that it correctly fails to
+# evaluate expression.
+opts = lldb.SBExpressionOptions()
+opts.SetAllowJIT(False)
+value = self.target().EvaluateExpression(expr, opts)
+self.assertTrue(value.GetError().Fail())
+self.assertIn(
+"Can't evaluate the expression without a running target",
+value.GetError().GetCString())
+
+# Evaluating the expression via JIT should work fine.
+value = self.target().EvaluateExpression(expr)
+self.assertSuccess(value.GetError())
Index: lldb/source/Expression/IRInterpreter.cpp
===
--- lldb/source/Expression/IRInterpreter.cpp
+++ lldb/source/Expression/IRInterpreter.cpp
@@ -282,12 +282,33 @@
   if (op_cursor == op_end)
 return true; // no offset to apply!
 
+  // DataLayout::getIndexedOffsetInType assumes the indices are
+  // instances of ConstantInt, so we need to resolve them.
   SmallVector indices(op_cursor, op_end);
+  SmallVector resolved_indices;
+
+  for (Value *idx : indices) {
+Constant *constant_idx = dyn_cast(idx);
+if (!constant_idx)
+  return false;
+
+ConstantInt *constant_int = dyn_cast(constant_idx);
+if (!constant_int) {
+  APInt v;
+  if (!ResolveConstantValue(v, constant_idx))
+return false;
+
+  constant_int =
+  cast(ConstantInt::get(idx->getType(), v));
+}
+
+resolved_indices.push_back(constant_int);
+  }
 
   Type *src_elem_ty =
   cast(constant_expr)->getSourceElementType();
-  uint64_t offset =
-  m_target_data.getIndexedOffsetInType(src_elem_ty, indices);
+  uint64_t offset = m_target_data.getIndexedOffsetInType(
+  src_elem_ty, resolved_indices);
 
   const bool is_signed = true;
   value += APInt(value.getBitWidth(), offset, is_signed);
@@ -465,12 +486,17 @@
   case Instruction::BitCast:
 return CanResolveConstant(constant_expr->getOperand(0));
   case Instruction::GetElementPtr: {
-ConstantExpr::const_op_iterator op_cursor = constant_expr->op_begin();
-Constant *base = dyn_cast(*op_cursor);
-if (!base)
-  return false;
-
-return CanResolveConstant(base);
+// Check that all operands of `getelementptr` can be constant-resolved.
+SmallVector operands(constant_expr->op_begin(),
+ constant_expr->op_end());
+for (Value *op : operands) {
+  Constant *constant_op = dyn_cast(op);
+  if (!constant_op)
+return false;
+  if (!CanResolveConstant(constant_op))
+return false;
+}
+return true;
   }
   }
 } else {


Index: lldb/test/API/lang/cpp/static_members/TestCPPStaticMembers.py
===
--- lldb/test/API/lang/cpp/static_members/TestCPPStaticMembers.py
+++ lldb/test/API/lang/cpp/static_members/TestCPPStaticMembers.py
@@ -41,3 +41,31 @@
 self.createTestTarget()
 self.expect("expression s_c", error=True,
 startstr="error: use of undeclared identifier 's_d'")
+
+def test_no_crash_in_IR_arithmetic(self):
+"""
+Test that LLDB doesn't crash o

[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 , @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 , @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(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

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

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a reviewer: LLDB.
teemperor added a comment.

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). We could also split out 
the interpreting change and then this is good to go.

Also I think a second set of eyes on this would be nice as I rarely touch the 
IRInterpreter, but not sure who the best person for that is. I'll add the LLDB 
group and let's see if anyone has a second opinion on this, but in general this 
LGTM module the test situation.




Comment at: lldb/source/Expression/IRInterpreter.cpp:286
   SmallVector indices(op_cursor, op_end);
+  SmallVector const_indices;
+

Maybe `resolved_indices`? `const_` always sounds a bit like it's meaning 
'const' qualified version of indices or so.



Comment at: lldb/source/Expression/IRInterpreter.cpp:288
+
+  for (Value *idx : indices) {
+Constant *constant_idx = dyn_cast(idx);

I think this deserves a comment that `getIndexedOffsetInType` can only handle 
ConstantInt indices (and that's why we're resolving them here).



Comment at: lldb/source/Expression/IRInterpreter.cpp:490
+ constant_expr->op_end());
+for (Value *op : operands) {
+  Constant *constant_op = dyn_cast(op);

`for (Value *op : constant_expr->ops())` ?



Comment at: lldb/source/Expression/IRInterpreter.cpp:494
+return false;
+  if (!CanResolveConstant(constant_op)) {
+return false;

nit no `{}` for single line ifs



Comment at: lldb/test/API/lang/cpp/static_members/TestCPPStaticMembers.py:71
+value = self.target().EvaluateExpression(expr)
+self.assertTrue(value.GetError().Success())

`self.assertSuccess` (that will print the error on failure to the test log)


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

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

[Andy Yankovsky via Phabricator via lldb-commits]

werat updated this revision to Diff 385892.
werat added a comment.
Herald added a subscriber: JDevlieghere.

Remove accidental code


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113498/new/

https://reviews.llvm.org/D113498

Files:
  lldb/source/Expression/IRInterpreter.cpp
  lldb/test/API/lang/cpp/static_members/TestCPPStaticMembers.py


Index: lldb/test/API/lang/cpp/static_members/TestCPPStaticMembers.py
===
--- lldb/test/API/lang/cpp/static_members/TestCPPStaticMembers.py
+++ lldb/test/API/lang/cpp/static_members/TestCPPStaticMembers.py
@@ -41,3 +41,31 @@
 self.createTestTarget()
 self.expect("expression s_c", error=True,
 startstr="error: use of undeclared identifier 's_d'")
+
+def test_no_crash_in_IR_arithmetic(self):
+"""
+Test that LLDB doesn't crash on evaluating specific expression 
involving
+pointer arithmetic and taking the address of a static class member.
+See https://bugs.llvm.org/show_bug.cgi?id=52449
+"""
+self.build()
+lldbutil.run_to_source_breakpoint(self, "// stop in main", 
lldb.SBFileSpec("main.cpp"))
+
+# This expression contains the following IR code:
+# ... i64 ptrtoint (i32* @_ZN1A3s_cE to i64)) ...
+expr = "(int*)100 + (long long)(&A::s_c)"
+
+# The IR interpreter doesn't support non-const operands to the
+# `GetElementPtr` IR instruction, so verify that it correctly fails to
+# evaluate expression.
+opts = lldb.SBExpressionOptions()
+opts.SetAllowJIT(False)
+value = self.target().EvaluateExpression(expr, opts)
+self.assertTrue(value.GetError().Fail())
+self.assertIn(
+"Can't evaluate the expression without a running target",
+value.GetError().GetCString())
+
+# Evaluating the expression via JIT should work fine.
+value = self.target().EvaluateExpression(expr)
+self.assertTrue(value.GetError().Success())
Index: lldb/source/Expression/IRInterpreter.cpp
===
--- lldb/source/Expression/IRInterpreter.cpp
+++ lldb/source/Expression/IRInterpreter.cpp
@@ -283,11 +283,30 @@
 return true; // no offset to apply!
 
   SmallVector indices(op_cursor, op_end);
+  SmallVector const_indices;
+
+  for (Value *idx : indices) {
+Constant *constant_idx = dyn_cast(idx);
+if (!constant_idx)
+  return false;
+
+ConstantInt *constant_int = dyn_cast(constant_idx);
+if (!constant_int) {
+  APInt v;
+  if (!ResolveConstantValue(v, constant_idx))
+return false;
+
+  constant_int =
+  cast(ConstantInt::get(idx->getType(), v));
+}
+
+const_indices.push_back(constant_int);
+  }
 
   Type *src_elem_ty =
   cast(constant_expr)->getSourceElementType();
   uint64_t offset =
-  m_target_data.getIndexedOffsetInType(src_elem_ty, indices);
+  m_target_data.getIndexedOffsetInType(src_elem_ty, const_indices);
 
   const bool is_signed = true;
   value += APInt(value.getBitWidth(), offset, is_signed);
@@ -465,12 +484,18 @@
   case Instruction::BitCast:
 return CanResolveConstant(constant_expr->getOperand(0));
   case Instruction::GetElementPtr: {
-ConstantExpr::const_op_iterator op_cursor = constant_expr->op_begin();
-Constant *base = dyn_cast(*op_cursor);
-if (!base)
-  return false;
-
-return CanResolveConstant(base);
+// Check that all operands of `getelementptr` can be constant-resolved.
+SmallVector operands(constant_expr->op_begin(),
+ constant_expr->op_end());
+for (Value *op : operands) {
+  Constant *constant_op = dyn_cast(op);
+  if (!constant_op)
+return false;
+  if (!CanResolveConstant(constant_op)) {
+return false;
+  }
+}
+return true;
   }
   }
 } else {


Index: lldb/test/API/lang/cpp/static_members/TestCPPStaticMembers.py
===
--- lldb/test/API/lang/cpp/static_members/TestCPPStaticMembers.py
+++ lldb/test/API/lang/cpp/static_members/TestCPPStaticMembers.py
@@ -41,3 +41,31 @@
 self.createTestTarget()
 self.expect("expression s_c", error=True,
 startstr="error: use of undeclared identifier 's_d'")
+
+def test_no_crash_in_IR_arithmetic(self):
+"""
+Test that LLDB doesn't crash on evaluating specific expression involving
+pointer arithmetic and taking the address of a static class member.
+See https://bugs.llvm.org

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

[Andy Yankovsky via Phabricator via lldb-commits]

werat created this revision.
werat requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Operands to `getelementptr` can be constants or constant expressions. Check
that all operands can be constant-resolved and resolve them during the
evaluation. If some operands can't be resolved as constants -- the expression
evaluation will fallback to JIT.

Fixes: https://bugs.llvm.org/show_bug.cgi?id=52449


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D113498

Files:
  lldb/source/Expression/IRInterpreter.cpp
  lldb/test/API/lang/cpp/static_members/TestCPPStaticMembers.py
  lldb/test/API/lang/cpp/static_members/main.cpp

Index: lldb/test/API/lang/cpp/static_members/main.cpp
===
--- lldb/test/API/lang/cpp/static_members/main.cpp
+++ lldb/test/API/lang/cpp/static_members/main.cpp
@@ -11,6 +11,14 @@
 long A::s_b = 2;
 int A::s_c = 3;
 
+// class Foo
+// {
+// public:
+// static int Bar;
+// };
+
+// int Foo::Bar = 10;
+
 int main() {
   A my_a;
   my_a.m_a = 1;
Index: lldb/test/API/lang/cpp/static_members/TestCPPStaticMembers.py
===
--- lldb/test/API/lang/cpp/static_members/TestCPPStaticMembers.py
+++ lldb/test/API/lang/cpp/static_members/TestCPPStaticMembers.py
@@ -41,3 +41,31 @@
 self.createTestTarget()
 self.expect("expression s_c", error=True,
 startstr="error: use of undeclared identifier 's_d'")
+
+def test_no_crash_in_IR_arithmetic(self):
+"""
+Test that LLDB doesn't crash on evaluating specific expression involving
+pointer arithmetic and taking the address of a static class member.
+See https://bugs.llvm.org/show_bug.cgi?id=52449
+"""
+self.build()
+lldbutil.run_to_source_breakpoint(self, "// stop in main", lldb.SBFileSpec("main.cpp"))
+
+# This expression contains the following IR code:
+# ... i64 ptrtoint (i32* @_ZN1A3s_cE to i64)) ...
+expr = "(int*)100 + (long long)(&A::s_c)"
+
+# The IR interpreter doesn't support non-const operands to the
+# `GetElementPtr` IR instruction, so verify that it correctly fails to
+# evaluate expression.
+opts = lldb.SBExpressionOptions()
+opts.SetAllowJIT(False)
+value = self.target().EvaluateExpression(expr, opts)
+self.assertTrue(value.GetError().Fail())
+self.assertIn(
+"Can't evaluate the expression without a running target",
+value.GetError().GetCString())
+
+# Evaluating the expression via JIT should work fine.
+value = self.target().EvaluateExpression(expr)
+self.assertTrue(value.GetError().Success())
Index: lldb/source/Expression/IRInterpreter.cpp
===
--- lldb/source/Expression/IRInterpreter.cpp
+++ lldb/source/Expression/IRInterpreter.cpp
@@ -283,11 +283,30 @@
 return true; // no offset to apply!
 
   SmallVector indices(op_cursor, op_end);
+  SmallVector const_indices;
+
+  for (Value *idx : indices) {
+Constant *constant_idx = dyn_cast(idx);
+if (!constant_idx)
+  return false;
+
+ConstantInt *constant_int = dyn_cast(constant_idx);
+if (!constant_int) {
+  APInt v;
+  if (!ResolveConstantValue(v, constant_idx))
+return false;
+
+  constant_int =
+  cast(ConstantInt::get(idx->getType(), v));
+}
+
+const_indices.push_back(constant_int);
+  }
 
   Type *src_elem_ty =
   cast(constant_expr)->getSourceElementType();
   uint64_t offset =
-  m_target_data.getIndexedOffsetInType(src_elem_ty, indices);
+  m_target_data.getIndexedOffsetInType(src_elem_ty, const_indices);
 
   const bool is_signed = true;
   value += APInt(value.getBitWidth(), offset, is_signed);
@@ -465,12 +484,18 @@
   case Instruction::BitCast:
 return CanResolveConstant(constant_expr->getOperand(0));
   case Instruction::GetElementPtr: {
-ConstantExpr::const_op_iterator op_cursor = constant_expr->op_begin();
-Constant *base = dyn_cast(*op_cursor);
-if (!base)
-  return false;
-
-return CanResolveConstant(base);
+// Check that all operands of `getelementptr` can be constant-resolved.
+SmallVector operands(constant_expr->op_begin(),
+ constant_expr->op_end());
+for (Value *op : operands) {
+  Constant *constant_op = dyn_cast(op);
+  if (!constant_op)
+return false;
+  if (!CanResolveConstant(constant_op)) {
+return false;
+  }
+}
+return true;
   }
   }
 }