[Lldb-commits] [PATCH] D77108: [lldb/DWARF] Fix evaluator crash when accessing empty stack

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk added a comment.

In D77108#1952039 , @labath wrote:

> In D77108#1951997 , @kwk wrote:
>
> > In D77108#1951879 , @labath wrote:
> >
> > > Most DW_OP cases check their stack, but it's quite possible that others 
> > > were missed too. It might be a nice cleanup to make a function like 
> > > (`getMinimalStackSize(op)`) and move this check up in front of the big 
> > > switch. That could not handle all operators, as for some of them, the 
> > > value is not statically known, but it would handle the vast majority of 
> > > them.
> >
> >
> > @labath I somewhat like that the logic for each `op` is close to the `case` 
> > statement. Creating the function that you suggested would separate this 
> > check out somewhere else and you would have to look at two places. At least 
> > for code review, the current solution is much clearer to me.
>
>
> I don't have a problem with the current patch (modulo the test tweak) -- it 
> fixes a real problem and it follows the style of the surrounding code. 
> However, DWARFExpression::Evaluate is gigantic (2600LOC), and nearly half of 
> that is error handling. Tastes may vary, but I think that's a bigger 
> readability problem than having to look at two places to understand an opcode.


@labath okay, that makes sense. Sorry.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77108



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D77108: [lldb/DWARF] Fix evaluator crash when accessing empty stack

[Djordje Todorovic via Phabricator via lldb-commits]

djtodoro added a comment.

In D77108#1952818 , @aprantl wrote:

> For the future, a clean solution would be extending the macros in Dwarf.def 
> to list the stack effects in the definitions of the DW_OP_*, for example
>
>   // opcode, name, version, vendor, in, out
>   HANDLE_DW_OP(0x12, dup, 2, DWARF, 1, 2)
>
>
> and then we could write a static verifier that ensures that the stack effects 
> of an entire expression is sound. (And we could check this in LLVM, already, 
> too).


`DW_OP_entry_value` may be requiring a special handling as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77108



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D77108: [lldb/DWARF] Fix evaluator crash when accessing empty stack

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

CC'ing the DWARF cabal.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77108



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D77108: [lldb/DWARF] Fix evaluator crash when accessing empty stack

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D77108#1952818 , @aprantl wrote:

> For the future, a clean solution would be extending the macros in Dwarf.def 
> to list the stack effects in the definitions of the DW_OP_*, for example
>
>   // opcode, name, version, vendor, in, out
>   HANDLE_DW_OP(0x12, dup, 2, DWARF, 1, 2)
>


That sounds very interesting. However,

> Is there any DWARF operator where the stack effect (number of consumes & 
> produced stack elements) isn't statically known?

`DW_OP_pick` comes to mind.  It always produces one item, but it can reach 
arbitrarily deep into the dwarf stack (without consuming that item). Glancing 
through the dwarf spec, it looks like this is the only such operator, so 
special-casing that wouldn't be too bad...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77108



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D77108: [lldb/DWARF] Fix evaluator crash when accessing empty stack

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

For the future, a clean solution would be extending the macros in Dwarf.def to 
list the stack effects in the definitions of the DW_OP_*, for example

  // opcode, name, version, vendor, in, out
  HANDLE_DW_OP(0x12, dup, 2, DWARF, 1, 2)

and then we could write a static verifier that ensures that the stack effects 
of an entire expression is sound. (And we could check this in LLVM, already, 
too).

Is there any DWARF operator where the stack effect (number of consumes & 
produced stack elements) isn't statically known?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77108



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D77108: [lldb/DWARF] Fix evaluator crash when accessing empty stack

[Med Ismail Bennani via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGf3a7d790df33: [lldb/DWARF] Fix evaluator crash when 
accessing empty stack. (authored by mib).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77108

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
@@ -234,6 +234,10 @@
   llvm::Failed());
 }
 
+TEST(DWARFExpression, DW_OP_stack_value) {
+  EXPECT_THAT_EXPECTED(Evaluate({DW_OP_stack_value}), llvm::Failed());
+}
+
 TEST(DWARFExpression, DW_OP_piece) {
   EXPECT_THAT_EXPECTED(Evaluate({DW_OP_const2u, 0x11, 0x22, DW_OP_piece, 2,
  DW_OP_const2u, 0x33, 0x44, DW_OP_piece, 2}),
Index: lldb/source/Expression/DWARFExpression.cpp
===
--- lldb/source/Expression/DWARFExpression.cpp
+++ lldb/source/Expression/DWARFExpression.cpp
@@ -2318,6 +2318,12 @@
 // rather is a constant value.  The value from the top of the stack is the
 // value to be used.  This is the actual object value and not the location.
 case DW_OP_stack_value:
+  if (stack.empty()) {
+if (error_ptr)
+  error_ptr->SetErrorString(
+  "Expression stack needs at least 1 item for DW_OP_stack_value.");
+return false;
+  }
   stack.back().SetValueType(Value::eValueTypeScalar);
   break;
 


Index: lldb/unittests/Expression/DWARFExpressionTest.cpp
===
--- lldb/unittests/Expression/DWARFExpressionTest.cpp
+++ lldb/unittests/Expression/DWARFExpressionTest.cpp
@@ -234,6 +234,10 @@
   llvm::Failed());
 }
 
+TEST(DWARFExpression, DW_OP_stack_value) {
+  EXPECT_THAT_EXPECTED(Evaluate({DW_OP_stack_value}), llvm::Failed());
+}
+
 TEST(DWARFExpression, DW_OP_piece) {
   EXPECT_THAT_EXPECTED(Evaluate({DW_OP_const2u, 0x11, 0x22, DW_OP_piece, 2,
  DW_OP_const2u, 0x33, 0x44, DW_OP_piece, 2}),
Index: lldb/source/Expression/DWARFExpression.cpp
===
--- lldb/source/Expression/DWARFExpression.cpp
+++ lldb/source/Expression/DWARFExpression.cpp
@@ -2318,6 +2318,12 @@
 // rather is a constant value.  The value from the top of the stack is the
 // value to be used.  This is the actual object value and not the location.
 case DW_OP_stack_value:
+  if (stack.empty()) {
+if (error_ptr)
+  error_ptr->SetErrorString(
+  "Expression stack needs at least 1 item for DW_OP_stack_value.");
+return false;
+  }
   stack.back().SetValueType(Value::eValueTypeScalar);
   break;
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D77108: [lldb/DWARF] Fix evaluator crash when accessing empty stack

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 253895.
mib marked an inline comment as done.
mib added a comment.

Addressed Pavel's request.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77108

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
@@ -234,6 +234,10 @@
   llvm::Failed());
 }
 
+TEST(DWARFExpression, DW_OP_stack_value) {
+  EXPECT_THAT_EXPECTED(Evaluate({DW_OP_stack_value}), llvm::Failed());
+}
+
 TEST(DWARFExpression, DW_OP_piece) {
   EXPECT_THAT_EXPECTED(Evaluate({DW_OP_const2u, 0x11, 0x22, DW_OP_piece, 2,
  DW_OP_const2u, 0x33, 0x44, DW_OP_piece, 2}),
Index: lldb/source/Expression/DWARFExpression.cpp
===
--- lldb/source/Expression/DWARFExpression.cpp
+++ lldb/source/Expression/DWARFExpression.cpp
@@ -2318,6 +2318,12 @@
 // rather is a constant value.  The value from the top of the stack is the
 // value to be used.  This is the actual object value and not the location.
 case DW_OP_stack_value:
+  if (stack.empty()) {
+if (error_ptr)
+  error_ptr->SetErrorString(
+  "Expression stack needs at least 1 item for DW_OP_stack_value.");
+return false;
+  }
   stack.back().SetValueType(Value::eValueTypeScalar);
   break;
 


Index: lldb/unittests/Expression/DWARFExpressionTest.cpp
===
--- lldb/unittests/Expression/DWARFExpressionTest.cpp
+++ lldb/unittests/Expression/DWARFExpressionTest.cpp
@@ -234,6 +234,10 @@
   llvm::Failed());
 }
 
