[lldb] Add expect_expr function for testing expression evaluation in dotests.

[teemperor]

Summary

This patch adds a new function to lldbtest: expect_expr. This function is supposed to replace the current approach
of calling expect/runCmd with expr, p etc.

expect_expr allows evaluating expressions and matching their value/summary/type/error message without
having to do any string matching that might allow unintended passes (e.g., self.expect("expr 3+4", substrs=["7"])
can unexpectedly pass for results like (Class7) $0 = 7, (int) $7 = 22, (int) $0 = 77 and so on).

This only uses the function in a few places to test and demonstrate it. I'll migrate the tests in follow up commits.

[labath]

Semi-random idea: Instead of running the expression through the command line and then unpacking the result, should we just use the appropriate SBFrame api here (EvaluateExpression/GetValueForVariablePath)?

[teemperor]

In D70314#1747711, @labath wrote:

Semi-random idea: Instead of running the expression through the command line and then unpacking the result, should we just use the appropriate SBFrame api here (EvaluateExpression/GetValueForVariablePath)?

That's one of the APIs we need to test, but we need to test all of them to prevent regressions like D67803. At least frame var and expr behave very differently (EvaluateExpression should be similar to expr but I never looked at that code path). So the idea is to have one command that tests all these APIs and if we can't do that (for example because the expression can't be repeated with the same result), at least make that safer/easier to do.

[labath]

In D70314#1747850, @teemperor wrote:

In D70314#1747711, @labath wrote:

Semi-random idea: Instead of running the expression through the command line and then unpacking the result, should we just use the appropriate SBFrame api here (EvaluateExpression/GetValueForVariablePath)?

That's one of the APIs we need to test, but we need to test all of them to prevent regressions like D67803. At least frame var and expr behave very differently (EvaluateExpression should be similar to expr but I never looked at that code path). So the idea is to have one command that tests all these APIs and if we can't do that (for example because the expression can't be repeated with the same result), at least make that safer/easier to do.

I agree we need to test all API's, but the testing strategies can differ. I am aware that "frame variable" and "expression" are very different. However, SBFrame::EvaluateExpression is not that different from the "expression" command -- in fact, I'd say that it's indistinguishable as for the stuff you're interested in testing here. And same goes for "frame variable" and SBFrame::GetValueForVariablePath.

So, I think we could just use one of the two mechanisms for the general evaluation test, and then have different tests for which test the unique properties of the specific mechanisms (e.g., command line option parsing). I certainly don't see a reason to run the same expression both through the command line and SB apis "just in case".

I'm thinking/hoping that the SB api approach would be a better fit for the "run this expression and see what you get" kinds of tests, because it avoids the need to reverse-engineer the values/types out of the textual output.

lldb/packages/Python/lldbsuite/test/lldbtest.py

2480–2482

I'm also not sold on the idea of running the same expression multiple times just because we've had some bug that would've been caught by that in the past. Lldb already does a lot more combinatorial testing than anything else in llvm. I don't think that adding more of it is the solution to any stability problem.

If there's some tricky aspect of combining "frame variable" and "expression" commands then we should have a separate test for that. I'd be much happier to have one or two tests that run a single expression a hundred times than putting the repetition in every test and hoping the shotgun effect of that will catch something.

[teemperor]

In D70314#1749427, @labath wrote:

In D70314#1747850, @teemperor wrote:

In D70314#1747711, @labath wrote:

Semi-random idea: Instead of running the expression through the command line and then unpacking the result, should we just use the appropriate SBFrame api here (EvaluateExpression/GetValueForVariablePath)?

That's one of the APIs we need to test, but we need to test all of them to prevent regressions like D67803. At least frame var and expr behave very differently (EvaluateExpression should be similar to expr but I never looked at that code path). So the idea is to have one command that tests all these APIs and if we can't do that (for example because the expression can't be repeated with the same result), at least make that safer/easier to do.

I agree we need to test all API's, but the testing strategies can differ. I am aware that "frame variable" and "expression" are very different. However, SBFrame::EvaluateExpression is not that different from the "expression" command -- in fact, I'd say that it's indistinguishable as for the stuff you're interested in testing here. And same goes for "frame variable" and SBFrame::GetValueForVariablePath.

So, I think we could just use one of the two mechanisms for the general evaluation test, and then have different tests for which test the unique properties of the specific mechanisms (e.g., command line option parsing). I certainly don't see a reason to run the same expression both through the command line and SB apis "just in case".

I'm thinking/hoping that the SB api approach would be a better fit for the "run this expression and see what you get" kinds of tests, because it avoids the need to reverse-engineer the values/types out of the textual output.

I haven't compared how similar the actual implementations are, but moving that to just use the SB API seems ok.

lldb/packages/Python/lldbsuite/test/lldbtest.py

2480–2482

I'm fine making this just frame var/expr and not expr/frame/expr, but I don't see how we can get rid of the fact that we need to test both APIs (which is not just about them interacting, but just that we need to test both unique code paths). Testing just one is not a responsible way of testing these APIs and duplicating calls for them in all relevant tests doesn't sound like an option either.

[labath]

lldb/packages/Python/lldbsuite/test/lldbtest.py

2480–2482

Both apis need to be tested, of course, and having some utility to do that seems reasonable. A different question is how when should this utility be used.

For instance, one could reasonable argue that "expression" is not needed for testing data formatters, as they only kick in after the result has been computed (and they should not care about how that result came to be). While OTOH, formatters have some pretty complex interactions with the "frame variable" command. However, it seems that both mechanisms are being used right now for testing formatters, so I am not going to argue for removing them.

I also don't think it's a disaster if someone runs both apis because "it's easy to do so", though I still think it's better (for various reasons) to write more isolated tests whereever possible.

[teemperor]

lldb/packages/Python/lldbsuite/test/lldbtest.py

2480–2482

From a reasonable point of view, data formatters should only be tested with one of them. However, our implementation isn't reasonable as the data formatters for frame var and expr work with completely differently computed ASTs under the hood (with expr we have this fragile indirection via the temporary AST for the expression). Testing the data formatters with both expr and frame var is one of the main motivations for doing this.

[teemperor]

• Moved to using the SBAPI.

We can't get fully rid of parsing some output as GetDescription of SBValue returns (type) $0 = VALUE\n but there
seems to be no way to get rid of the stuff around the value we want. But we now only strip it away instead of
trying to parse the type etc.

[teemperor]

I also got rid of the expr->frame var->expr flow and it's not just expr->frame var. I don't want to remove them as we found two formatter bugs by testing both but once these thing don't break constantly then we can remove the 'expr' variant from the simple expression function.

[teemperor]

• Removed everything that is not summary or value.

[labath]

This seems fine, assuming it is sufficient to achieve your goals.

lldb/packages/Python/lldbsuite/test/lldbtest.py

2396–2407

maybe something like self.expect(got, substrs=expected, exe=False) ?

2413

frame = self.frame(). If it turns out useful, we could also add an argument to run the expression on a specific frame...

[teemperor]

• Removed substr functionality.
• Using frame() now.

[labath]

I think this is a great start. We can see how we can extend this later...

[clayborg]

nice!

lldb/packages/Python/lldbsuite/test/commands/expression/call-function/TestCallBuiltinFunction.py

def test(self):
 
         self.runCmd("run", RUN_SUCCEEDED)
 
         interp = self.dbg.GetCommandInterpreter()
         result = lldb.SBCommandReturnObject()
 
         # Test different builtin functions.
 
-        self.expect("expr __builtin_isinf(0.0f)", substrs=["(int) $", " = 0\n"])
-        self.expect("expr __builtin_isnormal(0.0f)", substrs=["(int) $", " = 0\n"])
-        self.expect("expr __builtin_constant_p(1)", substrs=["(int) $", " = 1\n"])
-        self.expect("expr __builtin_abs(-14)", substrs=["(int) $", " = 14\n"])
+        self.expect_expr("__builtin_isinf(0.0f)", result_type="int", result_value="0")
+        self.expect_expr("__builtin_isnormal(0.0f)", result_type="int", result_value="0")
+        self.expect_expr("__builtin_constant_p(1)", result_type="int", result_value="1")
+        self.expect_expr("__builtin_abs(-14)", result_type="int", result_value="14")

lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/map/TestDataFormatterLibccMap.py

def test_with_run_command(self):
             self.runCmd(
                 "settings set target.max-children-count 256",
                 check=False)
 
         # Execute the cleanup function during test case tear down.
         self.addTearDownHook(cleanup)
 
         ns = self.namespace
-        self.expect('p ii',
-                    substrs=['%s::map' % ns,
-                             'size=0',
-                             '{}'])
-        self.expect('frame var ii',
-                    substrs=['%s::map' % ns,
-                             'size=0',
-                             '{}'])
+        self.expect_simple_expr('ii', result_type=['%s::map' % ns], result_value=['size=0', "{}"])
 
         lldbutil.continue_to_breakpoint(self.process(), bkpt)
 
-        self.expect('p ii',
-                    substrs=['%s::map' % ns, 'size=2',
-                             '[0] = ',
-                             'first = 0',
-                             'second = 0',
-                             '[1] = ',
-                             'first = 1',
-                             'second = 1'])
-
-        self.expect('frame variable ii',
-                    substrs=['%s::map' % ns, 'size=2',
+        self.expect_simple_expr('ii', result_type=['%s::map' % ns], result_value=['size=2',
                              '[0] = ',
                              'first = 0',
                              'second = 0',
                              '[1] = ',
                              'first = 1',
                              'second = 1'])
 
         lldbutil.continue_to_breakpoint(self.process(), bkpt)
 
-        self.expect('frame variable ii',
-                    substrs=['%s::map' % ns, 'size=4',
+        self.expect_simple_expr('ii', result_type=['%s::map' % ns], result_value=['size=4',
                              '[2] = ',
                              'first = 2',
                              'second = 0',
                              '[3] = ',
                              'first = 3',
                              'second = 1'])
 
         lldbutil.continue_to_breakpoint(self.process(), bkpt)
 
-        self.expect("p ii",
-                    substrs=['%s::map' % ns, 'size=8',
-                             '[5] = ',
-                             'first = 5',
-                             'second = 0',
-                             '[7] = ',
-                             'first = 7',
-                             'second = 1'])
-
-        self.expect("frame var ii",
-                    substrs=['%s::map' % ns, 'size=8',
+        self.expect_simple_expr('ii', result_type=['%s::map' % ns], result_value=['size=8',
                              '[5] = ',
                              'first = 5',
                              'second = 0',
                              '[7] = ',
                              'first = 7',
                              'second = 1'])
 
         # check access-by-index

lldb/packages/Python/lldbsuite/test/lldbtest.py

def expect(
                     print("Matched" if matched else "Not matched", file=sbuf)
                 keepgoing = matched if matching else not matched
                 if not keepgoing:
                     break
 
         self.assertTrue(matched if matching else not matched,
                         msg if msg else EXP_MSG(str, output, exe))
 
+    class ExpressionRunMode:
+      # Run the expression in the expression evaluator (similar to 'expression').
+      EXPRESSION = object()
+      # Interpret the expression as just a variable that should be printed (similar to 'frame var').
+      PRINT_VAR = object()
+
+    def expect_expr(
+            self,
+            expr,
+            result_value=None,
+            result_type=None,
+            error_msg=None,
+            run_mode=ExpressionRunMode.EXPRESSION
+            ):
+        """
+        Evaluates the given expression and verifies the result.
+        :param expr: The expression as a string.
+        :param result_value: The value that the expression should have. None if the value should not be checked.
+        :param result_type: The type that the expression result should have. None if the type should not be checked.
+        :param error_msg: The error message the expression should return. None if the error output should not be checked.
+        :param run_type: How the expression should be run. See ExpressionRunMode.
+
+        result_value, result_type and error_message can have the following types which influences how
+        their values are compared to their respective output:
+          * A list of strings: expect_expr will search for the list of strings in the respective output.
+                               The output is expected to contain these strings in the listed order.
+          * Any string type: expect_expr will assume that the respective output is equal to the given string.
+        """
+        # Utility method that checks result_value, result_type and error_message.
+        def check_str(outer_self, expected, got):
+            self.assertIsNotNone(expected)
+            self.assertIsNotNone(got)
+            # We got a list, so treat is as a list of needles we need to find in the given order.
+            if type(expected) is list:
+                remaining = got
+                for expected_part in expected:
+                    # Find the expected string.
+                    i = remaining.find(expected_part)
+                    # Assert that we found the string.
+                    outer_self.assertTrue(i != -1, "Couldn't find '" + expected_part
+                                          + "' in remaining output '" + remaining +
+                                          "'.\nFull string was: '" + got + "'")
+                    # Keep searching only the rest of the string to ensure the
+                    # strings are found in the given order.
+                    remaining = remaining[i + len(expected_part):]
+            else: # Otherwise we probably got one of Python's many string classes.
+                outer_self.assertEqual(got, expected)
+
+        self.assertTrue(expr.strip() == expr, "Expression contains trailing/leading whitespace: '" + expr + "'")
+
+        frame = self.dbg.GetTargetAtIndex(0).GetProcess().GetThreadAtIndex(0).GetFrameAtIndex(0)
+
+        if run_mode is self.ExpressionRunMode.PRINT_VAR:
+          eval_result = frame.FindVariable(expr)
+        elif run_mode is self.ExpressionRunMode.EXPRESSION:
+          eval_result = frame.EvaluateExpression(expr)
+        else:
+          self.fail("Unknown run mode " + str(run_mode))
+
+        if error_msg:
+            self.assertFalse(eval_result.IsValid())
+            check_str(self, error_msg, eval_result.GetError().GetCString())
+            return
+
+        if not eval_result.GetError().Success():
+            self.assertTrue(eval_result.GetError().Success(),
+                "Unexpected failure with msg: " + eval_result.GetError().GetCString())
+
+        if result_type:
+            check_str(self, result_type, eval_result.GetTypeName())
+        if result_value:
+            ss = lldb.SBStream()
+            eval_result.GetDescription(ss)
+            description = ss.GetData()
+            # Strip out the value from the description text that looks like this:
+            # (the_type) $434 = VALUE\n
+            value_desc = description.split(" = ", 1)[1].rstrip("\n")
+            check_str(self, result_value, value_desc)
+
+    def expect_simple_expr(self, var, result_value=None, result_type=None):
+        """
+        Takes a trivial expression such as a plain variable name or accessing a member ('instance.member')
+        and verifies that all ways LLDB can evaluate these expressions do it correctly
+        (e.g., 'frame var', 'expression' commands)
+        See expect_expr for documentation for result_value and result_type.
+        """
+        self.expect_expr(var, result_value=result_value, result_type=result_type, run_mode=self.ExpressionRunMode.EXPRESSION)
+        self.expect_expr(var, result_value=result_value, result_type=result_type, run_mode=self.ExpressionRunMode.PRINT_VAR)
+
     def invoke(self, obj, name, trace=False):
         """Use reflection to call a method dynamically with no argument."""
         trace = (True if traceAlways else trace)
 
         method = getattr(obj, name)
         import inspect
         self.assertTrue(inspect.ismethod(method),
                         name + "is a method name of object: " + str(obj))
         result = method()
         with recording(self, trace) as sbuf:
             print(str(method) + ":", result, file=sbuf)
         return result
 
     def build(
             self,
             architecture=None,
             compiler=None,
             dictionary=None):
         """Platform specific way to build the default binaries."""
         module = builder_module()
 
         dictionary = lldbplatformutil.finalize_build_dictionary(dictionary)
         if self.getDebugInfo() is None:
             return self.buildDefault(architecture, compiler, dictionary)
         elif self.getDebugInfo() == "dsym":
             return self.buildDsym(architecture, compiler, dictionary)
         elif self.getDebugInfo() == "dwarf":
             return self.buildDwarf(architecture, compiler, dictionary)
         elif self.getDebugInfo() == "dwo":
             return self.buildDwo(architecture, compiler, dictionary)
         elif self.getDebugInfo() == "gmodules":
             return self.buildGModules(architecture, compiler, dictionary)
         else: