This is an archive of the discontinued LLVM Phabricator instance.

[test-suite] Towards runtime tests for stackmaps
Needs ReviewPublic

Authored by vext01 on May 27 2022, 8:28 AM.

Details

Summary

Hi,

As you know, I've been improving testing for stackmaps.

The LLVM test suite contains static stackmap tests: static in the sense that they only check that the emitted stackmap section looks somewhat feasible.

I wanted to add tests that check that the locations described in the stackmap section do in fact hold the correct data *at runtime*.

This diff is a start at that. It needs more work (test more types and location kinds) but I wanted to raise a "draft" for early comments.

OK, so how does this work?

For each architecture there is a input_$ARCH.ll containing a function called @run_test. In that function is a series of pairs of calls like:

call void llvm.experimental.stackmap(i64 0, i32 0, i32 50);
call void sm_inspect()

The former emits the stackmap section at compile time, the latter checks *at runtime* that the storage referenced by the stackmap: a) is in the expected kind of storage (register, memory, constant etc.), and b) contains the right value. It does this by printing stuff to stdout which is then checked by your usual llvm-lit reference output stuff.

(For the above example, we expect some bytes stored directly inside the stackmap record's constant field to be, when casted to i32 , the integer value 50).

I've only made an x86_64 input so far. This demonstrates constant integers and integers in registers.

One nasty thing I've realised is that there is no guarantee that the live state isn't trashed between the call to llvm.experimental.stackmap() and sm_inspect(). It works in practice (by chance?) but only because:

  • sm_inspect() takes no arguments, so the argument registers are untouched.
  • I've added some notail, as tail call optimisations were moving stuff around between the two calls.

But in general, I think the compiler could do anything it wants to realise the call to sm_inspect(). How do we feel about this? Maybe we can't reliably test llvm.experimental.stackmap, but we may be able to test llvm.experimental.patchpoint as it emits stackmaps *and* does a call "atomically". There should be no window in which state could be trashed(?). I've not tried that, it's just a theory.

Other small questions:

  • Is LLVM going to be deterministic with regards to where values are being stored?
  • Do we need to test different optimisation levels? It occurs to me that I'm not sure what opt level I'm actually testing here!
  • In CMakelists.txt is ${ARCH} the right variable to use? I'm only testing 64-bit x86 here, but ${ARCH} is x86 for me... Will it try to run my tests on 32-bit x86?

(If we want to test other arches, someone with the machines will need to add test inputs and reference outputs -- I only have x86_64).

Diff Detail

Event Timeline

vext01 created this revision.May 27 2022, 8:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 27 2022, 8:28 AM
vext01 requested review of this revision.May 27 2022, 8:28 AM
vext01 updated this revision to Diff 434817.Jun 7 2022, 8:00 AM

Further to my last comment, here's a revised diff which uses llvm.experimental.patchpoint instead of llvm.experimental.stackmap.

In addition:

  • The test input now specifies how many bytes should be inspected for each stackmap record (sm_set_inspect_sizes()) -- this allows us to reliably check values if the size is not known, or if the size reported by the stackmap would include undefined bytes that can't be tested.
  • Some changes to testing infrastructure to allow this to run only on x86_64 for now.

The tests are structured with one call to patchpoint per function, due to this bug: https://github.com/llvm/llvm-project/issues/35420

We don't yet test all of the various types, but I think this is good enough for now. Follow up diffs can test more stuff.

Any comments?

vext01 updated this revision to Diff 434832.Jun 7 2022, 8:36 AM

Sorry, there was a small mistake in the test inputs. Fixed.

Added a couple of reviewers: people who have recently worked in MultiSource/UnitTests. Hope that's OK.

aaron.ballman resigned from this revision.Jun 8 2022, 5:26 AM

Normally I'd be happy to review, but I don't feel confident I know enough about stackmaps or the test-suite to have much to say, so I'm resigning as a review for this one. You might try asking some of the folks who have previously reviewed your stackmap work if they could review it over here as well, as one possible way to get more review eyeballs on this.

vext01 added a comment.Jun 8 2022, 7:13 AM

You might try asking some of the folks who have previously reviewed your stackmap work if they could review it over here as well

The problem is, very few people know about stackmaps. @reames would be the ideal reviewer, but I assume he is otherwise engaged.

arsenm added a subscriber: arsenm.Jun 8 2022, 5:33 PM
arsenm added inline comments.
MultiSource/UnitTests/Stackmaps/CMakeLists.txt
2–3

I'm surprised to see text IR here. It's not stable so this could be problematic. Are there other existing tests written in text IR?

MultiSource/UnitTests/Stackmaps/Harness.cpp
2

cassert?

9

cstring etc

MultiSource/UnitTests/Stackmaps/StackMapParser.cpp
2

I don't know about test suite conventions but all of these are missing license headers

vext01 updated this revision to Diff 435465.Jun 9 2022, 2:29 AM

Address @arsenm's comments.

MultiSource/UnitTests/Stackmaps/CMakeLists.txt
2–3
MultiSource/UnitTests/Stackmaps/Harness.cpp
2

Thanks, I didn't know about these headers.

MultiSource/UnitTests/Stackmaps/StackMapParser.cpp
2

The other tests in MultiSource/UnitTest likewise have no license headers.

The way I interpret LICENSE.txt is that all files are under the LLVM license unless explicitly listed under "Software from third parties included in the LLVM Project".

vext01 added a comment.Jun 9 2022, 2:31 AM

Is there a way to run my new tests on other x86_64 platforms before we merge the change? I've only tested linux.

@arsenm ping. Would this be OK to land please?

arsenm added inline comments.Jun 15 2022, 8:37 AM
MultiSource/UnitTests/Stackmaps/Harness.cpp
46

puts?

I spoke with @lhames about this change on discord.

We are going to try and make this test an lli test in the main LLVM repo, where having .ll inputs would be more appropriate.

We've arranged to meet up in August, when @lhames is back from holidays. Until then, let's not let this hold up D125680.

arichardson resigned from this revision.Jun 27 2023, 4:25 PM

cleanup of review queue