+TEST(DWARFExpression, DW_OP_stack_value) {
+  EXPECT_THAT_EXPECTED(Evaluate({DW_OP_stack_value}), llvm::Failed());
+}
+
 TEST(DWARFExpression, DW_OP_piece) {
   EXPECT_THAT_EXPECTED(Evaluate({DW_OP_const2u, 0x11, 0x22, DW_OP_piece, 2,
  DW_OP_const2u, 0x33, 0x44, DW_OP_piece, 2}),
Index: lldb/source/Expression/DWARFExpression.cpp
===
--- lldb/source/Expression/DWARFExpression.cpp
+++ lldb/source/Expression/DWARFExpression.cpp
@@ -2318,6 +2318,12 @@
 // rather is a constant value.  The value from the top of the stack is the
 // value to be used.  This is the actual object value and not the location.
 case DW_OP_stack_value:
+  if (stack.empty()) {
+if (error_ptr)
+  error_ptr->SetErrorString(
+  "Expression stack needs at least 1 item for DW_OP_stack_value.");
+return false;
+  }
   stack.back().SetValueType(Value::eValueTypeScalar);
   break;
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D77108: [lldb/DWARF] Fix evaluator crash when accessing empty stack

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added a comment.

In D77108#1951610 , @aprantl wrote:

> This is obviously good! Do you think that a similar error handling bug might 
> exist in other cases that depend top-of-stack?


I quickly ran through the function, and it seems this was the only location 
where we were missing the check.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77108



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D77108: [lldb/DWARF] Fix evaluator crash when accessing empty stack

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D77108#1951997 , @kwk wrote:

> In D77108#1951879 , @labath wrote:
>
> > Most DW_OP cases check their stack, but it's quite possible that others 
> > were missed too. It might be a nice cleanup to make a function like 
> > (`getMinimalStackSize(op)`) and move this check up in front of the big 
> > switch. That could not handle all operators, as for some of them, the value 
> > is not statically known, but it would handle the vast majority of them.
>
>
> @labath I somewhat like that the logic for each `op` is close to the `case` 
> statement. Creating the function that you suggested would separate this check 
> out somewhere else and you would have to look at two places. At least for 
> code review, the current solution is much clearer to me.


I don't have a problem with the current patch (modulo the test tweak) -- it 
fixes a real problem and it follows the style of the surrounding code. However, 
DWARFExpression::Evaluate is gigantic (2600LOC), and nearly half of that is 
error handling. Tastes may vary, but I think that's a bigger readability 
problem than having to look at two places to understand an opcode.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77108



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D77108: [lldb/DWARF] Fix evaluator crash when accessing empty stack

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk added a comment.

In D77108#1951879 , @labath wrote:

> In D77108#1951610 , @aprantl wrote:
>
> > This is obviously good! Do you think that a similar error handling bug 
> > might exist in other cases that depend top-of-stack?
>
>
> Most DW_OP cases check their stack, but it's quite possible that others were 
> missed too. It might be a nice cleanup to make a function like 
> (`getMinimalStackSize(op)`) and move this check up in front of the big 
> switch. That could not handle all operators, as for some of them, the value 
> is not statically known, but it would handle the vast majority of them.


@labath I somewhat like that the logic for each `op` is close to the `case` 
statement. Creating the function that you suggested would separate this check 
out somewhere else and you would have to look at two places. At least for code 
review, the current solution is much clearer to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77108



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D77108: [lldb/DWARF] Fix evaluator crash when accessing empty stack

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D77108#1951610 , @aprantl wrote:

> This is obviously good! Do you think that a similar error handling bug might 
> exist in other cases that depend top-of-stack?


Most DW_OP cases check their stack, but it's quite possible that others were 
missed too. It might be a nice cleanup to make a function like 
(`getMinimalStackSize(op)`) and move this check up in front of the big switch. 
That could not handle all operators, as for some of them, the value is not 
statically known, but it would handle the vast majority of them.




Comment at: lldb/unittests/Expression/DWARFExpressionTest.cpp:238
+TEST(DWARFExpression, DW_OP_stack_value) {
+  EXPECT_THAT_ERROR(Evaluate({DW_OP_stack_value}).takeError(), llvm::Failed());
+}

`EXPECT_THAT_EXPECTED(Evaluate(...), Failed())` is better, as it will produce 
an error (instead of a crash) in case the evaluation does succeed for any 
reason.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77108



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D77108: [lldb/DWARF] Fix evaluator crash when accessing empty stack

[Adrian Prantl via Phabricator via lldb-commits]

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

This is obviously good! Do you think that a similar error handling bug might 
exist in other cases that depend top-of-stack?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77108



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D77108: [lldb/DWARF] Fix evaluator crash when accessing empty stack

[Med Ismail Bennani via Phabricator via lldb-commits]

mib created this revision.
mib added a reviewer: aprantl.
mib added a project: LLDB.
Herald added a subscriber: lldb-commits.

This patch fixes a crash that happens on the DWARF expression evaluator
when trying to access the top of the stack while it's empty.

Signed-off-by: Med Ismail Bennani 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D77108

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
@@ -234,6 +234,10 @@
   llvm::Failed());
 }
 
