This is an archive of the discontinued LLVM Phabricator instance.

Handles failing driver tests of clang
ClosedPublic

Authored by Purva-Chaudhari on May 18 2022, 11:10 PM.

Details

Summary

Added support for incremental mode 8 and 28 ie. frontend::EmitBC: and frontend::PrintPreprocessedInput:
Added supporting clang tests to test in clang-repl mode

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMay 18 2022, 11:10 PM
Purva-Chaudhari requested review of this revision.May 18 2022, 11:10 PM
v.g.vassilev added a subscriber: rsmith.

@Purva-Chaudhari, this looks reasonable to me. I am adding more reviewers to discuss the way we refer other tests from clang-repl.

clang/test/Interpreter/clangtests.cpp
1

@rsmith, would it be acceptable to have a test that refers other tests from the repo in that manner?

dblaikie added inline comments.May 29 2022, 10:12 AM
clang/test/Interpreter/clangtests.cpp
1

Generally that's not done - and the inputs should be moved into an "Inputs" subdirectory and shared from there. Tests that are in different subdirectories - not sure if there's a good way to share those, maybe with an "Inputs" directory in a parent directory of both tests? We might not have precednt for that

dblaikie added inline comments.May 29 2022, 10:14 AM
clang/test/Interpreter/clangtests.cpp
1

But more broadly, could you explain what the goal of these tests are? Generally I would discourage what I think of as "shotgun" testing - taking some existing comprehensive test for a particular feature and using it to test a mostly orthogonal feature. The orthogonal feature should have more targeted tests/it shouldn't be using such broad testing in the regression suite here.

v.g.vassilev added inline comments.May 29 2022, 11:48 AM
clang/test/Interpreter/clangtests.cpp
1

My take is that clang-repl is basically clang that takes inputs incrementally. Being that, means that we should be in a position to process whatever clang processes and thus we run against all of the existing tests. We planned to add the ones which we did not support as regression tests.

We can add more targeted tests but they would be copies or simplifications of already existing ones. Hence there is my hesitation - reuse or duplication...

dblaikie added inline comments.May 29 2022, 1:59 PM
clang/test/Interpreter/clangtests.cpp
1

My take is that clang-repl is basically clang that takes inputs incrementally. Being that, means that we should be in a position to process whatever clang processes and thus we run against all of the existing tests.

Yeah, that's sort of the "proof by absurdity" - we wouldn't want every clang test running in both ahead of time and incremental mode in the usual "check-clang" regression suite (I wouldn't mind having a separate mode for testing - more of an integration test that some buildbots or those working on more comprehensive clang-repl support could run, but most people/especially fast bots would not). So then the question for me is which tests should we have running all the time in "check-clang" - and my general answer is: Situations that have motivated code changes/support in clang-repl: If no code was added/changed/etc to clang-repl, then no test should be added to "check-clang" for that test case.

If that "run everything under check-clang run under clang-repl to find missing functionality" found some clang test that didn't work with clang-repl, yeah, I'd generally be in favour of not reusing an input or duplicating in its entirety - but reducing the test case to test only the specific clang-repl functionality issue, and testing that in particular.

Like we shouldn't test every feature of static-assert with clang-repl and clang in every "clang-check" if most of those features aren't distinctly interesting in both cases. Just enough in clang-repl to test what makes static-assert interesting in clang-repl.

v.g.vassilev added inline comments.Jun 25 2022, 1:11 PM
clang/test/Interpreter/clangtests.cpp
1

@dblaikie, that makes sense to me.

@Purva-Chaudhari, can you address it by having just something like
// RUN: clang-repl -Xcc -E to exercise the -E and likewise for the EmitBC case?

This revision is now accepted and ready to land.Jul 22 2022, 7:08 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 2 2022, 12:01 AM