[llvm-commits] [llvm] r162034 - /llvm/trunk/include/llvm/Object/ELF.h

I'm adding the dev list to this discussion, as it's a bit meta, and not
specific to these patches.

> > Test cases?
> We discussed this a bit on IRC. It is my impression that we already
> went too far with PPC ELF without testing. As Roman itself noticed two
> days ago, it is possible to break hello word on ppc elf and not notice
> it:

> We have been too slow to add tests, even when we noticed that entire
> functions could be deleted and the tests would still pass:
> (which btw, is still the case on trunk).
> So sorry for the current state for those starting to hack now on pcc
> elf, but to make sure we are in a better position tomorrow I think we
> have to start requiring tests as features are added and hopefully also
> cover the backlog we have so far.

Hi all,
The changes so far are an attempt to get existing test cases to run. So
I'd argue against the changes being considered new features. :slight_smile:

Yea, new features is probably the wrong word. Let's say "new code". =]

2003-01-04-ArgumentBug.ll is the one chosen at random that Adhemerval
has been focused on. That wasn't called out explicitly, and we do
expect some number of test cases to begin functioning better once this
initial batch of patches gets worked into the tree.

This is good, but where is this test? :: runs find :: Ah, its in the JIT.
Ok, it's not clear that this is the best test to start with, but let's not
get ahead of ourselves...

What got us in trouble here is that we didn't understand the
'test-suite' as being a separate batch of tests from 'make check', and
simply didn't know to run it. (Until after Roman hit them and reported
the regressions to us).

The test-suite is good to run, but it's not really what Rafael or I are
referring to here.

Tests in LLVM can be broken down into a few categories:

1) Direct unittests of core APIs in llvm/unittests
2) Direct testing of the output of the various analyses, transformations,
etc., in llvm/test
3) Testing the execution of compiled small programs by building them and
running them on test hardware (possibly the host, possibly not). These are
in test-suite/SingleSource/UnitTests and
4) Testing the execution of large "real world" programs in the nightly test
suite after building with the Clang/LLVM toolchain. These consist of
everything else in the test-suite/, especially the test-suite/MultiSource
side of things

We don't expect the latter two to be used on every single check-in. We have
build bots that run them, and you should use them whenever changing
something in a way that is risky or may well have failures that only show
up in the last "whole system" testing pattern.

We expect nearly every patch[1] to be covered to some extent either by
unittests of the APIs or by FileCheck-driven output-tests in 'test/...'.
The reason for this split in importance is that these first two don't
require any particular hardware or execution environment, and thus are
suitable to require every developer run and pass on every checkin. This is
what Rafael and I are looking for in these patches, as they will ensure
developers who don't have a PowerPC system to run any execution tests on
don't immediately break that backend. =]

Now, as Rafael is alluding to, there is an unfortunate reality: we don't
have enough tests in the tree to cover the *existing* PowerPC support, much
less to cover your additions. That means you may need to add tests for the
existing PPC support, just to be able to fix a bug in and have a test that
covers that fix . I hope this isn't too much of a burden, as it will
greatly improve the quality of our PPC support.

The common pattern when building up support for a platform is to take some
of the execution tests in #3 and #4 which aren't working correctly, figure
out what isn't working, and what the output of the compiler *should* be in
order for it to work. Then to write a very small test which produces the
wrong output, and use FileCheck within it to assert that the correct output
is instead produced, and include that with the fix as a new test in #1 or
#2. This way, we won't regress even if we don't run the complete test

If everything currently in #3 and #4 works correctly, but other programs
don't, my suggestion would be to follow essentially the same process with
whatever system-tests you have that are failing. We're hoping to make it
easier in the future to contribute new system tests to #4, but currently
it's a bit painful.

Now, the JIT tests are just really really special. Honestly, I would ignore
them and JIT in general until the normal Clang/LLVM static compilation
pipeline is working well and well tested. Then ask on llvmdev for how best
to test JIT problems if you still have them. =]

Hopefully not too much information overload! =] Let me know if I can
clarify anything.

PS: You really don't need to read the foot notes... ;] I'm long winded.

[1] Yea, "nearly" is kind-of annoying here. If you're curious, here is how
I would break it down further:

There are a few things inside of LLVM that are not in an easily testable
state. For example, the inner details of the register allocator. Some of
these cases we can't add tests for because we have to have a function that
needs N-thousands of virtual registers before the bad code path is taken.
We're actively trying to reduce the amount of LLVM's pipeline that is
subject to this though, so its important to try to find a way to test these
things first.

There are also patches where adding a test is infeasible: for example
patches that make algorithms not be n^2 or exponential. In some cases, the
inputs that trigger these are necessarily too large to include in a
human-readable test suite.

Finally there are lots of patches which just don't need testing. These are
refactorings, cleanups, and other things which don't actually add, remove,
or change functionality, and thus they should already be covered by
existing tests.

Chandler, thanks for the detailed discussion! It's very useful to have
clear expectations on testing.

Much of this is explained in the docs at
http://llvm.org/docs/TestingGuide.html, but I'm surprised to see there
isn't any mention of llvm/unittests in that document. Are the core API
tests run as part of "gmake check-all" in the build directory, or is
there another invocation required to run them? (It would probably be
good to mention these tests in the testing guide in any case.)

Regarding the state of PPC ELF: We definitely understand the need to
include test cases with changes we make to the functionality. Because
of certain schedule pressures I can't get into in detail, though, we
won't be able to spend much time trying to fix up missing tests for
already existing functionality, at least in the short term. Hopefully
the people who added that logic will be able to step up to that. There
is already a great deal to do to get the port into compliance with the
ELF ABI. We do recognize that it's in our interest to have full test
coverage; it's just that we have quite a small team and a lot to do in a
short time, and we have to make priority decisions.

We are approaching LLVM on a couple of different trajectories based on
separate customer needs. On the one hand we have some folks very
interested in the static compilation side with ABI compliance, and on
the other we have people needing JIT functionality where
self-consistency is all that's needed (rather than strict compliance).
Because of this, your suggestion to ignore the JIT for now in favor of
Clang/LLVM static compilation is probably not one we can follow. In a
perfect world, that's how I would like to do things as well, but as
usual, reality bites.

(Side note: Strangely, despite our team being so small, two of us are
named William Schmidt. He goes by Will, I go by Bill. Sorry in advance
for the inevitable confusion this will cause...)

Thanks again for the help, and we look forward to working with the
community on strengthening the PowerPC port!

-- Bill

Bill Schmidt, Ph.D.
IBM Advance Toolchain for PowerLinux
IBM Linux Technology Center