Page MenuHomePhabricator

[lldb] Revive TestBasicEntryValuesX86_64
ClosedPublic

Authored by labath on Wed, May 6, 7:27 AM.

Details

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

Diff Detail

Event Timeline

labath created this revision.Wed, May 6, 7:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptWed, May 6, 7:27 AM
labath marked 2 inline comments as done.Wed, May 6, 7:36 AM

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_x86_64/main.cpp
0

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 added a comment.Wed, May 6, 12:03 PM

Thanks for working on this!

lldb/test/API/functionalities/param_entry_vals/basic_entry_values_x86_64/main.cpp
0

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

7

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.

8

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.

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_x86_64/main.cpp
0

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 marked 6 inline comments as done.Thu, May 7, 3:45 AM
labath added inline comments.
lldb/test/API/functionalities/param_entry_vals/basic_entry_values_x86_64/main.cpp
0

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

7

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 updated this revision to Diff 262595.Thu, May 7, 3:49 AM
labath marked an inline comment as done.
  • make the test architecture-independent (and rename accordingly)
djtodoro added inline comments.Thu, May 7, 4:12 AM
lldb/test/API/functionalities/param_entry_vals/basic_entry_values/TestBasicEntryValues.py
6

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 marked an inline comment as done.Thu, May 7, 4:26 AM

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 marked an inline comment as done.Thu, May 7, 4:38 AM
labath added inline comments.
lldb/test/API/functionalities/param_entry_vals/basic_entry_values/TestBasicEntryValues.py
6

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

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
6

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 accepted this revision.Thu, May 7, 10:35 AM

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

This revision is now accepted and ready to land.Thu, May 7, 10:35 AM
djtodoro accepted this revision.Thu, May 7, 11:19 PM

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 marked an inline comment as done.Mon, May 11, 7:11 AM

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
6

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

This revision was automatically updated to reflect the committed changes.

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

lldb/test/API/functionalities/param_entry_vals/basic_entry_values_x86_64/main.cpp
1

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?

4–5

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

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