Page MenuHomePhabricator

[lldb/lit] Introduce %clang_host substitutions
ClosedPublic

Authored by labath on Wed, Oct 30, 8:50 AM.

Details

Summary

This patch addresses an ambiguity in how our existing tests invoke the
compiler. Roughly two thirds of our current "shell" tests invoke the
compiler to build the executables for the host. However, there is also
a significant number of tests which don't build a host binary (because
they don't need to run it) and instead they hardcode a certain target.

We also have code which adds a bunch of default arguments to the %clang
substitutions. However, most of these arguments only really make sense
for the host compilation. So far, this has worked mostly ok, because the
arguments we were adding were not conflicting with the target-hardcoding
tests (though they did provoke an occasional "argument unused" warning).

However, this started to break down when we wanted to use
target-hardcoding clang-cl tests (D69031) because clang-cl has a
substantially different command line, and it was getting very confused
by some of the arguments we were adding on non-windows hosts.

This patch avoid this problem by creating separate %clang(xx,_cl)_host
substutitions, which are specifically meant to be used for compiling
host binaries. All funny host-specific options are moved there. To
ensure that the regular %clang substitutions are not used for compiling
host binaries (skipping the extra arguments) I employ a little
hac^H^H^Htrick -- I add an invalid --target argument to the %clang
substitution, which means that one has to use an explicit --target in
order for the compilation to succeed.

Diff Detail

Event Timeline

labath created this revision.Wed, Oct 30, 8:50 AM
Herald added a project: Restricted Project. · View Herald Transcript

this looks like a reasonable incremental improvement. That said, I'm wondering how useful clang_host is as a concept in the LIT tests. In my own mental model the tests are grouped into

  1. end-to-end tests that test lldb against a variety of user-configurable compilers and with different debug info formats
  2. unit-tests/internal consistency tests where we may use a compiler for the sake of not having to check in a binary, but where we expect a specific output from the compiler and thus should always use the just-built clang.

The packages subdirectory covers category (1) and the lit subdirectory covers category (2).

Based on that I would expect that we either always hard-code a triple in the lit tests (may result in less coverage if the target isn't available), or we run them for each available target. Giving the host target preference seems arbitrary, given how extensively LLDB is used on an x86_64 host, debugging AArch64 targets, for example.

Yes, I understand what you're saying, but I am afraid the situation is not as clear-cut as that. I would be very happy if it was, but it's definitely not the current situation, and given the complexities involved, I'm not sure if we should even aim for that goal. I mean, a lot of times these things end up being down to the personal preference of the author (or reviewer), but there are cases where writing a test of the appropriate category (based on your above classification) would be very hard. Lemme give some examples:

  • there is a subclass of "dotest" tests which test lldb's python api (most of them tagged with the "pyapi" category), which test lldb's python api. These definitely don't benefit from being run with multiple compilers (in fact, a lot of them don't run the compiler at all). I don't think it makes sense to change these into "lit" tests.
  • another class of "dotest" tests are those which test some interactive behavior of lldb (inferior control, thread plans, stepping and stuff). While these may touch debug info peripherally, it's usually not their main focus, and I don't think we should be relying on them on testing debug info. However, being able to run them in a more "interactive" manner is usually more convenient.
  • "pexpect" tests definitely don't/shouldn't test debug info, but they currently exist only in the "dotest" suite (though they are fairly independent, and they could be extracted if we wanted to)
  • on the other side of the fence, there are "lit" tests which do not test debug info, or even "debugging", but they still need a host executable, because debugging something is the only way to trigger some functionality. For instance the "register" tests need a functional executable to check that they can read/write registers from it. Unwind tests need a running executable because we can't test even basic unwind without a running process now. Reproducers need a debuggable executable too, but they don't really care about it's contents, etc.

So overall, while there definitely are tests which would be better off moved from one category or the other, I am doubtful that we'll be able to enforce a separation where all "configurable" tests are dotest tests, and all "lit" tests hardcode a triple. The one barrier we have right now is that it is not possible to run the "lit" suite with a different compiler, and that's something I think we should keep. And we can definitely change some %clang_hosts to a hard coded triple (I think that would remove some "UNSUPPORTED: windows" stanzas) and/or be stricter about using %clang_host in the future. In fact I am happy that you spoke out against %clang_host, because when I was going through these tests, I got the impression that the only tests which have a specific triple hard-coded are those that I have written myself. :)

For the test which don't need a host executable, I don't think that running them for each available target would be useful, as they generally don't care about those details. What they sometimes care about is the object file format (because of dwarf differences in elf vs. macho), so it may make sense to introduce some substitution like %elf_triple and %macho_triple, which would choose some elf flavour of one of the configured targets. However, I am afraid that even that would be of limited usefulness, as it's pretty hard to make a specific test which is target agnostic (I tried) -- there are differences in pointer sizes, and even the different assemblers can't agree on what they use as a comment character.

aprantl accepted this revision.Wed, Oct 30, 12:15 PM

The one barrier we have right now is that it is not possible to run the "lit" suite with a different compiler, and that's something I think we should keep.

Very much agreed.

For the test which don't need a host executable, I don't think that running them for each available target would be useful, as they generally don't care about those details.

That's plausible.

Thanks for sharing your thoughts!

This revision is now accepted and ready to land.Wed, Oct 30, 12:15 PM
JDevlieghere accepted this revision.Wed, Oct 30, 12:23 PM

All my thoughts have already been brought up and discussed. :-)

LGTM!

Thanks guys.

This revision was automatically updated to reflect the committed changes.