[lldb] Revive TestBasicEntryValuesX86_64

[labath]

Summary

This function rewrites the test to be (hopefully) less susceptible to
codegen changes and re-enables it.

The most interesting changes are:

• use an attribute((optnone)) function instead of a volatile asm to "use" a value. This isn't strictly necessary, but it makes the function simpler while achieving the same effect.
• use a call to a function with the exact same signature instead of a volatile asm to "destroy" arguments. This makes the independent of the ABI, and (together with avoiding the usage of the arguments after the call) ensures that the compiler has no reason to move the argument from its initial register (previously we needed to guess where will the compiler store the arguments).

[labath]

I should note that I did look into making an assembly file out of this, but that turned out to be more complicated than I expected and then David convinced me that it should be possible to write this in a way that prevents the optimizer from being too smart. Nevertheless, I am still interested in making assembly-based tests for this (and similar features) because it enables testing scenarios that we could not get (reliably or at all) a compiler to produce.

lldb/test/API/functionalities/param_entry_vals/basic_entry_values/main.cpp

21–22

If we remove this check, the test will be completely architecture- and abi-independent. I don't think this check is particularly useful (we use llvm to print the dwarf expression, and there are better ways to test the image lookup command). Maybe we could just keep it to ensure that we really are evaluating entry values, but change the check the just search for the DW_OP_entry_value keyword (and then run the test on all architectures)?

61–62

I'm not sure why this was previously expected to fail -- I don't see a reason why the compiler couldn't emit an entry value for x now, or before this patch. And in the new setup, the expression actually succeeds.

[vsk]

Thanks for working on this!

lldb/test/API/functionalities/param_entry_vals/basic_entry_values/main.cpp

21–22

We should stop matching %rdi, as the purpose of the check is just to determine whether we really are testing entry value evaluation. However, llvm doesn't support entry value production for all platforms, so we would need to restrict the test to {x86_64, arm, aarch64} (still a clear improvement over the current situation).

34

Hm, I thought inline tests only supported running one command per breakpoint. Have you tried running this test with --param dotest-args='-t' to verify that FUNC2-EXPR2 gets matched? If it does, that's great; if not, we can manufacture a second breakpoint by adding another '++global' or convert this to an API test.

56

Ditto, I have the same concern about whether 'expr sink' is evaluated.

61–62

IIRC this is left over from when entry value propagation in LiveDebugValues was being reworked.

[djtodoro]

Thanks a lot for this!

Nevertheless, I am still interested in making assembly-based tests for this (and similar features) because it enables testing scenarios that we could not get (reliably or at all) a compiler to produce.

