[Lldb-commits] [PATCH] D70314: [lldb] Add better test commands for expression evaluation

[Pavel Labath via Phabricator via lldb-commits]

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

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


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

https://reviews.llvm.org/D70314



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

[Lldb-commits] [PATCH] D70314: [lldb] Add better test commands for expression evaluation

[Raphael Isemann via Phabricator via lldb-commits]

teemperor updated this revision to Diff 238209.
teemperor added a comment.

- Removed substr functionality.
- Using `frame()` now.


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

https://reviews.llvm.org/D70314

Files:
  
lldb/packages/Python/lldbsuite/test/commands/expression/call-function/TestCallBuiltinFunction.py
  
lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/string/TestDataFormatterLibcxxString.py
  lldb/packages/Python/lldbsuite/test/lldbtest.py


Index: lldb/packages/Python/lldbsuite/test/lldbtest.py
===
--- lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -2366,6 +2366,45 @@
 self.assertTrue(matched if matching else not matched,
 msg if msg else EXP_MSG(str, output, exe))
 
+def expect_expr(
+self,
+expr,
+result_summary=None,
+result_value=None,
+result_type=None,
+error_msg=None,
+):
+"""
+Evaluates the given expression and verifies the result.
+:param expr: The expression as a string.
+:param result_summary: The summary that the expression should have. 
None if the summary should not be checked.
+: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.
+"""
+self.assertTrue(expr.strip() == expr, "Expression contains 
trailing/leading whitespace: '" + expr + "'")
+
+frame = self.frame()
+eval_result = frame.EvaluateExpression(expr)
+
+if error_msg:
+self.assertFalse(eval_result.IsValid())
+self.assertEqual(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:
+self.assertEqual(result_type, eval_result.GetTypeName())
+
+if result_value:
+self.assertEqual(result_value, eval_result.GetValue())
+
+if result_summary:
+self.assertEqual(result_summary, eval_result.GetSummary())
+
 def invoke(self, obj, name, trace=False):
 """Use reflection to call a method dynamically with no argument."""
 trace = (True if traceAlways else trace)
Index: 
lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/string/TestDataFormatterLibcxxString.py
===
--- 
lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/string/TestDataFormatterLibcxxString.py
+++ 
lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/string/TestDataFormatterLibcxxString.py
@@ -96,10 +96,11 @@
 cappedSummary.find("someText") <= 0,
 "cappedSummary includes the full string")
 
+self.expect_expr("s", result_type=ns+"::wstring", 
result_summary='L"hello world! מזל טוב!"')
+
 self.expect(
 "frame variable",
 substrs=[
-'(%s::wstring) s = L"hello world! מזל טוב!"'%ns,
 '(%s::wstring) S = L"!"'%ns,
 '(const wchar_t *) mazeltov = 0x',
 'L"מזל טוב"',
Index: 
lldb/packages/Python/lldbsuite/test/commands/expression/call-function/TestCallBuiltinFunction.py
===
--- 
lldb/packages/Python/lldbsuite/test/commands/expression/call-function/TestCallBuiltinFunction.py
+++ 
lldb/packages/Python/lldbsuite/test/commands/expression/call-function/TestCallBuiltinFunction.py
@@ -39,7 +39,7 @@
 
 # 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")


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

[Lldb-commits] [PATCH] D70314: [lldb] Add better test commands for expression evaluation

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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




Comment at: lldb/packages/Python/lldbsuite/test/lldbtest.py:2396-2407
+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

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



Comment at: lldb/packages/Python/lldbsuite/test/lldbtest.py:2413
+
+frame = 
self.dbg.GetTargetAtIndex(0).GetProcess().GetThreadAtIndex(0).GetFrameAtIndex(0)
+eval_result = frame.EvaluateExpression(expr)

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


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

https://reviews.llvm.org/D70314



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

[Lldb-commits] [PATCH] D70314: [lldb] Add better test commands for expression evaluation

[Raphael Isemann via Phabricator via lldb-commits]

teemperor updated this revision to Diff 238205.
teemperor added a comment.

- Removed everything that is not summary or value.


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

https://reviews.llvm.org/D70314

Files:
  
lldb/packages/Python/lldbsuite/test/commands/expression/call-function/TestCallBuiltinFunction.py
  
lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/string/TestDataFormatterLibcxxString.py
  lldb/packages/Python/lldbsuite/test/lldbtest.py

Index: lldb/packages/Python/lldbsuite/test/lldbtest.py
===
--- lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -2366,6 +2366,71 @@
 self.assertTrue(matched if matching else not matched,
 msg if msg else EXP_MSG(str, output, exe))
 
+def expect_expr(
+self,
+expr,
+result_summary=None,
+result_value=None,
+result_type=None,
+error_msg=None,
+):
+"""
+Evaluates the given expression and verifies the result.
+:param expr: The expression as a string.
+:param result_summary: The summary that the expression should have. None if the summary should not be checked.
+: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.
+
+result_summary, 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)
+eval_result = frame.EvaluateExpression(expr)
+
+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:
+check_str(self, result_value, eval_result.GetValue())
+
+if result_summary:
+check_str(self, result_summary, eval_result.GetSummary())
+
 def invoke(self, obj, name, trace=False):
 """Use reflection to call a method dynamically with no argument."""
 trace = (True if traceAlways else trace)
Index: lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/string/TestDataFormatterLibcxxString.py
===
--- lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/string/TestDataFormatterLibcxxString.py
+++ 

[Lldb-commits] [PATCH] D70314: [lldb] Add better test commands for expression evaluation

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

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.


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

https://reviews.llvm.org/D70314



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

[Lldb-commits] [PATCH] D70314: [lldb] Add better test commands for expression evaluation

[Raphael Isemann via Phabricator via lldb-commits]

teemperor updated this revision to Diff 238189.
teemperor added a comment.

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


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

https://reviews.llvm.org/D70314

Files:
  
lldb/packages/Python/lldbsuite/test/commands/expression/call-function/TestCallBuiltinFunction.py
  
lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/map/TestDataFormatterLibccMap.py
  lldb/packages/Python/lldbsuite/test/lldbtest.py

Index: lldb/packages/Python/lldbsuite/test/lldbtest.py
===
--- lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -2366,6 +2366,95 @@
 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)
+

[Lldb-commits] [PATCH] D70314: [lldb] Add better test commands for expression evaluation

[Raphael Isemann via Phabricator via lldb-commits]

teemperor marked an inline comment as done.
teemperor added inline comments.



Comment at: lldb/packages/Python/lldbsuite/test/lldbtest.py:2480-2482
+self.expect_expr(var, result_value=result_value, 
result_type=result_type, run_type=self.ExpressionCommandType.EXPRESSION)
+self.expect_expr(var, result_value=result_value, 
result_type=result_type, run_type=self.ExpressionCommandType.FRAME_VAR)
+self.expect_expr(var, result_value=result_value, 
result_type=result_type, run_type=self.ExpressionCommandType.EXPRESSION)

labath wrote:
> teemperor wrote:
> > labath wrote:
> > > 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.
> > 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.
> 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.
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.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D70314



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

[Lldb-commits] [PATCH] D70314: [lldb] Add better test commands for expression evaluation

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/packages/Python/lldbsuite/test/lldbtest.py:2480-2482
+self.expect_expr(var, result_value=result_value, 
result_type=result_type, run_type=self.ExpressionCommandType.EXPRESSION)
+self.expect_expr(var, result_value=result_value, 
result_type=result_type, run_type=self.ExpressionCommandType.FRAME_VAR)
+self.expect_expr(var, result_value=result_value, 
result_type=result_type, run_type=self.ExpressionCommandType.EXPRESSION)

teemperor wrote:
> labath wrote:
> > 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.
> 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.
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.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D70314



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

[Lldb-commits] [PATCH] D70314: [lldb] Add better test commands for expression evaluation

[Raphael Isemann via Phabricator via lldb-commits]

teemperor planned changes to this revision.
teemperor marked an inline comment as done.
teemperor added a comment.

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.




Comment at: lldb/packages/Python/lldbsuite/test/lldbtest.py:2480-2482
+self.expect_expr(var, result_value=result_value, 
result_type=result_type, run_type=self.ExpressionCommandType.EXPRESSION)
+self.expect_expr(var, result_value=result_value, 
result_type=result_type, run_type=self.ExpressionCommandType.FRAME_VAR)
+self.expect_expr(var, result_value=result_value, 
result_type=result_type, run_type=self.ExpressionCommandType.EXPRESSION)

labath wrote:
> 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.
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.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D70314



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

[Lldb-commits] [PATCH] D70314: [lldb] Add better test commands for expression evaluation

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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.




Comment at: lldb/packages/Python/lldbsuite/test/lldbtest.py:2480-2482
+self.expect_expr(var, result_value=result_value, 
result_type=result_type, run_type=self.ExpressionCommandType.EXPRESSION)
+self.expect_expr(var, result_value=result_value, 
result_type=result_type, run_type=self.ExpressionCommandType.FRAME_VAR)
+self.expect_expr(var, result_value=result_value, 
result_type=result_type, run_type=self.ExpressionCommandType.EXPRESSION)

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.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D70314



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

[Lldb-commits] [PATCH] D70314: [lldb] Add better test commands for expression evaluation

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

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.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D70314



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

[Lldb-commits] [PATCH] D70314: [lldb] Add better test commands for expression evaluation

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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)?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D70314



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

[Lldb-commits] [PATCH] D70314: [lldb] Add better test commands for expression evaluation

[Raphael Isemann via Phabricator via lldb-commits]

teemperor created this revision.
teemperor added reviewers: JDevlieghere, shafik, labath.
Herald added subscribers: lldb-commits, abidh, christof.
Herald added a project: LLDB.

This patch adds two new commands to lldbtest: `expect_expr` and 
`expect_simple_expr`. These commands
are supposed to replace the current approach of calling `expect`/`runCmd` with 
`frame var`, `expr`, `p`
(and `SBFrame.EvaluateExpression` in the future).

`expect_expr` allows evaluating expressions and matching their 
result/type/error message. It allows checking
the type/error message/result of both `frame var` and `expr` without having to 
parse the output of these
commands each time (or using hacks like `substrs=[" = 1\n"]` to match the 
value). It no longer allows unintended weak
type/value checks like we currently have with calling `self.expect` (e.g. this 
can semi-randomly pass when
the result variable was assigned `$1`: `self.expect("expr 3+4", 
substrs=["1"])`).

`expect_simple_expr` allows evaluating simple expressions like printing 
variables but is testing them more thoroughly
than we currently do (by for example running the expression multiple times and 
with both `frame var` and `expr`).
If we used this method we would have caught major regressions like the one 
fixed in D67803 .

Currently I just added the method to a few tests to demonstrate them. I'll 
migrate the rest of the test
suit over the next weeks because it's quite time intensive to rewrite all of 
this (and also because I already had some
tests that discovered bugs after I migrated them to `expect_simple_expr`).


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D70314

Files:
  
lldb/packages/Python/lldbsuite/test/commands/expression/call-function/TestCallBuiltinFunction.py
  
lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/map/TestDataFormatterLibccMap.py
  lldb/packages/Python/lldbsuite/test/lldbtest.py

Index: lldb/packages/Python/lldbsuite/test/lldbtest.py
===
--- lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -2366,6 +2366,121 @@
 self.assertTrue(matched if matching else not matched,
 msg if msg else EXP_MSG(str, output, exe))
 
+
+class ExpressionCommandType:
+EXPRESSION = object()
+FRAME_VAR = object()
+# FIXME: SBFRAME_EVALUATE
+
+def expect_expr(
+self,
+expr,
+result_value=None,
+result_type=None,
+error_msg=None,
+run_type=ExpressionCommandType.EXPRESSION
+):
+"""
+Evaluates the given expression with the expression command or optionally 'frame var'
+: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.
+
+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.
+"""
+self.assertTrue(expr.strip() == expr, "Expression contains trailing/leading whitespace: '" + expr + "'")
+
+# Run the command and extract the output.
+if run_type in (self.ExpressionCommandType.FRAME_VAR,
+self.ExpressionCommandType.EXPRESSION):
+if run_type is self.ExpressionCommandType.EXPRESSION:
+run_cmd = "expr -- "
+elif run_type is self.ExpressionCommandType.FRAME_VAR:
+run_cmd = "frame var "
+else:
+self.assertTrue(False, "Shouldn't end up here: " + str(run_type))
+self.ci.HandleCommand(run_cmd + expr, self.res, False)
+err_out = self.res.GetError()
+out = self.res.GetOutput()
+success = self.res.Succeeded()
+# Both 'frame var' and 'expr' have results in the format:
+#   ()  = 
+# This extracts these parts out of these two commands.
+self.assertTrue(" = " in out, "No result in output: '" + out + "'")
+var, result = out.split(" = ", 1)
+var_name = var.split(" ")[-1]
+var_type = var[:-len(var_name)]
+
+# Turn '(type)