[Lldb-commits] [PATCH] D155198: [lldb] Consider OP_addrx in DWARFExpression::Update_DW_OP_addr
clayborg added a comment. Sorry for the late response, but I had verbally agreed to this approach when Felipe and I spoke about how to fix this. LGTM! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155198/new/ https://reviews.llvm.org/D155198 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D155198: [lldb] Consider OP_addrx in DWARFExpression::Update_DW_OP_addr
This revision was automatically updated to reflect the committed changes.
Closed by commit rGf09f0a6b1076: [lldb] Consider OP_addrx in
DWARFExpression::Update_DW_OP_addr (authored by fdeazeve, committed by aprantl).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D155198/new/
https://reviews.llvm.org/D155198
Files:
lldb/source/Expression/DWARFExpression.cpp
lldb/unittests/Expression/DWARFExpressionTest.cpp
Index: lldb/unittests/Expression/DWARFExpressionTest.cpp
===
--- lldb/unittests/Expression/DWARFExpressionTest.cpp
+++ lldb/unittests/Expression/DWARFExpressionTest.cpp
@@ -522,6 +522,11 @@
ASSERT_TRUE(evaluate(expr, status, result)) << status.ToError();
ASSERT_EQ(result.GetValueType(), Value::ValueType::LoadAddress);
ASSERT_EQ(result.GetScalar().UInt(), 0x5678u);
+
+ ASSERT_TRUE(expr.Update_DW_OP_addr(dwarf_cu, 0xdeadbeef));
+ ASSERT_TRUE(evaluate(expr, status, result)) << status.ToError();
+ ASSERT_EQ(result.GetValueType(), Value::ValueType::LoadAddress);
+ ASSERT_EQ(result.GetScalar().UInt(), 0xdeadbeefu);
}
class CustomSymbolFileDWARF : public SymbolFileDWARF {
Index: lldb/source/Expression/DWARFExpression.cpp
===
--- lldb/source/Expression/DWARFExpression.cpp
+++ lldb/source/Expression/DWARFExpression.cpp
@@ -408,13 +408,33 @@
// the heap data so "m_data" will now correctly manage the heap data.
m_data.SetData(encoder.GetDataBuffer());
return true;
-} else {
- const offset_t op_arg_size =
- GetOpcodeDataSize(m_data, offset, op, dwarf_cu);
- if (op_arg_size == LLDB_INVALID_OFFSET)
-break;
- offset += op_arg_size;
}
+if (op == DW_OP_addrx) {
+ // Replace DW_OP_addrx with DW_OP_addr, since we can't modify the
+ // read-only debug_addr table.
+ // Subtract one to account for the opcode.
+ llvm::ArrayRef data_before_op = m_data.GetData().take_front(offset - 1);
+
+ // Read the addrx index to determine how many bytes it needs.
+ const lldb::offset_t old_offset = offset;
+ m_data.GetULEB128();
+ if (old_offset == offset)
+return false;
+ llvm::ArrayRef data_after_op = m_data.GetData().drop_front(offset);
+
+ DataEncoder encoder(m_data.GetByteOrder(), m_data.GetAddressByteSize());
+ encoder.AppendData(data_before_op);
+ encoder.AppendU8(DW_OP_addr);
+ encoder.AppendAddress(file_addr);
+ encoder.AppendData(data_after_op);
+ m_data.SetData(encoder.GetDataBuffer());
+ return true;
+}
+const offset_t op_arg_size =
+GetOpcodeDataSize(m_data, offset, op, dwarf_cu);
+if (op_arg_size == LLDB_INVALID_OFFSET)
+ break;
+offset += op_arg_size;
}
return false;
}
Index: lldb/unittests/Expression/DWARFExpressionTest.cpp
===
--- lldb/unittests/Expression/DWARFExpressionTest.cpp
+++ lldb/unittests/Expression/DWARFExpressionTest.cpp
@@ -522,6 +522,11 @@
ASSERT_TRUE(evaluate(expr, status, result)) << status.ToError();
ASSERT_EQ(result.GetValueType(), Value::ValueType::LoadAddress);
ASSERT_EQ(result.GetScalar().UInt(), 0x5678u);
+
+ ASSERT_TRUE(expr.Update_DW_OP_addr(dwarf_cu, 0xdeadbeef));
+ ASSERT_TRUE(evaluate(expr, status, result)) << status.ToError();
+ ASSERT_EQ(result.GetValueType(), Value::ValueType::LoadAddress);
+ ASSERT_EQ(result.GetScalar().UInt(), 0xdeadbeefu);
}
class CustomSymbolFileDWARF : public SymbolFileDWARF {
Index: lldb/source/Expression/DWARFExpression.cpp
===
--- lldb/source/Expression/DWARFExpression.cpp
+++ lldb/source/Expression/DWARFExpression.cpp
@@ -408,13 +408,33 @@
// the heap data so "m_data" will now correctly manage the heap data.
m_data.SetData(encoder.GetDataBuffer());
return true;
-} else {
- const offset_t op_arg_size =
- GetOpcodeDataSize(m_data, offset, op, dwarf_cu);
- if (op_arg_size == LLDB_INVALID_OFFSET)
-break;
- offset += op_arg_size;
}
+if (op == DW_OP_addrx) {
+ // Replace DW_OP_addrx with DW_OP_addr, since we can't modify the
+ // read-only debug_addr table.
+ // Subtract one to account for the opcode.
+ llvm::ArrayRef data_before_op = m_data.GetData().take_front(offset - 1);
+
+ // Read the addrx index to determine how many bytes it needs.
+ const lldb::offset_t old_offset = offset;
+ m_data.GetULEB128();
+ if (old_offset == offset)
+return false;
+ llvm::ArrayRef data_after_op = m_data.GetData().drop_front(offset);
+
+ DataEncoder encoder(m_data.GetByteOrder(), m_data.GetAddressByteSize());
+ encoder.AppendData(data_before_op);
+ encoder.AppendU8(DW_OP_addr);
+ encoder.AppendAddress(file_addr);
+
[Lldb-commits] [PATCH] D155198: [lldb] Consider OP_addrx in DWARFExpression::Update_DW_OP_addr
JDevlieghere accepted this revision as: JDevlieghere. JDevlieghere added a comment. LGTM. I agree with Adrian: we can go ahead and land this to unblock the bots. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155198/new/ https://reviews.llvm.org/D155198 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D155198: [lldb] Consider OP_addrx in DWARFExpression::Update_DW_OP_addr
aprantl added a comment. As far as I'm concerned, this doesn't look too controversial to me and it is unblocking one of the bots. I think it would be okay to tentatively land this, but be on the lookout and promptly react to any post-commit feedback from @clayborg. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155198/new/ https://reviews.llvm.org/D155198 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D155198: [lldb] Consider OP_addrx in DWARFExpression::Update_DW_OP_addr
fdeazeve added a comment. Hi @clayborg, just wanted to make sure this doesn't fall off your radar :) This fixes all outstanding DWARF 5 issues with Debug Maps, so it would be cool if we could get this in. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155198/new/ https://reviews.llvm.org/D155198 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D155198: [lldb] Consider OP_addrx in DWARFExpression::Update_DW_OP_addr
fdeazeve updated this revision to Diff 541986.
fdeazeve added a comment.
Rebased
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D155198/new/
https://reviews.llvm.org/D155198
Files:
lldb/source/Expression/DWARFExpression.cpp
lldb/unittests/Expression/DWARFExpressionTest.cpp
Index: lldb/unittests/Expression/DWARFExpressionTest.cpp
===
--- lldb/unittests/Expression/DWARFExpressionTest.cpp
+++ lldb/unittests/Expression/DWARFExpressionTest.cpp
@@ -522,6 +522,11 @@
ASSERT_TRUE(evaluate(expr, status, result)) << status.ToError();
ASSERT_EQ(result.GetValueType(), Value::ValueType::LoadAddress);
ASSERT_EQ(result.GetScalar().UInt(), 0x5678u);
+
+ ASSERT_TRUE(expr.Update_DW_OP_addr(dwarf_cu, 0xdeadbeef));
+ ASSERT_TRUE(evaluate(expr, status, result)) << status.ToError();
+ ASSERT_EQ(result.GetValueType(), Value::ValueType::LoadAddress);
+ ASSERT_EQ(result.GetScalar().UInt(), 0xdeadbeefu);
}
class CustomSymbolFileDWARF : public SymbolFileDWARF {
Index: lldb/source/Expression/DWARFExpression.cpp
===
--- lldb/source/Expression/DWARFExpression.cpp
+++ lldb/source/Expression/DWARFExpression.cpp
@@ -408,13 +408,33 @@
// the heap data so "m_data" will now correctly manage the heap data.
m_data.SetData(encoder.GetDataBuffer());
return true;
-} else {
- const offset_t op_arg_size =
- GetOpcodeDataSize(m_data, offset, op, dwarf_cu);
- if (op_arg_size == LLDB_INVALID_OFFSET)
-break;
- offset += op_arg_size;
}
+if (op == DW_OP_addrx) {
+ // Replace DW_OP_addrx with DW_OP_addr, since we can't modify the
+ // read-only debug_addr table.
+ // Subtract one to account for the opcode.
+ llvm::ArrayRef data_before_op = m_data.GetData().take_front(offset - 1);
+
+ // Read the addrx index to determine how many bytes it needs.
+ const lldb::offset_t old_offset = offset;
+ m_data.GetULEB128();
+ if (old_offset == offset)
+return false;
+ llvm::ArrayRef data_after_op = m_data.GetData().drop_front(offset);
+
+ DataEncoder encoder(m_data.GetByteOrder(), m_data.GetAddressByteSize());
+ encoder.AppendData(data_before_op);
+ encoder.AppendU8(DW_OP_addr);
+ encoder.AppendAddress(file_addr);
+ encoder.AppendData(data_after_op);
+ m_data.SetData(encoder.GetDataBuffer());
+ return true;
+}
+const offset_t op_arg_size =
+GetOpcodeDataSize(m_data, offset, op, dwarf_cu);
+if (op_arg_size == LLDB_INVALID_OFFSET)
+ break;
+offset += op_arg_size;
}
return false;
}
Index: lldb/unittests/Expression/DWARFExpressionTest.cpp
===
--- lldb/unittests/Expression/DWARFExpressionTest.cpp
+++ lldb/unittests/Expression/DWARFExpressionTest.cpp
@@ -522,6 +522,11 @@
ASSERT_TRUE(evaluate(expr, status, result)) << status.ToError();
ASSERT_EQ(result.GetValueType(), Value::ValueType::LoadAddress);
ASSERT_EQ(result.GetScalar().UInt(), 0x5678u);
+
+ ASSERT_TRUE(expr.Update_DW_OP_addr(dwarf_cu, 0xdeadbeef));
+ ASSERT_TRUE(evaluate(expr, status, result)) << status.ToError();
+ ASSERT_EQ(result.GetValueType(), Value::ValueType::LoadAddress);
+ ASSERT_EQ(result.GetScalar().UInt(), 0xdeadbeefu);
}
class CustomSymbolFileDWARF : public SymbolFileDWARF {
Index: lldb/source/Expression/DWARFExpression.cpp
===
--- lldb/source/Expression/DWARFExpression.cpp
+++ lldb/source/Expression/DWARFExpression.cpp
@@ -408,13 +408,33 @@
// the heap data so "m_data" will now correctly manage the heap data.
m_data.SetData(encoder.GetDataBuffer());
return true;
-} else {
- const offset_t op_arg_size =
- GetOpcodeDataSize(m_data, offset, op, dwarf_cu);
- if (op_arg_size == LLDB_INVALID_OFFSET)
-break;
- offset += op_arg_size;
}
+if (op == DW_OP_addrx) {
+ // Replace DW_OP_addrx with DW_OP_addr, since we can't modify the
+ // read-only debug_addr table.
+ // Subtract one to account for the opcode.
+ llvm::ArrayRef data_before_op = m_data.GetData().take_front(offset - 1);
+
+ // Read the addrx index to determine how many bytes it needs.
+ const lldb::offset_t old_offset = offset;
+ m_data.GetULEB128();
+ if (old_offset == offset)
+return false;
+ llvm::ArrayRef data_after_op = m_data.GetData().drop_front(offset);
+
+ DataEncoder encoder(m_data.GetByteOrder(), m_data.GetAddressByteSize());
+ encoder.AppendData(data_before_op);
+ encoder.AppendU8(DW_OP_addr);
+ encoder.AppendAddress(file_addr);
+ encoder.AppendData(data_after_op);
+ m_data.SetData(encoder.GetDataBuffer());
+ return true;
+}
+const offset_t op_arg_size =
[Lldb-commits] [PATCH] D155198: [lldb] Consider OP_addrx in DWARFExpression::Update_DW_OP_addr
fdeazeve updated this revision to Diff 540001.
fdeazeve added a comment.
Minor comment improvements
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D155198/new/
https://reviews.llvm.org/D155198
Files:
lldb/source/Expression/DWARFExpression.cpp
lldb/unittests/Expression/DWARFExpressionTest.cpp
Index: lldb/unittests/Expression/DWARFExpressionTest.cpp
===
--- lldb/unittests/Expression/DWARFExpressionTest.cpp
+++ lldb/unittests/Expression/DWARFExpressionTest.cpp
@@ -522,6 +522,11 @@
ASSERT_TRUE(evaluate(expr, status, result)) << status.ToError();
ASSERT_EQ(result.GetValueType(), Value::ValueType::LoadAddress);
ASSERT_EQ(result.GetScalar().UInt(), 0x5678u);
+
+ ASSERT_TRUE(expr.Update_DW_OP_addr(dwarf_cu, 0xdeadbeef));
+ ASSERT_TRUE(evaluate(expr, status, result)) << status.ToError();
+ ASSERT_EQ(result.GetValueType(), Value::ValueType::LoadAddress);
+ ASSERT_EQ(result.GetScalar().UInt(), 0xdeadbeefu);
}
class CustomSymbolFileDWARF : public SymbolFileDWARF {
Index: lldb/source/Expression/DWARFExpression.cpp
===
--- lldb/source/Expression/DWARFExpression.cpp
+++ lldb/source/Expression/DWARFExpression.cpp
@@ -408,13 +408,33 @@
// the heap data so "m_data" will now correctly manage the heap data.
m_data.SetData(encoder.GetDataBuffer());
return true;
-} else {
- const offset_t op_arg_size =
- GetOpcodeDataSize(m_data, offset, op, dwarf_cu);
- if (op_arg_size == LLDB_INVALID_OFFSET)
-break;
- offset += op_arg_size;
}
+if (op == DW_OP_addrx) {
+ // Replace DW_OP_addrx with DW_OP_addr, since we can't modify the
+ // read-only debug_addr table.
+ // Subtract one to account for the opcode.
+ llvm::ArrayRef data_before_op = m_data.GetData().take_front(offset - 1);
+
+ // Read the addrx index to determine how many bytes it needs.
+ const lldb::offset_t old_offset = offset;
+ m_data.GetULEB128();
+ if (old_offset == offset)
+return false;
+ llvm::ArrayRef data_after_op = m_data.GetData().drop_front(offset);
+
+ DataEncoder encoder(m_data.GetByteOrder(), m_data.GetAddressByteSize());
+ encoder.AppendData(data_before_op);
+ encoder.AppendU8(DW_OP_addr);
+ encoder.AppendAddress(file_addr);
+ encoder.AppendData(data_after_op);
+ m_data.SetData(encoder.GetDataBuffer());
+ return true;
+}
+const offset_t op_arg_size =
+GetOpcodeDataSize(m_data, offset, op, dwarf_cu);
+if (op_arg_size == LLDB_INVALID_OFFSET)
+ break;
+offset += op_arg_size;
}
return false;
}
Index: lldb/unittests/Expression/DWARFExpressionTest.cpp
===
--- lldb/unittests/Expression/DWARFExpressionTest.cpp
+++ lldb/unittests/Expression/DWARFExpressionTest.cpp
@@ -522,6 +522,11 @@
ASSERT_TRUE(evaluate(expr, status, result)) << status.ToError();
ASSERT_EQ(result.GetValueType(), Value::ValueType::LoadAddress);
ASSERT_EQ(result.GetScalar().UInt(), 0x5678u);
+
+ ASSERT_TRUE(expr.Update_DW_OP_addr(dwarf_cu, 0xdeadbeef));
+ ASSERT_TRUE(evaluate(expr, status, result)) << status.ToError();
+ ASSERT_EQ(result.GetValueType(), Value::ValueType::LoadAddress);
+ ASSERT_EQ(result.GetScalar().UInt(), 0xdeadbeefu);
}
class CustomSymbolFileDWARF : public SymbolFileDWARF {
Index: lldb/source/Expression/DWARFExpression.cpp
===
--- lldb/source/Expression/DWARFExpression.cpp
+++ lldb/source/Expression/DWARFExpression.cpp
@@ -408,13 +408,33 @@
// the heap data so "m_data" will now correctly manage the heap data.
m_data.SetData(encoder.GetDataBuffer());
return true;
-} else {
- const offset_t op_arg_size =
- GetOpcodeDataSize(m_data, offset, op, dwarf_cu);
- if (op_arg_size == LLDB_INVALID_OFFSET)
-break;
- offset += op_arg_size;
}
+if (op == DW_OP_addrx) {
+ // Replace DW_OP_addrx with DW_OP_addr, since we can't modify the
+ // read-only debug_addr table.
+ // Subtract one to account for the opcode.
+ llvm::ArrayRef data_before_op = m_data.GetData().take_front(offset - 1);
+
+ // Read the addrx index to determine how many bytes it needs.
+ const lldb::offset_t old_offset = offset;
+ m_data.GetULEB128();
+ if (old_offset == offset)
+return false;
+ llvm::ArrayRef data_after_op = m_data.GetData().drop_front(offset);
+
+ DataEncoder encoder(m_data.GetByteOrder(), m_data.GetAddressByteSize());
+ encoder.AppendData(data_before_op);
+ encoder.AppendU8(DW_OP_addr);
+ encoder.AppendAddress(file_addr);
+ encoder.AppendData(data_after_op);
+ m_data.SetData(encoder.GetDataBuffer());
+ return true;
+}
+const
[Lldb-commits] [PATCH] D155198: [lldb] Consider OP_addrx in DWARFExpression::Update_DW_OP_addr
fdeazeve created this revision.
Herald added a project: All.
fdeazeve requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
This rewrites DW_OP_addrx inside DWARFExpression as an DW_OP_addr so that we can
update addresses that are originally located in the debug_addr section.
The full discussion behind this can be found in
https://discourse.llvm.org/t/dwarfexpression-and-dw-op-addrx/71627/12, but a
summary follows.
When SymbolFileDWARF::ParseVariableDIE creates DWARFExpressions for variables
whose location is an OP_addr, it knows how to remap addresses appropriately in
the DebugMap case. It then calls DWARFExpression::Update_DW_OP_addr, which
updates the value associated with OP_addr.
However, when we have an OP_addrx, the update function does nothing. This makes
sense, as the DWARFExpression does not have a mutable view of the debug_addr
section. In non-DebugMap flows this is not an issue, as the debug_addr contains
the correct addresses of variables. In DebugMap flows, this is problematic
because the work done by RelinkOSOAddress is lost. By updating the OP to
OP_addr, we can also update the address as required,
We also explored the alternative of relinking the debug_addr section when we are
initializing OSOs (InitOSO). However, this creates an inconsistent story for
users of DWARFExpression::GetLocation_DW_OP_addr. This function returns an
address without telling callers whether that address came from an OP_addr or an
OP_addrx. If we preemptively relink OP_addrx results without doing the same for
OP_addr results, then callers can’t know whether the address they got was an
executable address or an object file address. In other words, they can’t know
whether they need to call LinkOSOFileAddress on those results or not.
This patch addresses the majority of test failures when enabling DWARF 5 for
MachO (over 200 test failures).
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D155198
Files:
lldb/source/Expression/DWARFExpression.cpp
lldb/unittests/Expression/DWARFExpressionTest.cpp
Index: lldb/unittests/Expression/DWARFExpressionTest.cpp
===
--- lldb/unittests/Expression/DWARFExpressionTest.cpp
+++ lldb/unittests/Expression/DWARFExpressionTest.cpp
@@ -522,6 +522,11 @@
ASSERT_TRUE(evaluate(expr, status, result)) << status.ToError();
ASSERT_EQ(result.GetValueType(), Value::ValueType::LoadAddress);
ASSERT_EQ(result.GetScalar().UInt(), 0x5678u);
+
+ ASSERT_TRUE(expr.Update_DW_OP_addr(dwarf_cu, 0xdeadbeef));
+ ASSERT_TRUE(evaluate(expr, status, result)) << status.ToError();
+ ASSERT_EQ(result.GetValueType(), Value::ValueType::LoadAddress);
+ ASSERT_EQ(result.GetScalar().UInt(), 0xdeadbeefu);
}
class CustomSymbolFileDWARF : public SymbolFileDWARF {
Index: lldb/source/Expression/DWARFExpression.cpp
===
--- lldb/source/Expression/DWARFExpression.cpp
+++ lldb/source/Expression/DWARFExpression.cpp
@@ -408,13 +408,32 @@
// the heap data so "m_data" will now correctly manage the heap data.
m_data.SetData(encoder.GetDataBuffer());
return true;
-} else {
- const offset_t op_arg_size =
- GetOpcodeDataSize(m_data, offset, op, dwarf_cu);
- if (op_arg_size == LLDB_INVALID_OFFSET)
-break;
- offset += op_arg_size;
}
+if (op == DW_OP_addrx) {
+ // Replace DW_OP_addrx with DW_OP_addr, since we can't modify the
+ // read-only debug_addr table.
+ llvm::ArrayRef data_before_op = m_data.GetData().take_front(offset - 1);
+
+ // Read the addrx index to determine how many bytes it needs.
+ const lldb::offset_t old_offset = offset;
+ m_data.GetULEB128();
+ if (old_offset == offset)
+return false;
+ llvm::ArrayRef data_after_op = m_data.GetData().drop_front(offset);
+
+ DataEncoder encoder(m_data.GetByteOrder(), m_data.GetAddressByteSize());
+ encoder.AppendData(data_before_op);
+ encoder.AppendU8(DW_OP_addr);
+ encoder.AppendAddress(file_addr);
+ encoder.AppendData(data_after_op);
+ m_data.SetData(encoder.GetDataBuffer());
+ return true;
+}
+const offset_t op_arg_size =
+GetOpcodeDataSize(m_data, offset, op, dwarf_cu);
+if (op_arg_size == LLDB_INVALID_OFFSET)
+ break;
+offset += op_arg_size;
}
return false;
}
Index: lldb/unittests/Expression/DWARFExpressionTest.cpp
===
--- lldb/unittests/Expression/DWARFExpressionTest.cpp
+++ lldb/unittests/Expression/DWARFExpressionTest.cpp
@@ -522,6 +522,11 @@
ASSERT_TRUE(evaluate(expr, status, result)) << status.ToError();
ASSERT_EQ(result.GetValueType(), Value::ValueType::LoadAddress);
ASSERT_EQ(result.GetScalar().UInt(), 0x5678u);
+
+ ASSERT_TRUE(expr.Update_DW_OP_addr(dwarf_cu, 0xdeadbeef));
+
