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

Tue Mar 31 02:42:52 PDT 2020

labath added a comment.

In D77108#1951997 <https://reviews.llvm.org/D77108#1951997>, @kwk wrote:

> In D77108#1951879 <https://reviews.llvm.org/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