This is an archive of the discontinued LLVM Phabricator instance.

[llvm-lit] Keep start/end part of the output when truncating
AcceptedPublic

Authored by a.elovikov on Dec 7 2022, 4:34 PM.

Details

Diff Detail

Event Timeline

a.elovikov created this revision.Dec 7 2022, 4:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 7 2022, 4:34 PM
Herald added a subscriber: delcypher. · View Herald Transcript
a.elovikov requested review of this revision.Dec 7 2022, 4:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 7 2022, 4:34 PM
jdenny accepted this revision.Dec 9 2022, 7:13 AM

Seems straight-forward to me.

Even so, sometimes a concrete use case can help clarify context when researching changes later. Is there something like a buildbot failure you can easily link from the commit log? If not, no worries. The rationale seems clear.

Thanks for the improvement.

This revision is now accepted and ready to land.Dec 9 2022, 7:13 AM

My use-case is downstream where I use FileCheck against stderr like this:

// a.cpp:
// RUN: DEBUG_OUR_RUNTIME=1 %t.out > %t.log 2>&1
// RUN: FileCheck %s < %t.log
//
// ...
// assert(Something);

Debug output is bigger than the threshold (although I only FileCheck a tiny bit of it) so without this change I don't see the failing assert when running llvm-lit. With the patch assert message is visible in the llvm-lit's output.

That comment seems like sufficient documentation. Thanks.

I forgot to ask earlier: is there no test case for this feature?

I forgot to ask earlier: is there no test case for this feature?

check-all (for cmake -GNinja ../llvm -DCMAKE_BUILD_TYPE=Release) is clean so it seems the feature didn't add test test when originally introduced.

Do you have a reference to how the simple test should look like? I've checked a couple of tests in utils/lit/tests but they look like an overkill for this patch.

Do you have a reference to how the simple test should look like? I've checked a couple of tests in utils/lit/tests but they look like an overkill for this patch.

You could add a new case to llvm/utils/lit/tests/shtest-output-printing.py. That means another example test file in llvm/utils/lit/tests/Inputs/shtest-output-printing/.

You could have the example test file's RUN line dump >1024 bytes. That's about 13 lines in an 80-column window, so it wouldn't be terrible to hardcode the text in an echo command.

The check lines could look for the first few bytes of the output, the "...", and the very last few bytes. It doesn't seem worthwhile to verify the start and end are actually 512 bytes.

This seems straight-forward, but let me know if I overlooked a complexity.

I have troubles making it work... I'm adding truncate.txt next to Inputs/shtest-output-printing/basic.txt with content like this:

# Verify that truncated output preserves both begin/end. The command needs to
# fail.
# RUN: not echo "Start             other output                             \n\
# RUN: ---------------------------------------------------------------------\n\
# RUN: ---------------------------------------------------------------------\n\
# RUN: ---------------------------------------------------------------------\n\
# RUN: ---------------------------------------------------------------------\n\
# RUN: ---------------------------------------------------------------------\n\
# RUN: ---------------------------------------------------------------------\n\
# RUN: ---------------------------------------------------------------------\n\
# RUN: ---------------------------------------------------------------------\n\
# RUN: ---------------------------------------------------------------------\n\
# RUN: ---------------------------------------------------------------------\n\
# RUN: ---------------------------------------------------------------------\n\
# RUN: ---------------------------------------------------------------------\n\
# RUN: ---------------------------------------------------------------------\n\
# RUN: ---------------------------------------------------------------------\n\
# RUN: ---------------------------------------------------------------------\n\
# RUN: ------------------------------------------------------End" >%t.out 2>&1

but then the output in build/utils/lit/tests/Output/shtest-output-printing.py.tmp.out is (long lines truncated with "$" symbol)

******************** TEST 'shtest-output-printing :: truncate.txt' FAILED ********************
Script:
--
: 'RUN: at line 3';   not echo "Start             other output                             \n ---------------------------------------------------------------------\n -------------$
--
Exit Code: 127

Command Output (stdout):
--
$ ":" "RUN: at line 3"
$ "echo" "Start             other output                             \n ---------------------------------------------------------------------\n -----------------------------------$
# command stderr:
stdin and stderr redirects not supported for echo
error: command failed with exit status: 127
I think it might be due to "stdin and stderr redirects not supported for echo" but not sure...
  1. RUN: ------------------------------------------------------End" >%t.out 2>&1

I think you can just drop the 2>&1.

stdin and stderr redirects not supported for echo

LIT's builtin echo shoudn't write to stderr.

My first version was without "2>&1", still doesn't show the content of the redirected output for some reason.

jdenny added a comment.Dec 9 2022, 1:12 PM

Sorry, I didn't realize that builtins don't trigger this code. (Feels like a bug... to investigate some other time.)

Instead of echo, you can write a small python script. For example, you could copy llvm/utils/lit/tests/Inputs/shtest-shell/write-to-stderr.py to llvm/utils/lit/tests/Inputs/shtest-output-printing/ and change it to write large output. This time you can use a loop instead of hard-coded text.

See llvm/utils/lit/tests/Inputs/shtest-shell/redirects.txt for how to call the python script. See llvm/utils/lit/tests/Inputs/shtest-shell/lit.cfg for how to define the python substitution.