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

[Djordje Todorovic via Phabricator via lldb-commits]

djtodoro added a comment.

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?



================
Comment at: lldb/test/API/functionalities/param_entry_vals/basic_entry_values_x86_64/main.cpp:21-22
+  ++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)
 }
----------------
labath wrote:
> vsk wrote:
> > labath wrote:
> > > 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)?
> > 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).
> Sounds good. (I'm not sure we even have functioning bots for non-x86, non-arm platforms).
>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}.


================
Comment at: lldb/test/API/functionalities/param_entry_vals/basic_entry_values_x86_64/main.cpp:61-62
-
-  //% 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
-
----------------
vsk wrote:
> labath wrote:
> > 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.
> IIRC this is left over from when entry value propagation in LiveDebugValues was being reworked.
Yes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79491