Add -output-dir and other file handling improvements

[mattmccutchen-cci]

Draft squash commit message

- Add -output-dir option to write updated files to a directory structure
  parallel to the base dir (#347). When -output-dir is used, a source
  file outside the base dir can't be handled because there is no way to
  compute its output path. For consistency, this is now an error even
  when -output-dir is not used.

- Convert all 3C regression tests from -output-postfix to -output-dir to
  avoid leaving temporary files in the clang/test/3C directory (#378).

- Expand "3c -help" documentation. In particular, direct the user to pass
  "--" when they don't want to use a compilation database to avoid
  accidentally using unwanted compiler options and suppress the warning
  if no compilation database is found (#343).

- For consistency, have stdout mode output the main file even if it is
  unchanged (#328).

- Fix bugs in matching of file paths against the base dir (#327).

- Other minor bug fixes: see the pull request description for details.

List of changes and regression testing status

• Fixes Various bugs in base dir matching #327 (.., symlink, foo.c is not under foo/): all have dedicated tests
• -output-dir (fixes 3C needs a better way to output multiple files. #347): Assume this is sufficiently exercised by many of the other tests.
• -allow-sources-outside-base-dir: Won’t bother with a test, following the precedent of -allow-unwritable-changes.
• New/revised internal functions getCanonicalFilePath, filePathStartsWith, etc.: Assume these are tested as much as we care via the higher-level features they implement.
• Fixes Better behavior with unchanged files #328 remaining parts:
  • Multiple source files in stdout mode is now an exit 1 instead of an assertion failure: Not worth testing?
  • For consistency, stdout mode outputs the file even if it is unchanged: sufficiently exercised by other tests
• -output-dir + -output-postfix is an error: Not worth testing? Agreed
• Error when a source file is outside the base directory: Not worth testing?
• -base-dir with trailing slash + -output-dir gets canonicalized properly and doesn’t cause files to be written at a wrong path: update 2021-02-11: has a dedicated test!
• Nonzero exit when failing to write an output file: Not worth testing?
• Send all debug output to errs() instead of outs() to avoid corrupting stdout mode: Not worth testing? Agreed
• Revised 3c -help text, most notably with explanation of -- (fixes Interaction of error and -- param produces different results in output of 3c #343) (test N/A)
• Test suite stuff
  • Migrate from -output-postfix to -output-dir (fixes 3C output is not cleaned up when a test fails. #378); required adding -base-dir in a bunch of places
  • Pass -base-dir in many more commands since input files are now required to be under the base dir and the base dir defaults to lit's working directory, which is not clang/test/3C
  • Replace count 0 with diff now that stdout mode outputs the file even if it is unchanged
  • test_updater: Removed tests from the list that were deleted in Remove obsolete tests that were incorrectly resurrected #364; mark some tests as no longer producing whitespace diffs
  • All calls to 3c now use --
  • Remove -fcheckedc-extension flag from 3c calls because it got in my way (not from %clang; that can be done later in Clean up test RUN commands #346)
  • Add rm -rf %t* to all tests that mention %t. This is overkill for tests that only use %t.unused and the like, but I didn’t think it was worth the trouble to exclude those tests. This boilerplate is unpleasant but is expected to go away eventually with Refactor test RUN commands into a separate tool #355. I looked into solutions via the lit config and didn’t think it was promising. See the LLVM bug report.
  • Change the test for an unimplemented canWrite constraints case to use a crazy #include instead of a typedef, in anticipation of Fix for issue #373 #408.
  • typedef, typedefbounds: Enable the idempotence tests because they pass and having lines that looked like commands but weren't real RUN lines was confusing my scripts.

TODO

• Migrate all existing regression tests from -output-postfix to -output-dir (fixes 3C output is not cleaned up when a test fails. #378)
  • testgenerator
  • processor
  • test_updater
  • Unmanaged tests
  • Delete temporary files before starting the test
  • Managed tests:
    • Revert undesired modifications to stuff other than RUN lines
    • Verify that there were no preexisting diffs in RUN lines
  • Review changes to unmanaged tests
• Fix path canonicalization and comparison bugs (fixes Various bugs in base dir matching #327) if it isn't too hard
• Review my own code (done except as noted elsewhere)
• Ensure that all changes have regression tests where appropriate (see notes in list of changes)
• Update to latest main
• Clean up commit sequence to ease code review
• Draft the final squash commit message

[mattmccutchen-cci]

John (or anyone else interested): Please let me know if you think any more of the changes deserve regression tests. I wanted to get your opinion before spending the time to write them.

I replaced the original, messy commit history with a sequence of commits broken down specifically to make this PR easier to review, though I still plan to squash-merge the PR. If anyone wants to see the old history, let me know.

[john-h-kastner]

A few questions about the code, but it generally looks good. I've been over the manually updated tests and don't see anything amiss there.

[kyleheadley]

I took a brief look, and none of the changes stood out as needing tweaks, but I don't know the particulars of unix file system, etc. As far as tests:

* `-output-dir` (fixes #347): Assume this is sufficiently exercised by many of the other tests.

It looks like you added tests (or commands within the tests) to run with this option. If they don't exercise the new functionality, then a new test is warranted, but otherwise I don't know what the assumption is here

* `-output-dir` + `-output-postfix` is an error: Not worth testing?

* Send all debug output to `errs()` instead of `outs()` to avoid corrupting stdout mode: Not worth testing?

It may be useful to have a single general-purpose test for each of these. Definitely don't add them as run commands in existing tests, but a special-case regression test for each of them would be useful for potential future regressions, since you've identified an intended behavior

[mattmccutchen-cci]

I took a brief look, and none of the changes stood out as needing tweaks, but I don't know the particulars of unix file system, etc.

Great!

As far as tests:

* `-output-dir` (fixes #347): Assume this is sufficiently exercised by many of the other tests.

It looks like you added tests (or commands within the tests) to run with this option. If they don't exercise the new functionality, then a new test is warranted, but otherwise I don't know what the assumption is here

Yes, I migrated a large number of existing tests to use -output-dir, so the basic functionality is covered. There may be some special cases that aren't covered by those tests, and I think what I was asking was whether those special cases merit dedicated regression tests; I think the answer is "probably not".

* `-output-dir` + `-output-postfix` is an error: Not worth testing?

* Send all debug output to `errs()` instead of `outs()` to avoid corrupting stdout mode: Not worth testing?

It may be useful to have a single general-purpose test for each of these. Definitely don't add them as run commands in existing tests, but a special-case regression test for each of them would be useful for potential future regressions, since you've identified an intended behavior

Re -output-dir + -output-postfix: I could easily add a test for this, but is it worth writing and maintaining a test for a behavior that seems so unlikely to break (one line of code that isn't dependent on anything else to speak of)? Would we test all the other command line validation too? Update: We decided at today's meeting that it isn't worth writing a test.

Re outs(): If there are outs() statements that trigger only under certain conditions, I don't see how it would be feasible to completely guard against that via regression tests. Granted, the four outs() statements we had before this PR all triggered on essentially every run with the -verbose option, which suggests that one test that the -verbose option doesn't corrupt stdout might have value. Still, I'm not sure whether it's worth maintaining a test given that inappropriate use of outs() is easy to spot in code review if we remember it (I can add it to the 3C code style guidelines). Update: Mike pointed out that testing this is a low priority because (1) serious users will not be using stdout mode and (2) the worst that is likely to happen is that a user of stdout mode gets uncompilable output, files an issue with us, and switches to -output-postfix or -output-dir as a workaround.

[kyleheadley]

Re -output-dir + -output-postfix: I could easily add a test for this, but is it worth writing and maintaining a test for a behavior that seems so unlikely to break (one line of code that isn't dependent on anything else to speak of)? Would we test all the other command line validation too?

Re outs(): If there are outs() statements that trigger only under certain conditions, I don't see how it would be feasible to completely guard against that via regression tests. Granted, the four outs() statements we had before this PR all triggered on essentially every run with the -verbose option, which suggests that one test that the -verbose option doesn't corrupt stdout might have value. Still, I'm not sure whether it's worth maintaining a test given that inappropriate use of outs() is easy to spot in code review if we remember it (I can add it to the 3C code style guidelines).

I see, you were asking for explanations for why these issues might regress, rather than votes. If -output-dir + -output-postfix produces an error when attempted, that doesn't need a regression test. If it's a user error to make the attempt, than we need something more. On reflection the latter isn't even a regression test situation, so I must have misunderstood this request.

For outs() we should have a regression test. If we could catch anything in code review, we wouldn't need regression tests, they're there because some issues are easy to avoid automatically before reaching code review. But if 3c's output was being corrupted, then we already have lots of tests for that. I assumed you meant something more subtle.

[john-h-kastner]

Looks good.