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<Value *, 8> indices(op_cursor, op_end);
+ SmallVector<Value *, 8> resolved_indices;
+
+ for (Value *idx : indices) {
+ Constant *constant_idx = dyn_cast<Constant>(idx);
+ if (!constant_idx)
+ return false;
+
+ ConstantInt *constant_int = dyn_cast<ConstantInt>(constant_idx);
+ if (!constant_int) {
+ APInt v;
+ if (!ResolveConstantValue(v, constant_idx))
+ return false;
+
+ constant_int =
+ cast<ConstantInt>(ConstantInt::get(idx->getType(), v));
+ }
+
+ resolved_indices.push_back(constant_int);
+ }
Type *src_elem_ty =
cast<GEPOperator>(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<Constant>(*op_cursor);
- if (!base)
- return false;
-
- return CanResolveConstant(base);
+ // Check that all operands of `getelementptr` can be constant-resolved.
+ SmallVector<Value *, 8> operands(constant_expr->op_begin(),
+ constant_expr->op_end());
+ for (Value *op : operands) {
+ Constant *constant_op = dyn_cast<Constant>(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/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<Value *, 8> indices(op_cursor, op_end);
+ SmallVector<Value *, 8> resolved_indices;
+
+ for (Value *idx : indices) {
+ Constant *constant_idx = dyn_cast<Constant>(idx);
+ if (!constant_idx)
+ return false;
+
+ ConstantInt *constant_int = dyn_cast<ConstantInt>(constant_idx);
+ if (!constant_int) {
+ APInt v;
+ if (!ResolveConstantValue(v, constant_idx))
+ return false;
+
+ constant_int =
+ cast<ConstantInt>(ConstantInt::get(idx->getType(), v));
+ }
+
+ resolved_indices.push_back(constant_int);
+ }
Type *src_elem_ty =
cast<GEPOperator>(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<Constant>(*op_cursor);
- if (!base)
- return false;
-
- return CanResolveConstant(base);
+ // Check that all operands of `getelementptr` can be constant-resolved.
+ SmallVector<Value *, 8> operands(constant_expr->op_begin(),
+ constant_expr->op_end());
+ for (Value *op : operands) {
+ Constant *constant_op = dyn_cast<Constant>(op);
+ if (!constant_op)
+ return false;
+ if (!CanResolveConstant(constant_op))
+ return false;
+ }
+ return true;
}
}
} else {
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
