llvm dev - Jul 2017 - Swallowing of input in FileCheck

Hi,

Debugging tests which make use of FileCheck can be a frustrating experience, as
all input will be swallowed (even with -v flag passed to lit),
and one would often need to copy-and-paste and rerun the failing command
manually without piping into FileCheck.
Initially I’ve assumed that this is done due to stream processing, but looking
at FileCheck source code I can see that
it actually gets the entire input into RAM anyway:

```
  do {
    Buffer.reserve(Buffer.size() + ChunkSize);
    ReadBytes = read(FD, Buffer.end(), ChunkSize);
    if (ReadBytes == -1) {
      if (errno == EINTR) continue;
      return std::error_code(errno, std::generic_category());
    }
    Buffer.set_size(Buffer.size() + ReadBytes);
  } while (ReadBytes != 0);
```

Thus, I propose modifying FileCheck default behavior to dump all swallowed
output on stderr when the test has failed.
Would there be any objections to such a change?
I understand the concern that log files might become unnecessarily large, but
since it would only be done for failed
test I think the added readability would be worth it.

Regards,
George

On Fri, Jul 7, 2017 at 1:20 PM, George Karpenkov via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
>
> Thus, I propose modifying FileCheck default behavior to dump all swallowed
> output on stderr when the test has failed.
> Would there be any objections to such a change?
>
Yes.

> I understand the concern that log files might become unnecessarily large,
> but since it would only be done for failed
> test I think the added readability would be worth it.
>
I disagree, it would be too much output. During development, it's pretty
common to cause tens of tests to fail. I don't really want 10 entire
assembly files dumped into my console during incremental development. Our
test output is already long, and I wish it were shorter.

I agree that this is a real problem when remote buildbots in different
configurations get involved. Locally debugging FileCheck failures is easy,
you just copy-paste the command like you said and pipe it to less. It's
only a pain when you aren't sure if a failure on a bot will reproduce
locally. So, I would be in favor of an option to lit that we enable on
buildslaves that dumps the output. We already have a '\bFileCheck\b'
substitution in lit. We'd just expand it to 'FileCheck
--dump-on-failure'
or something on bots.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170707/02606c8a/attachment.html>

> On Jul 7, 2017, at 2:19 PM, Reid Kleckner <rnk at google.com> wrote:
> 
> On Fri, Jul 7, 2017 at 1:20 PM, George Karpenkov via llvm-dev <llvm-dev
at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote:
> Thus, I propose modifying FileCheck default behavior to dump all swallowed
output on stderr when the test has failed.
> Would there be any objections to such a change?
> 
> Yes.
>  
> I understand the concern that log files might become unnecessarily large,
but since it would only be done for failed
> test I think the added readability would be worth it.
> 
> I disagree, it would be too much output. During development, it's
pretty common to cause tens of tests to fail. I don't really want 10 entire
assembly files dumped into my console during incremental development. Our test
output is already long, and I wish it were shorter.
Could this be solved by having lit be intelligent about showing less output when
there are large numbers of test failures (w/o other output), and truncating very
large outputs?

I do think there are situations where having the output just show up by default
locally could prevent needing to rerun a command, which is handy.
> I agree that this is a real problem when remote buildbots in different
configurations get involved. Locally debugging FileCheck failures is easy, you
just copy-paste the command like you said and pipe it to less. It's only a
pain when you aren't sure if a failure on a bot will reproduce locally. So,
I would be in favor of an option to lit that we enable on buildslaves that dumps
the output. We already have a '\bFileCheck\b' substitution in lit.
We'd just expand it to 'FileCheck --dump-on-failure' or something on
bots.
This sounds reasonable to me, no matter what on the above question.

 - Daniel

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170707/84d7266c/attachment.html>