This is an archive of the discontinued LLVM Phabricator instance.

Adding a scripted test for PR25717
ClosedPublic

Authored by ygao on Dec 21 2015, 5:07 PM.

Details

Reviewers
rafael
Summary

Hi,
I am trying to add a test for PR25717. The test itself is fairly straight-forward: it generates a
large .c temporary file on the fly and then runs the preprocessor over the generated file to
make sure we do not see any unexpected fatal errors.
I have never added a scripted test before, so it would be really nice if someone with this
experience could take a look that I did make all the necessary changes.
Many thanks in advance,

  • Gao

Diff Detail

Event Timeline

ygao updated this revision to Diff 43420.Dec 21 2015, 5:07 PM
ygao retitled this revision from to Adding a scripted test for PR25717.
ygao updated this object.
ygao added inline comments.Dec 21 2015, 5:10 PM
test/Preprocessor/bigoutput.py
7 ↗(On Diff #43420)

For example, it would be nice to use %t as the temporary file name (so lit will fill in with a generated
name) instead of a hard-coded name here, but I am trying to force UNIX line ending here. So I could
not figure out how to combine %t with the open() call.

Can't you use the preprocessor to produce a large output? See the
testcase in https://llvm.org/bugs/show_bug.cgi?id=10651#c10 for
example.

Cheers,
Rafael

ygao added a comment.Dec 23 2015, 11:44 AM

Hi Rafael,
Yes I can do that, and it works for me. Do you know whether there is a way to keep the UNIX line ending on the test file upon checkout rather than having svn or git auto-translate the line endings? The UNIX line ending is necessary to trigger the original bug,

  • Gao
ygao updated this revision to Diff 43554.Dec 23 2015, 12:22 PM

I think what I can do is to run
svn propset svn:eol-style 'LF'
on the new file, and hopefully the subversion or git client will get the hint and not auto-translate the line endings.

Are you sure the line ending in a problem? The preprocessor output has
16 lines, but the last one is very long.

In any case, it looks like at least on linux clang produces an output
with '\n' even when the input has CRLF:

$ file test.c
test.c: ASCII text, with CRLF line terminators
$ clang -E test.c > test.i
$ file test.i
test.i: C source, ASCII text, with very long lines

Cheers,
Rafael

ygao added a comment.Dec 23 2015, 2:43 PM

If you take a look at

void PrintPreprocessedAction::ExecuteAction() {
      ...
      while (next < end) {
      if (*cur == 0x0D) {  // CR
        if (*next == 0x0A)  // CRLF
          BinaryMode = false;
      ...

The value of this BinaryMode reflects whether the line ending style of the input file
is CRLF or LF. And it is passed all the way down to the constructor of raw_fd_ostream,

std::unique_ptr<llvm::raw_pwrite_stream> CompilerInstance::createOutputFile(...) {
    ...
  if (!Binary || OS->supportsSeeking())
    return std::move(OS);

  auto B = llvm::make_unique<llvm::buffer_ostream>(*OS);
  ...

So I think the line ending style of the input file does affect whether the output is buffered.

Gosh, that is quite unfortunate. The problems I see are

  • The code only work on windows, as opening a file as text on other

systems is not special.

  • There is a confusion in the code about binary being "crlf

translation" or "produce a .o". The logic for creating a buffer is
there because the .o writer needs something it can seek on.

What I would suggest is:

  • Check that this test fails at least on windows with your patch

reverted. If so, commit it. BTW, don't you need to drop the "|
FileCheck" to cause a crash?

  • Pass two flags to createDefaultOutputFile: TranslateCrLf and

NeedsSeek instead of just Binary.

  • Stop using F_Text flag and just print the correct line ending so

this works on any system.

The last two can be just a bug report for now :-)

Cheers,
Rafael

ygao added a comment.Jan 4 2016, 5:00 PM

What I would suggest is:

Check that this test fails at least on windows with your patch

reverted. If so, commit it. BTW, don't you need to drop the "|
FileCheck" to cause a crash?

Hmm, even with the pipe removed, I still have difficulty reproducing the crash within lit. I was doing:
C:\llvm\build>python ..\llvm\utils\lit\lit.py -v --no-progress-bar --param build_mode=Release tools\clang\test\Preprocessor\macro.c
But the test is passing. And the run line is simply:
// RUN: %clang_cc1 -E -x c %s

And, if I use the RUN line directly on a command-prompt, the test crashes as expected.

ygao added a comment.Jan 5 2016, 2:39 PM

In llvm/utils/lit/lit/TestRunner.py line#202 inside function executeShCmd(), the test is spawned by
subprocess.Popen(command, ..., stdout = subprocess.PIPE, stderr = subprocess.PIPE, ...)
And the test passes. I can verify with a small python script doing just a subprocess.Popen.

If the test is spawned with stdout = None instead, the test will crash as expected.

I could not find a way to trick TestRunner into leaving stdout as None. Is there supposed to be
a command-line option to lit? Or maybe a special redirection mark on the RUN line? Maybe a lit
expert can help me here.

ygao updated this revision to Diff 44351.Jan 8 2016, 11:45 AM
ygao updated this revision to Diff 45784.Jan 22 2016, 6:05 PM
rafael accepted this revision.Jan 25 2016, 10:12 AM
rafael added a reviewer: rafael.

LGTM assuming you can get the test to fail by reverting the fix

This revision is now accepted and ready to land.Jan 25 2016, 10:12 AM
ygao added a comment.Jan 25 2016, 11:03 AM

Thanks, Rafael!
I did verify that the test would fail without my fix. But what do I need to do to make buildbots turn on the new lit parameter?

ygao closed this revision.Jan 29 2016, 2:17 PM

The test is commited in rL258898 and rL258902, and it is enabled on buildbot in rL259248.
Thanks!