[lldb/DWARF] Fix evaluator crash when accessing empty stack

[mib]

Summary

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 <[e-mail]>

[aprantl]

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

[labath]

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.

lldb/unittests/Expression/DWARFExpressionTest.cpp

238

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.

[kwk]

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.

[labath]

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.

[mib]

Addressed Pavel's request.

[mib]

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.

[aprantl]

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?

[aprantl]

CC'ing the DWARF cabal.

[labath]

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

[djtodoro]

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.

[kwk]

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.

lldb/source/Expression/DWARFExpression.cpp

case DW_OP_call4:
       return false;
 
     // OPCODE: DW_OP_stack_value
     // OPERANDS: None
     // DESCRIPTION: Specifies that the object does not exist in memory but
     // 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;
 
     // OPCODE: DW_OP_convert
     // OPERANDS: 1
     //      A ULEB128 that is either a DIE offset of a
     //      DW_TAG_base_type or 0 for the generic (pointer-sized) type.
     //

lldb/unittests/Expression/DWARFExpressionTest.cpp

EXPECT_THAT_ERROR(
       llvm::Failed());
 
   // Unsupported.
   EXPECT_THAT_ERROR(
       t.Eval({DW_OP_const1s, 'X', DW_OP_convert, 0x1d}).takeError(),
       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}),
                        llvm::HasValue(GetScalar(32, 0x44332211, true)));
   EXPECT_THAT_EXPECTED(
       Evaluate({DW_OP_piece, 1, DW_OP_const1u, 0xff, DW_OP_piece, 1}),
       // Note that the "00" should really be "undef", but we can't
       // represent that yet.
       llvm::HasValue(GetScalar(16, 0xff00, true)));
 }