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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
