Page MenuHomePhabricator

[test-suite] Fix support for user mode emulation when using cmake/lit.
ClosedPublic

Authored by kristof.beyls on May 6 2019, 7:01 AM.

Details

Summary

When using user mode emulation, i.e. cross-compiling programs for a
different target and running them on a host under qemu user mode
emulation, timeit and fpcmp should have host versions, not target
versions.

Running under user mode emulation has been broken for a while,
presumably since https://reviews.llvm.org/rT341257

I first tried an alternative approach where fpcmp would be run under qemu user mode emulation too.
That in itself worked, but if going for that approach, for orthogonality reasons, we probably should also run the other helper programs as if they were running on the target, i.e. also under qemu user mode emulation.
I ran into issues with running timeit under qemu user mode emulation and also running RunSafely.sh under user mode emulation doesn't seem trivial.

In the end, it seemed better to me to explicitly add a cmake option to mark that we're running under qemu user mode emulation, and in that mode, only aim to run the test/benchmark under qemu user mode emulation, rather than also all the helper programs (such as fpcmp, timeit, RunSafely.sh) under it (which is what would be needed if we just kept on using only the RUN_UNDER option for qemu user mode emulation.

Diff Detail

Event Timeline

kristof.beyls created this revision.May 6 2019, 7:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 6 2019, 7:01 AM
Herald added a subscriber: mgorny. · View Herald Transcript

I haven't created any tests for this yet - I thought I'd first try and get feedback on whether this approach seems right before I spent time learning about how to get the test-suite unit/regression tests set up.

lenary added a subscriber: lenary.Jul 4 2019, 5:10 AM

We have also been running into issues trying to run the test suite under qemu (for risc-v).

This patch seems (at first glance) to solve some of these issues. I think it's good that there is now an explicit "TEST_SUITE_USER_MODE_EMULATION" flag, which can be used in a variety of parts of the codebase (including the lit configuration) where required, and declares intent a little better than just RUN_UNDER alone.

I'm happy to report that our configuration of the suite, when we apply this patch, is now using the correct timeit and fpcmp binaries.

I would like you to fix the CMake issue I note below.

CMakeLists.txt
237

Currently this patch only copes with using 1 to denote True, which is less than CMake normally accepts ("On" is a good, explicit value that CMake allows, in addition to "True").

However, there is a hack that this CMake configuration uses for TEST_SUITE_PROFILE_GENERATE which we can replicate inside this if: set the value to an explicitly python-like string "True" (in this branch) and "False" (in an else branch). Then you can just directly set config.user_mode_emulation = @TEST_SUITE_USER_MODE_EMULATION@ in lit.site.cfg.in, and the configuration should be less brittle to exactly which boolean value that someone chooses at the command line for CMake.

In the future, it would probably make sense to have a python function for lit.site.cfg.in which can parse any cmake-valid boolean from a string into the correct python boolean, which feels like a nicer solution, but can come along later.

asb added a subscriber: asb.Jul 4 2019, 7:57 AM

Thanks for the feedback Sam!

I've followed your suggestion to set the value of TEST_SUITE_USER_MODE_EMULATION in a similar way as TEST_SUITE_PROFILE_GENERATE. I agree that's a bit cleaner.

I also looked further into creating a test in the test-suite's regression tests (in litsupport-tests) for this new functionality. However, the framework that exists there is really only for the lit-part of the running tests, not the cmake part of configuring.
Since this patch is fully on the cmake side, I don't think there is any way to add a test in litsupport-tests for this. A testing framework would need to be added to test the cmake logic of the test-suite if we'd want to test this. I've decided not to try and do that for now.

I wonder what the other issues are you're seeing after applying this patch?
I also wonder if you've also tried the corresponding lnt patch at D61598 that makes it possible to use this new functionality from lnt runtest test-suite?

I wonder what the other issues are you're seeing after applying this patch?
I also wonder if you've also tried the corresponding lnt patch at D61598 that makes it possible to use this new functionality from lnt runtest test-suite?

I don't think I've run into any further issues (that are the test-suite's fault) since applying your patch.

Thanks for pointing me to that patch for LNT. I will see if it works for me. Until yesterday I was just invoking cmake directly, not via LNT, but now I need LNT's CSV-making powers.

I'd be happy for this to land, but I don't know what else needs to be checked before it can.

I wonder what the other issues are you're seeing after applying this patch?
I also wonder if you've also tried the corresponding lnt patch at D61598 that makes it possible to use this new functionality from lnt runtest test-suite?

I don't think I've run into any further issues (that are the test-suite's fault) since applying your patch.

Thanks for pointing me to that patch for LNT. I will see if it works for me. Until yesterday I was just invoking cmake directly, not via LNT, but now I need LNT's CSV-making powers.

FWIW, I also have a downstream patch for LNT to use comma as the separator, rather than semi-colon. I find that results in less clicking being needed when opening the CSV file in Excel. Happy to upstream that too if you also find out that commas, rather than semi-colons, as a field separator in CSV files works better.

I'd be happy for this to land, but I don't know what else needs to be checked before it can.

I don't think anything else needs to be checked. It'd be great if the test-suite itself had a regression testing system as good as e.g. llvm or clang so that with regression tests even more confidence could be had in changes made. But that is an orthogonal issue.

Could you on this review mark that you've approved this patch ("Accept Revision")? Maybe you'll need to first make yourself a reviewer to be able to do that - I'm not sure.

lenary accepted this revision.Jul 11 2019, 2:21 AM

FWIW, I also have a downstream patch for LNT to use comma as the separator, rather than semi-colon. I find that results in less clicking being needed when opening the CSV file in Excel. Happy to upstream that too if you also find out that commas, rather than semi-colons, as a field separator in CSV files works better.

I don't mind so much about the CSV being semicolon-separated, I can cope with the extra clicks, and I don't want to break anyone else's systems. That said, happy for you to post the patch so further discussion can happen.

Could you on this review mark that you've approved this patch ("Accept Revision")? Maybe you'll need to first make yourself a reviewer to be able to do that - I'm not sure.

Sure, done!

This revision is now accepted and ready to land.Jul 11 2019, 2:21 AM
This revision was automatically updated to reflect the committed changes.