[Lldb-commits] [PATCH] D79491: [lldb] Revive TestBasicEntryValuesX86_64

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

@vsk (assuming you're the original author of this test - couldn't quite figure out the revision history, sorry) - could you check if some of this could be simplified a bit to make it more clear what's being tested/what's "interesting" here? (I've provided some inline comments to specifics)



================
Comment at: lldb/test/API/functionalities/param_entry_vals/basic_entry_values/main.cpp:26-28
 void func2(int &sink, int x) {
-  use(x);
-
-  // Destroy 'x' in the current frame.
-  DESTROY_RSI;
-
-  //% self.filecheck("expr x", "main.cpp", "-check-prefix=FUNC2-EXPR-FAIL", expect_cmd_failure=True)
-  // FUNC2-EXPR-FAIL: couldn't get the value of variable x: variable not available
-
-  ++sink;
-
-  // Destroy 'sink' in the current frame.
-  DESTROY_RBX;
-
-  //% self.filecheck("expr sink", "main.cpp", "-check-prefix=FUNC2-EXPR")
-  // FUNC2-EXPR: ${{.*}} = 2
+  use<int &, int>(sink, x);
+  use<int &, int>(dummy, 0);
----------------
Any idea if/why this test tests the int& again, like func1 did? Rather than passing only an int value? (or instead keep this function that tests both int and int& in one, and remove the test that only tests int& since that's covered here?)


================
Comment at: lldb/test/API/functionalities/param_entry_vals/basic_entry_values/main.cpp:47-49
 void func4_amb(int &sink, int x) {
-  use(x);
-
-  // Destroy 'x' in the current frame.
-  DESTROY_RSI;
-
-  //% self.filecheck("expr x", "main.cpp", "-check-prefix=FUNC4-EXPR-FAIL", expect_cmd_failure=True)
-  // FUNC4-EXPR-FAIL: couldn't get the value of variable x: variable not available
-
-  ++sink;
-
-  // Destroy 'sink' in the current frame.
-  DESTROY_RBX;
-
-  //% self.filecheck("expr sink", "main.cpp", "-check-prefix=FUNC4-EXPR", expect_cmd_failure=True)
-  // FUNC4-EXPR: couldn't get the value of variable sink: Could not evaluate DW_OP_entry_value.
+  use<int &, int>(sink, x);
+  use<int &, int>(dummy, 0);
----------------
Not sure why the two parameters here are interesting, compared to 1 (and, I'd probably prefer just "int", seems simpler)


================
Comment at: lldb/test/API/functionalities/param_entry_vals/basic_entry_values/main.cpp:77
   // FUNC7-BT-NEXT: [inlined] func8_inlined
   // FUNC7-BT-NEXT: [inlined] func9_inlined
   // FUNC7-BT-NEXT: func10
----------------
Any idea if 2 inline frames are especially interesting, rather than just 1? If so, might be worth explaining why.


================
Comment at: lldb/test/API/functionalities/param_entry_vals/basic_entry_values_x86_64/main.cpp:17-18
+  // with other values.
+  use<int &>(sink);
+  use<int &>(dummy);
 
----------------
You can omit the explicit template parameters here - they can be deduced... oh, but they'd be deduced as value rather than reference?

I think the test would probably be easier to understand if it only traficked in values anyway - what do you think? (just pass ints by value, inspect int values, etc) Or is there something interesting about testing int& versus int parameters? (I guess maybe the original author thought there was - since func1 tests the reference case, func2 tests the value case)

You could still omit the template parameter, and wouldn't need a variable called "dummy" here, instead writing:
```
use(0);
```
for instance


================
Comment at: lldb/test/API/functionalities/param_entry_vals/basic_entry_values_x86_64/main.cpp:20
 
+  ++global;
   //% self.filecheck("image lookup -va $pc", "main.cpp", "-check-prefix=FUNC1-DESC")
----------------
Is this here so you can break & observe the entry_value location being used? (because, I guess, if you break on "use(dummy)" it'll be before use is called, so it'll still be using "use(sink)"'s location... actually, no - even before the call to "use(dummy)" (but after, or even during, the call to "use(sink)") we would have to rely on the entry_value (because use(sink) might've modified the register, so it's no longer representing the 'sink' value)

So I /think/ this "++global" can be removed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79491