+TEST(DWARFExpression, DW_OP_stack_value) {
+  EXPECT_THAT_ERROR(Evaluate({DW_OP_stack_value}).takeError(), llvm::Failed());
+}
+
 TEST(DWARFExpression, DW_OP_piece) {
   EXPECT_THAT_EXPECTED(Evaluate({DW_OP_const2u, 0x11, 0x22, DW_OP_piece, 2,
  DW_OP_const2u, 0x33, 0x44, DW_OP_piece, 2}),
Index: lldb/source/Expression/DWARFExpression.cpp
===
--- lldb/source/Expression/DWARFExpression.cpp
+++ lldb/source/Expression/DWARFExpression.cpp
@@ -2318,6 +2318,12 @@
 // rather is a constant value.  The value from the top of the stack is the
 // value to be used.  This is the actual object value and not the location.
 case DW_OP_stack_value:
+  if (stack.empty()) {
+if (error_ptr)
+  error_ptr->SetErrorString(
+  "Expression stack needs at least 1 item for DW_OP_stack_value.");
+return false;
+  }
   stack.back().SetValueType(Value::eValueTypeScalar);
   break;
 


Index: lldb/unittests/Expression/DWARFExpressionTest.cpp
===
--- lldb/unittests/Expression/DWARFExpressionTest.cpp
+++ lldb/unittests/Expression/DWARFExpressionTest.cpp
@@ -234,6 +234,10 @@
   llvm::Failed());
 }
 
+TEST(DWARFExpression, DW_OP_stack_value) {
+  EXPECT_THAT_ERROR(Evaluate({DW_OP_stack_value}).takeError(), llvm::Failed());
+}
+
 TEST(DWARFExpression, DW_OP_piece) {
   EXPECT_THAT_EXPECTED(Evaluate({DW_OP_const2u, 0x11, 0x22, DW_OP_piece, 2,
  DW_OP_const2u, 0x33, 0x44, DW_OP_piece, 2}),
Index: lldb/source/Expression/DWARFExpression.cpp
===
--- lldb/source/Expression/DWARFExpression.cpp
+++ lldb/source/Expression/DWARFExpression.cpp
@@ -2318,6 +2318,12 @@
 // rather is a constant value.  The value from the top of the stack is the
 // value to be used.  This is the actual object value and not the location.
 case DW_OP_stack_value:
+  if (stack.empty()) {
+if (error_ptr)
+  error_ptr->SetErrorString(
+  "Expression stack needs at least 1 item for DW_OP_stack_value.");
+return false;
+  }
   stack.back().SetValueType(Value::eValueTypeScalar);
   break;
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits