[cfe-dev] [llvm-dev] Testing Best Practices/Goals (in the context of compiler-rt)

[Alexey Samsonov via cfe-dev]

On Fri, Feb 26, 2016 at 1:11 PM, David Blaikie <dblaikie at gmail.com> wrote:

>
>
> On Fri, Feb 26, 2016 at 1:07 PM, Sean Silva <chisophugis at gmail.com> wrote:
>
>>
>>
>> On Wed, Feb 17, 2016 at 8:45 AM, David Blaikie via cfe-dev <
>> cfe-dev at lists.llvm.org> wrote:
>>
>>>
>>>
>>> On Fri, Feb 12, 2016 at 5:43 PM, Alexey Samsonov <vonosmas at gmail.com>
>>> wrote:
>>>
>>>>
>>>>
>>>> On Thu, Feb 11, 2016 at 1:50 PM, David Blaikie <dblaikie at gmail.com>
>>>> wrote:
>>>>
>>>>>
>>>>>
>>>>> On Wed, Feb 10, 2016 at 3:55 PM, Alexey Samsonov via cfe-dev <
>>>>> cfe-dev at lists.llvm.org> wrote:
>>>>>
>>>>>> I mostly agree with what Richard and Justin said. Adding a few notes
>>>>>> about the general strategy we use:
>>>>>>
>>>>>> (1) lit tests which look "end-to-end" proved to be way more
>>>>>> convenient for testing runtime libraries than unit tests.
>>>>>>
>>>>> We do have
>>>>>> the latter, and use them to provide test coverage for utility
>>>>>> functions, but we quite often accompany fix to the runtime library with
>>>>>> "end-to-end" small reproducer extracted from the real-world code that
>>>>>> exposed the issue.
>>>>>> Incidentally, this tests a whole lot of other functionality: Clang
>>>>>> driver, frontend, LLVM passes, etc, but it's not the intent of the test.
>>>>>>
>>>>>
>>>>> Indeed - this is analogous to the tests for, say, LLD that use llvm-mc
>>>>> to produce the inputs rather than checking in object files. That area is
>>>>> open to some discussion as to just how many tools we should rope in/how
>>>>> isolated we should make tests (eg: maybe building the json object file
>>>>> format was going too far towards isolation? Not clear - opinions differ).
>>>>> But the point of the test is to test the compiler-rt functionality that was
>>>>> added/removed/modified.
>>>>>
>>>>> I think most people are in agreement with that, while acknowledging
>>>>> the fuzzy line about how isolated we might be.
>>>>>
>>>>
>>>> Yes.
>>>>
>>>>
>>>>>
>>>>>
>>>>>> These tests are sometimes platform-specific and poorly portable, but
>>>>>> they are more reliable (we make the same steps as the
>>>>>> user of the compiler), and serve the purpose of documentation.
>>>>>>
>>>>>> (2) If we change LLVM instrumentation - we add a test to LLVM. If we
>>>>>> change Clang code generation or driver behavior - we add
>>>>>> a test to Clang. No excuses here.
>>>>>>
>>>>>> (3) Sometimes we still add a compiler-rt test for the change in LLVM
>>>>>> or Clang: e.g. if we enhance Clang frontend to teach UBSan
>>>>>> to detecting yet another kind of overflow, it makes sense to add a
>>>>>> test to UBSan test-suite that demonstrates it, in addition to
>>>>>> Clang test verifying that we emit a call to UBSan runtime. Also,
>>>>>> compiler-rt test would allow us to verify that the actual error report
>>>>>> we present to the user is sane.
>>>>>>
>>>>>
>>>>> This bit ^ is a bit unclear to me. If there was no change to the UBSan
>>>>> runtime, and the code generated by Clang is equivalent/similar to an
>>>>> existing use of the UBSan runtime - what is it that the new compiler-rt
>>>>> test is providing? (perhaps you could give a concrete example you had in
>>>>> mind to look at?)
>>>>>
>>>>
>>>> See r235568 (change to Clang) followed by r235569 (change to
>>>> compiler-rt test). Now, it's a cheat because I'm fixing test, not adding
>>>> it. However, I would have definitely added it, if it was missing.
>>>>
>>>
>>> Right, I think the difference here is "if it was missing" - the test
>>> case itself seems like it could be a reasonable one (are there other tests
>>> of the same compiler-rt functionality? (I assume the compiler-rt
>>> functionality is the implementation of sadd/ssub?))
>>>
>>>
>>>> In this case, a change to Clang
>>>> instrumentation (arguments passed to UBSan runtime callbacks) improved
>>>> the user-facing part of the tool, and compiler-rt test suite is a good
>>>> place to verify that.
>>>>
>>>
>>> This seems like the problematic part - changes to LLVM improve the
>>> user-facing part of Clang, but we don't add end-to-end tests of that, as a
>>> general rule. I'm trying to understand why the difference between that and
>>> compiler-rt
>>>
>>
>> In what way do changes in LLVM change the user-facing part of Clang?
>> It obviously depends on how broadly one defines user-facing. Is a 1%
>> performance improvement from a particular optimization user-facing? Is
>> better debug info accuracy user-facing? I'm not sure. But it seems clear
>> that "the user sees a diagnostic or not" definitely is.
>>
>
> There's more than just performance in LLVM - ABI features, and yes, I'd
> argue some pieces of debug info are pretty user facing (as are some
> optimizations). We also have the remarks system in place now. (also the
> compiler crashing (or not) is pretty user facing).
>
>
>> Also, I think part of this is that in compiler-rt there are usually more
>> moving parts we don't control. E.g. it isn't just the interface between
>> LLVM and clang. The information needs to pass through archivers, linkers,
>> runtime loaders, etc. that all may have issues that affect whether the user
>> sees the final result. In general the interface between LLVM and clang has
>> no middlemen so there really isn't anything to check.
>>
>
> Correctness/behavior of the compiler depends on those things too (linkers,
> loaders, etc) to produce the final working product the user requested. If
> we emitted symbols with the wrong linkage we could produce linker errors,
> drop important entities, etc. But we don't generally test that the output
> of LLVM/Clang produces the right binary when linked, we test that it
> produces the right linkages on the resulting entities.
>

Sure, but verifying the Clang compiles/links a certain source file
successfully can easily live in a dedicated test-suite, even after it's
moved to GitHub, because probably it would be compiler-agnostic, and
wouldn't change frequently.

That said, I wouldn't mind having more Clang end-to-end tests, but the
major issue with actually invoking linkers and using system libraries is
portability.
These portability requirements are a lesser issue for sanitizers, because
we support only a handful of platforms.



> - David
>
>
>>
>> -- Sean Silva
>>
>>
>>>
>>>
>>>>
>>>>
>>>
>>>> You may argue that Clang test would have been enough (I disagree with
>>>> that), or that it qualifies as "adding coverage" (maybe).
>>>>
>>>>
>>>>>
>>>>>
>>>>>> (4) True, we're intimidated by test-suite :) I feel that current use
>>>>>> of compiler-rt test suite to check compiler-rt libs better follows
>>>>>> the doctrine described by David.
>>>>>>
>>>>>
>>>>> Which David? ;) (I guess David Li, not me)
>>>>>
>>>>
>>>> Nope, paragraph 2 from your original email.
>>>>
>>>>
>>>>> I think maybe what could be worth doing would be separating out the
>>>>> broader/intentionally "end to end" sort of tests from the ones intended to
>>>>> test compiler-rt in relative isolation.
>>>>>
>>>>
>>>> It's really hard to draw the line here, even some of compiler-rt unit
>>>> tests require instrumentation, therefore depend on new features of
>>>> Clang/LLVM. Unlike builtins, which are
>>>> trivial to test in isolation, testing sanitizer runtimes in isolation
>>>> (w/o compiler) is often hard to implement (we tried to do so for TSan, but
>>>> found unit tests extremely hard to write),
>>>> and is barely useful - compiler-rt runtimes don't consist of modules
>>>> (like LLVMCodeGen and LLVMMC for instance), and are never used w/o the
>>>> compiler anyway.
>>>>
>>>>
>>>>>
>>>>> Most importantly, I'd expect only the latter to run in a "make
>>>>> check-all" run, as we do for Clang/LLVM, etc.
>>>>>
>>>>
>>>> And now we're getting to the goals :) Why would such a change be good?
>>>> Do you worry about the time it takes to execute the test suite?
>>>>
>>>>
>>>>>
>>>>>
>>>>>> Also, there's significant complexity in compiler-rt test suite that
>>>>>> narrows the tests executed
>>>>>> to those supported by current host.
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>>
>>>>>> On Wed, Feb 10, 2016 at 2:33 PM, Xinliang David Li via cfe-dev <
>>>>>> cfe-dev at lists.llvm.org> wrote:
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Wed, Feb 10, 2016 at 2:11 PM, Justin Bogner via llvm-dev <
>>>>>>> llvm-dev at lists.llvm.org> wrote:
>>>>>>>
>>>>>>>> David Blaikie via cfe-dev <cfe-dev at lists.llvm.org> writes:
>>>>>>>> > Recently had a bit of a digression in a review thread related to
>>>>>>>> some tests
>>>>>>>> > going in to compiler-rt (
>>>>>>>> >
>>>>>>>> http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20160208/330759.html
>>>>>>>> > ) and there seems to be some disconnect at least between my
>>>>>>>> expectations
>>>>>>>> > and reality. So I figured I'd have a bit of a discussion out here
>>>>>>>> on the
>>>>>>>> > dev lists where there's a bit more visibility.
>>>>>>>> >
>>>>>>>> > My basic expectation is that the lit tests in any LLVM project
>>>>>>>> except the
>>>>>>>> > test-suite are targeted tests intended to test only the
>>>>>>>> functionality in
>>>>>>>> > the project. This seems like a pretty well accepted doctrine
>>>>>>>> across most
>>>>>>>> > LLVM projects - most visibly in Clang, where we make a concerted
>>>>>>>> effort not
>>>>>>>> > to have tests that execute LLVM optimizations, etc.
>>>>>>>> >
>>>>>>>> > There are exceptions/middle ground to this - DIBuilder is in
>>>>>>>> LLVM, but
>>>>>>>> > essentially tested in Clang rather than writing LLVM unit tests.
>>>>>>>> It's
>>>>>>>> > somewhat unavoidable that any of the IR building code (IRBuilder,
>>>>>>>> > DIBuilder, IR asm printing, etc) is 'tested' incidentally in
>>>>>>>> Clang in
>>>>>>>> > process of testing Clang's IR generation. But these are seen as
>>>>>>>> incidental,
>>>>>>>> > not intentionally trying to cover LLVM with Clang tests (we don't
>>>>>>>> add a
>>>>>>>> > Clang test if we add a new feature to IRBuilder just to test the
>>>>>>>> IRBuilder).
>>>>>>>> >
>>>>>>>> > Another case with some middle ground are things like linker tests
>>>>>>>> and
>>>>>>>> > objdump, dwarfdump, etc - in theory to isolate the test we would
>>>>>>>> checkin
>>>>>>>> > binaries (or the textual object representation lld had for a
>>>>>>>> while, etc) to
>>>>>>>> > test those tools. Some tests instead checkin assembly and
>>>>>>>> assemble it with
>>>>>>>> > llvm-mc. Again, not to cover llvm-mc, but on the assumption that
>>>>>>>> llvm-mc is
>>>>>>>> > tested, and just using it as a tool to make tests easier to
>>>>>>>> maintain.
>>>>>>>> >
>>>>>>>> > So I was surprised to find that the compiler-rt lit tests seem to
>>>>>>>> diverge
>>>>>>>> > from this philosophy & contain more intentional end-to-end tests
>>>>>>>> (eg:
>>>>>>>> > adding a test there when making a fix to Clang to add a counter
>>>>>>>> to a
>>>>>>>> > function that was otherwise missing a counter - I'd expect that
>>>>>>>> to be
>>>>>>>> > tested in Clang and that there would already be coverage in
>>>>>>>> compiler-rt for
>>>>>>>> > "if a function has a counter, does compiler-rt do the right thing
>>>>>>>> with that
>>>>>>>> > counter" (testing whatever code in compiler-rt needs to be
>>>>>>>> tested)).
>>>>>>>> >
>>>>>>>> > Am I off base here? Are compiler-rt's tests fundamentally
>>>>>>>> different to the
>>>>>>>> > rest of the LLVM project? Why? Should they continue to be?
>>>>>>>>
>>>>>>>> I think there's a bit of grey area in terms testing the runtime -
>>>>>>>> generally it's pretty hard to use the runtime without a fairly
>>>>>>>> end-to-end test, so tests of the runtime often end up looking pretty
>>>>>>>> close to an end-to-end test.
>>>>>>>>
>>>>>>>> That said, I don't think that should be used as an excuse to sneak
>>>>>>>> arbitrary end-to-end tests into compiler-rt. We should absolutely
>>>>>>>> write
>>>>>>>> tests in clang and llvm that we're inputting what we expect to the
>>>>>>>> runtime and try to keep the tests in compiler-rt as focused on just
>>>>>>>> exercising the runtime code as possible.
>>>>>>>>
>>>>>>>
>>>>>>> Yes, we should not use compiler-rt tests as an excuse of not adding
>>>>>>> clang/LLVM test. The latter should always be added if possible -- they are
>>>>>>> platform independent and is the first level of defense.  runtime test's
>>>>>>> focus is also more on the runtime lib itself and interaction between
>>>>>>>  runtime, compiler, binutils and other tools.
>>>>>>>
>>>>>>> David
>>>>>>>
>>>>>>>>
>>>>>>>> IIUC, the correct place for integration tests in general is
>>>>>>>> somewhere
>>>>>>>> like test-suite, but I think it's a bit intimidating to some people
>>>>>>>> to
>>>>>>>> add new tests there (Are there docs on this?). I suspect some of the
>>>>>>>> profiling related tests in compiler-rt are doing a bit much and
>>>>>>>> should
>>>>>>>> graduate to a spot in the test-suite (but I don't have time to
>>>>>>>> volunteer
>>>>>>>> to do the work, unfortunately).
>>>>>>>> _______________________________________________
>>>>>>>> LLVM Developers mailing list
>>>>>>>> llvm-dev at lists.llvm.org
>>>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> cfe-dev mailing list
>>>>>>> cfe-dev at lists.llvm.org
>>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Alexey Samsonov
>>>>>> vonosmas at gmail.com
>>>>>>
>>>>>> _______________________________________________
>>>>>> cfe-dev mailing list
>>>>>> cfe-dev at lists.llvm.org
>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>> --
>>>> Alexey Samsonov
>>>> vonosmas at gmail.com
>>>>
>>>
>>>
>>> _______________________________________________
>>> cfe-dev mailing list
>>> cfe-dev at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>>
>>>
>>
>


-- 
Alexey Samsonov
vonosmas at gmail.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20160226/9b3fe458/attachment.html>