I also think this would be more stable if we can make assembler-based tests (but we'll need to address all archs from {x86_64, arm, aarch64}).
I am just wondering, what are the obstacles for writing the assembler-based tests? Is it LLDB testing infrastructure or writing tests itself?

lldb/test/API/functionalities/param_entry_vals/basic_entry_values/main.cpp

21–22

We should stop matching %rdi, as the purpose of the check is just to determine whether we really are testing entry value evaluation. However, llvm doesn't support entry value production for all platforms, so we would need to restrict the test to {x86_64, arm, aarch64} (still a clear improvement over the current situation).

+1
We can teach the TestBasicEntryValuesX86_64.py to keep arm and aarch64 as well.
Furthermore, we can add a function into decorators something like skipUnlessEntryValuesSupportedArch checking if the arch is in {x86_64, arm, aarch64}.

61–62

Yes.

[labath]

lldb/test/API/functionalities/param_entry_vals/basic_entry_values/main.cpp

21–22

Sounds good. (I'm not sure we even have functioning bots for non-x86, non-arm platforms).

34

I've verified that this works. The trick is that the //% lines need to be in a single continuous block. (The restriction not based on commands/statements, but text blocks -- the inline machinery does not understand where python statements end).

I made a note to my self to see if this restriction can be lifted.

[labath]

• make the test architecture-independent (and rename accordingly)

[djtodoro]

lldb/test/API/functionalities/param_entry_vals/basic_entry_values/TestBasicEntryValues.py

5

I haven't refreshed the page before submitting my previous comment.

WDYT about adding a decorators-function (e.g. skipUnlessEntryValuesSupportedArch) so we can use it in other tests as well?

[labath]

In D79491#2024647, @djtodoro wrote:

Thanks a lot for this!

Nevertheless, I am still interested in making assembly-based tests for this (and similar features) because it enables testing scenarios that we could not get (reliably or at all) a compiler to produce.

I also think this would be more stable if we can make assembler-based tests (but we'll need to address all archs from {x86_64, arm, aarch64}).
I am just wondering, what are the obstacles for writing the assembler-based tests? Is it LLDB testing infrastructure or writing tests itself?

A bit of both, maybe. Writing a test which works on a single target (os+arch) was relatively easy (for me, because I've done a lot of this stuff lately, maybe not so easy for others), but the difficulties started when I wanted to make that test run on other oses (which have different asm dialects). I was ok with leaving the test x86-specific, since most developers have an x86 machine and there is nothing arch-specific about this functionality. However, I did not want to restrict the test to any single OS, and since this test requires a running program, the asm needed to match what the host expects.

I did manage to ifdef around the asm platform quirks, but I still haven't managed to get the darwin linker to recognize my hand-written dwarf. I am sure this can be fixed (I think it's because I reduced the input too much), and I do want to try it out, but I am not sure the result will be something we can recommend as a general practice.

A different way to address this would be to remove the requirement for a running process. The test doesn't really require that, it just needs a relatively complex static snapshot of the process. A core file is just that, but we currently don't have any good way of creating such core files. If we had that, we could run this test over a core file. That would still be platform specific, but then it wouldn't matter (so much) because it wouldn't be tied to the host platform.

[labath]

lldb/test/API/functionalities/param_entry_vals/basic_entry_values/TestBasicEntryValues.py

5

I think that's a good idea, and that the check can be folded into the existing skipUnlessHasCallSiteInfo decorator.

[djtodoro]

In D79491#2024771, @labath wrote:

In D79491#2024647, @djtodoro wrote:

Thanks a lot for this!

Nevertheless, I am still interested in making assembly-based tests for this (and similar features) because it enables testing scenarios that we could not get (reliably or at all) a compiler to produce.

I also think this would be more stable if we can make assembler-based tests (but we'll need to address all archs from {x86_64, arm, aarch64}).
I am just wondering, what are the obstacles for writing the assembler-based tests? Is it LLDB testing infrastructure or writing tests itself?

A bit of both, maybe. Writing a test which works on a single target (os+arch) was relatively easy (for me, because I've done a lot of this stuff lately, maybe not so easy for others), but the difficulties started when I wanted to make that test run on other oses (which have different asm dialects). I was ok with leaving the test x86-specific, since most developers have an x86 machine and there is nothing arch-specific about this functionality. However, I did not want to restrict the test to any single OS, and since this test requires a running program, the asm needed to match what the host expects.

I did manage to ifdef around the asm platform quirks, but I still haven't managed to get the darwin linker to recognize my hand-written dwarf. I am sure this can be fixed (I think it's because I reduced the input too much), and I do want to try it out, but I am not sure the result will be something we can recommend as a general practice.

I see.. Enabling it only for specific OSes could be a temp solution, but we can discuss about that.

A different way to address this would be to remove the requirement for a running process. The test doesn't really require that, it just needs a relatively complex static snapshot of the process. A core file is just that, but we currently don't have any good way of creating such core files. If we had that, we could run this test over a core file. That would still be platform specific, but then it wouldn't matter (so much) because it wouldn't be tied to the host platform.

Good idea! A corefile could be solution, and I believe LLDB parses corefiles from other architectures very well (as multiarch GDB does).

lldb/test/API/functionalities/param_entry_vals/basic_entry_values/TestBasicEntryValues.py

5

Please note that entry values and call site info are not the same thing. Entry values implementation ended up describing target-specific instructions (in terms of DWARF operations) that loads values into registers that transfers the parameters, and that is used for the entry values printing. Eventually, we will have all targets supporting the entry values, but the call site Info is something different. The call site info is controlled by the DIFlagAllCallsDescribed flag, and it is generated by the language front-end, according to DWARF standard. Therefore, it makes sense to me to add new independent function (skipUnlessEntryValuesSupportedArch), but the whole feature depends on both skipUnlessEntryValuesSupportedArch and skipUnlessHasCallSiteInfo.

[vsk]

LGTM, thanks. It'd be nice to have a skipUnlessEntryValuesSupportedArch decorator, but I don't think that has to be folded into this change.

[djtodoro]

It'd be nice to have a skipUnlessEntryValuesSupportedArch decorator, but I don't think that has to be folded into this change.

OK. I agree.

LGTM. Thanks!

[labath]

In D79491#2024849, @djtodoro wrote:

I see.. Enabling it only for specific OSes could be a temp solution, but we can discuss about that.

A different way to address this would be to remove the requirement for a running process. The test doesn't really require that, it just needs a relatively complex static snapshot of the process. A core file is just that, but we currently don't have any good way of creating such core files. If we had that, we could run this test over a core file. That would still be platform specific, but then it wouldn't matter (so much) because it wouldn't be tied to the host platform.

Good idea! A corefile could be solution, and I believe LLDB parses corefiles from other architectures very well (as multiarch GDB does).

Yes, the trick there will be to figure out a core file representation that is both readable and expressive enough to work for this as well as various other use cases (I think this approach would be very useful for testing general unwinding machinery).

lldb/test/API/functionalities/param_entry_vals/basic_entry_values/TestBasicEntryValues.py

5

Ah, right. Yes, that makes sense. I'll create a separate patch for the new decorator.

[dblaikie]

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

lldb/test/API/functionalities/param_entry_vals/basic_entry_values/main.cpp

17–18

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

20

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?

26–28

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

47–49

Not sure why the two parameters here are interesting, compared to 1 (and, I'd probably prefer just "int", seems simpler)

77

Any idea if 2 inline frames are especially interesting, rather than just 1? If so, might be worth explaining why.

[omjavaid]

This breaks on arm-linux. I am going remove arm temporarily.

lldb/test/API/functionalities/param_entry_vals/basic_entry_values/Makefile

 CXX_SOURCES := main.cpp
 CXXFLAGS_EXTRAS := -O2 -glldb
 include Makefile.rules

lldb/test/API/functionalities/param_entry_vals/basic_entry_values/TestBasicEntryValues.py

+from lldbsuite.test import lldbinline
+from lldbsuite.test.decorators import *
+from lldbsuite.test import lldbplatformutil
+
+supported_archs = ["x86_64", "arm", "aarch64"]
+
+lldbinline.MakeInlineTest(__file__, globals(),
+        [skipIf(archs=no_match(supported_archs)),
+         skipIf(compiler="clang", compiler_version=['<', '10.0']),
+         skipUnlessHasCallSiteInfo,
+         skipIf(dwarf_version=['<', '4'])])

lldb/test/API/functionalities/param_entry_vals/basic_entry_values/main.cpp

-// Note: This test requires the SysV AMD64 ABI to be in use, and requires
-// compiler support for DWARF entry values.
+// Note: This test requires compiler support for DWARF entry values.
 
-// Inhibit dead-arg-elim by using 'x'.
-template<typename T> __attribute__((noinline)) void use(T x) {
-  asm volatile (""
-      /* Outputs */  :
-      /* Inputs */   : "g"(x)
-      /* Clobbers */ :
-  );
-}
-
-// Destroy %rsi in the current frame.
-#define DESTROY_RSI \
-  asm volatile ("xorq %%rsi, %%rsi" \
-      /* Outputs */  : \
-      /* Inputs */   : \
-      /* Clobbers */ : "rsi" \
-  );
-
-// Destroy %rbx in the current frame.
-#define DESTROY_RBX \
-  asm volatile ("xorq %%rbx, %%rbx" \
-      /* Outputs */  : \
-      /* Inputs */   : \
-      /* Clobbers */ : "rbx" \
-  );
+int dummy;
+volatile int global = 0;
+
+template <typename... T> __attribute__((optnone)) void use(T...) {}
 
 struct S1 {
   int field1 = 123;
   int *field2 = &field1;
 };
 
-__attribute__((noinline))
-void func1(int &sink, int x) {
-  use(x);
-
-  // Destroy 'x' in the current frame.
-  DESTROY_RSI;
-
-  // NOTE: Currently, we do not generate DW_OP_entry_value for the 'x',
-  // since it gets copied into a register that is not callee saved,
-  // and we can not guarantee that its value has not changed.
-
-  ++sink;
-
-  // Destroy 'sink' in the current frame.
-  DESTROY_RBX;
+__attribute__((noinline)) void func1(int &sink) {
+  // First use works around a compiler "bug" where an unused variable gets
+  // no location descriptions. The second use overwrites the function arguments
+  // with other values.
+  use<int &>(sink);
+  use<int &>(dummy);
 
+  ++global;
   //% self.filecheck("image lookup -va $pc", "main.cpp", "-check-prefix=FUNC1-DESC")
-  // FUNC1-DESC: name = "sink", type = "int &", location = DW_OP_entry_value(DW_OP_reg5 RDI)
+  // FUNC1-DESC: name = "sink", type = "int &", location = DW_OP_entry_value
 }
 
 __attribute__((noinline))
 void func2(int &sink, int x) {
-  use(x);
-
-  // Destroy 'x' in the current frame.
-  DESTROY_RSI;
+  use<int &, int>(sink, x);
+  use<int &, int>(dummy, 0);
 
-  //% 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
+  ++global;
+  //% self.filecheck("expr x", "main.cpp", "-check-prefix=FUNC2-EXPR1")
+  //% self.filecheck("expr sink", "main.cpp", "-check-prefix=FUNC2-EXPR2")
+  // FUNC2-EXPR1: ${{.*}} = 123
+  // FUNC2-EXPR2: ${{.*}} = 2
 }
 
 __attribute__((noinline))
 void func3(int &sink, int *p) {
-  use(p);
-
-  // Destroy 'p' in the current frame.
-  DESTROY_RSI;
+  use<int &, int *>(sink, p);
+  use<int &, int *>(dummy, nullptr);
 
   //% self.filecheck("expr *p", "main.cpp", "-check-prefix=FUNC3-EXPR")
   // FUNC3-EXPR: (int) ${{.*}} = 123
-
-  ++sink;
 }
 
 __attribute__((noinline))
 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
+  use<int &, int>(sink, x);
+  use<int &, int>(dummy, 0);
 
-  ++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.
+  ++global;
+  //% self.filecheck("expr x", "main.cpp", "-check-prefix=FUNC4-EXPR-FAIL",
+  //%     expect_cmd_failure=True)
+  //% self.filecheck("expr sink", "main.cpp","-check-prefix=FUNC4-EXPR",
+  //%     expect_cmd_failure=True)
+  // FUNC4-EXPR-FAIL: couldn't get the value of variable x: Could not evaluate DW_OP_entry_value.
+  // FUNC4-EXPR: couldn't get the value of variable sink:
+  // Could not evaluate DW_OP_entry_value.
 }
 
 __attribute__((noinline))
 void func5_amb() {}
 
 __attribute__((noinline))
 void func6(int &sink, int x) {
   if (sink > 0)
     func4_amb(sink, x); /* tail (taken) */
   else
     func5_amb(); /* tail */
 }
 
 __attribute__((noinline))
 void func7(int &sink, int x) {
   //% self.filecheck("bt", "main.cpp", "-check-prefix=FUNC7-BT")
   // FUNC7-BT: func7
   // FUNC7-BT-NEXT: [inlined] func8_inlined
   // FUNC7-BT-NEXT: [inlined] func9_inlined
   // FUNC7-BT-NEXT: func10
-  use(x);
-
-  // Destroy 'x' in the current frame.
-  DESTROY_RSI;
-
-  //% self.filecheck("expr x", "main.cpp", "-check-prefix=FUNC7-EXPR-FAIL", expect_cmd_failure=True)
-  // FUNC7-EXPR-FAIL: couldn't get the value of variable x: variable not available
+  use<int &, int>(sink, x);
+  use<int &, int>(dummy, 0);
 
-  ++sink;
-
-  // Destroy 'sink' in the current frame.
-  DESTROY_RBX;
-
-  //% self.filecheck("expr sink", "main.cpp", "-check-prefix=FUNC7-EXPR")
-  // FUNC7-EXPR: ${{.*}} = 4
+  ++global;
+  //% self.filecheck("expr x", "main.cpp", "-check-prefix=FUNC7-EXPR1")
+  //% self.filecheck("expr sink", "main.cpp", "-check-prefix=FUNC7-EXPR2")
+  // FUNC7-EXPR1: ${{.*}} = 123
+  // FUNC7-EXPR2: ${{.*}} = 5
 }
 
 __attribute__((always_inline))
 void func8_inlined(int &sink, int x) {
   func7(sink, x);
 }
 
 __attribute__((always_inline))
 void func9_inlined(int &sink, int x) {
   func8_inlined(sink, x);
 }
 
 __attribute__((noinline, disable_tail_calls))
 void func10(int &sink, int x) {
   func9_inlined(sink, x);
 }
 
 __attribute__((noinline))
 void func11_tailcalled(int &sink, int x) {
   //% self.filecheck("bt", "main.cpp", "-check-prefix=FUNC11-BT")
   // FUNC11-BT: func11_tailcalled{{.*}}
   // FUNC11-BT-NEXT: func12{{.*}} [artificial]
-  use(x);
-
-  // Destroy 'x' in the current frame.
-  DESTROY_RSI;
+  use<int &, int>(sink, x);
+  use<int &, int>(dummy, 0);
 
-  //% self.filecheck("expr x", "main.cpp", "-check-prefix=FUNC11-EXPR-FAIL", expect_cmd_failure=True)
-  // FUNC11-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=FUNC11-EXPR")
-  // FUNC11-EXPR: ${{.*}} = 5
+  ++global;
+  //% self.filecheck("expr x", "main.cpp", "-check-prefix=FUNC11-EXPR1")
+  //% self.filecheck("expr sink", "main.cpp", "-check-prefix=FUNC11-EXPR2")
+  // FUNC11-EXPR1: ${{.*}} = 123
+  // FUNC11-EXPR2: ${{.*}} = 5
 }
 
 __attribute__((noinline))
 void func12(int &sink, int x) {
   func11_tailcalled(sink, x);
 }
 
 __attribute__((noinline))
 void func13(int &sink, int x) {
   //% self.filecheck("bt", "main.cpp", "-check-prefix=FUNC13-BT")
   // FUNC13-BT: func13{{.*}}
   // FUNC13-BT-NEXT: func14{{.*}}
-  use(x);
-
-  // Destroy 'x' in the current frame.
-  DESTROY_RSI;
-
-  //% self.filecheck("expr x", "main.cpp", "-check-prefix=FUNC13-EXPR-FAIL", expect_cmd_failure=True)
-  // FUNC13-EXPR-FAIL: couldn't get the value of variable x: variable not available
-
-  use(sink);
+  use<int &, int>(sink, x);
+  use<int &, int>(dummy, 0);
 
-  // Destroy 'sink' in the current frame.
-  DESTROY_RBX;
+  ++global;
 
-  //% self.filecheck("expr sink", "main.cpp", "-check-prefix=FUNC13-EXPR")
-  // FUNC13-EXPR: ${{.*}} = 5
+  //% self.filecheck("expr x", "main.cpp", "-check-prefix=FUNC13-EXPR1")
+  //% self.filecheck("expr sink", "main.cpp", "-check-prefix=FUNC13-EXPR2")
+  // FUNC13-EXPR1: ${{.*}} = 123
+  // FUNC13-EXPR2: ${{.*}} = 5
 }
 
 __attribute__((noinline, disable_tail_calls))
 void func14(int &sink, void (*target_no_tailcall)(int &, int)) {
   // Move the call target into a register that won't get clobbered. Do this
   // by calling the same indirect target twice, and hoping that regalloc is
   // 'smart' enough to stash the call target in a non-clobbered register.
   //
   // llvm.org/PR43926 tracks work in the compiler to emit call targets which
   // describe non-clobbered values.
   target_no_tailcall(sink, 123);
   target_no_tailcall(sink, 123);
 }
 
 __attribute__((disable_tail_calls))
 int main() {
   int sink = 0;
   S1 s1;
 
   // Test location dumping for DW_OP_entry_value.
-  func1(sink, 123);
+  func1(sink);
 
+  sink = 2;
   // Test evaluation of "DW_OP_constu" in the parent frame.
   func2(sink, 123);
 
   // Test evaluation of "DW_OP_fbreg -24, DW_OP_deref" in the parent frame.
   // Disabled for now, see: llvm.org/PR43343
 #if 0
   func3(sink, s1.field2);
 #endif
 
   // The sequences `main -> func4 -> func{5,6}_amb -> sink` are both plausible.
   // Test that lldb doesn't attempt to guess which one occurred: entry value
   // evaluation should fail.
   func6(sink, 123);
 
+  sink = 5;
   // Test that evaluation can "see through" inlining.
   func10(sink, 123);
 
   // Test that evaluation can "see through" tail calls.
   func12(sink, 123);
 
   // Test that evaluation can "see through" an indirect tail call.
   func14(sink, func13);
 
   return 0;
 }

lldb/test/API/functionalities/param_entry_vals/basic_entry_values_x86_64/TestBasicEntryValuesX86_64.py

-from lldbsuite.test import lldbinline
-from lldbsuite.test import decorators
-from lldbsuite.test import lldbplatformutil
-
-supported_platforms = ["linux"]
-supported_platforms.extend(lldbplatformutil.getDarwinOSTriples())
-
-lldbinline.MakeInlineTest(__file__, globals(),
-        [decorators.skipIf(bugnumber="llvm.org/PR44774"),
-         decorators.skipUnlessPlatform(supported_platforms),
-         decorators.skipIf(compiler="clang", compiler_version=['<', '10.0']),
-         decorators.skipUnlessArch('x86_64'),
-         decorators.skipUnlessHasCallSiteInfo,
-         decorators.skipIf(dwarf_version=['<', '4'])])