Convert root_cause test to use Clang's built-in diagnostic verifier. #365

mattmccutchen-cci · 2020-12-31T19:23:42Z

This means we now verify the line numbers of the diagnostics for free, which would have been a pain with the existing CHECK-DAG system.

Make the 3c tool accept a -verify option analogous to that of clang -cc1 to verify diagnostics against // expected-warning comments. Currently, this applies only to the rewriting phase.
Fix a few mistakes in the expected diagnostics in the root_cause test.

Issues for special attention in code review:

Diagnostic verification failure diagnostics (like any other diagnostics) currently do not cause the 3c tool to exit nonzero. Should we change that? If not, how should the test check for a diagnostic verification failure? The PR currently does this a hacky way in order to demonstrate the rest of the design.
In updating the expected diagnostics in the root_cause test: Two were obviously backwards and I corrected them. The "Bad pointer type solution" diagnostic was not specifically expected but was included in the count of 11 warnings expected by the old test. Should I expect this specific diagnostic or do something else?

john-h-kastner · 2021-01-04T15:54:52Z

Looks good. The extra diagnostic should be there.

As for the diagnostic verification failure, to me it makes sense for these to cause 3C to exit nonzero. I think this would be as simple as having _3CInterface::writeAllConvertedFilesToDisk return false if the Tool.run call return a nonzero value. This would cause main in 3CStandalone.cpp to return 1.

mattmccutchen-cci · 2021-01-04T20:32:17Z

As for the diagnostic verification failure, to me it makes sense for these to cause 3C to exit nonzero. I think this would be as simple as having _3CInterface::writeAllConvertedFilesToDisk return false if the Tool.run call return a nonzero value. This would cause main in 3CStandalone.cpp to return 1.

Good idea. This worked (I have not yet updated the PR); it exposed a problem in another test, but I worked around it. However, is it OK for now to leave the writeAllConvertedFilesToDisk return value behavior inconsistent with buildInitialConstraints, solveConstraints, and writeConvertedFileToDisk (which I think is used by clangd3c and not by 3c)? Or should I try to change all of the methods now?

john-h-kastner · 2021-01-04T21:03:27Z

I lean towards making the change everywhere. I suppose that could cause it to fail early on some inputs where it was previously able to continue running after the error. If that doesn't happen on the regression tests (or we can easily fix errors that arise) I say we make the change.

mattmccutchen-cci · 2021-01-05T18:32:03Z

@jackastner I made the change. As stated in the commit message, I'm not sure it makes complete sense, but I think it's harmless for now. The tests still pass. Good?

john-h-kastner

Yeah, looks good. I agree with the concern you raise in the commit message. We'll potentially have to deal with that when we update the clangd tool.

fails. It's unclear whether (1) there are still more operations in these methods that can fail and would merit an early return and (2) these early returns could cause problems for callers such as clangd3c that want to recover. But for now, adding these early returns is better than the status quo, and the only caller we support is the 3c tool, which just exits when one of these methods returns false. This exposed a problem with the partial_checked_arr test: it ran 3c on a lit temporary file named partial_checked_arr.c.tmp, and that file extension seems to cause 3c to spew errors. Previously 3c nevertheless exited 0 and the test carried on; now 3c would exit 1. Work around this by using -output-postfix like many of our other tests. This may not be the approach we want in the long term, but we don't have the new approach ready yet.

This means we now verify the line numbers of the diagnostics for free, which would have been a pain with the existing CHECK-DAG system. - Make the 3c tool accept a -verify option analogous to that of `clang -cc1` to verify diagnostics against `// expected-warning` comments. Currently, this applies only to the rewriting phase. - Fix a few mistakes in the expected diagnostics in the root_cause test.

kyleheadley · 2021-01-05T19:11:26Z

clang/test/3C/partial_checked_arr.c

-// RUN: 3c -alltypes %s > %t
-// RUN: 3c -alltypes %t | count 0
+// RUN: 3c -alltypes -output-postfix=checked %s
+// RUN: 3c -alltypes %S/partial_checked_arr.checked.c -- | count 0
+// RUN: rm %S/partial_checked_arr.checked.c


This is a regression to when we didn't know about %t temp files. Which admittedly may have been so soon that the changes overlapped. If there's no reason for the regressions, lets leave the simpler code that doesn't create extra files if it ends prematurely.

%t expanded to a filename ending in .c.tmp. For some reason, 3c doesn't like that as an input file name and spewed a bunch of errors. For now, I just changed this test to do the same thing as many of our other tests. When we have a proper solution to this problem, we can update all the tests.

I'm sorry for not giving you more time to review this.

It didn't give me any errors when I committed it. Is that because of tests on a different system, or related to the changes made in this pull request?

Before this pull request, when I ran the 3c command from the RUN line manually, I saw the errors, but 3c still exited 0, so the test passed, and lit doesn't seem to show the stderr of passing tests. With the changes in this pull request, 3c exited 1, so I had to do something about the test.

This seems significant to me, maybe for more issues? One for .tmp and one for passing with errors. If 3c ever allows tests to pass that have errors in them, that's bad for everyone. Did this pull request fix all of that? The OP was concerned with a subset and then you fixed more.

I guess for now we need to remember not to pass lit temp files to 3c. I'll see if there's a good place to fit that in the temp files issue I made earlier to document future changes.

This seems significant to me, maybe for more issues? One for .tmp and one for passing with errors.

Agreed!

If 3c ever allows tests to pass that have errors in them, that's bad for everyone. Did this pull request fix all of that? The OP was concerned with a subset and then you fixed more.

I'm not sure, and I don't really want to spend more time researching it right now. Someone else is probably more familiar than I am with all the types of errors that can occur in 3c.

I guess for now we need to remember not to pass lit temp files to 3c. I'll see if there's a good place to fit that in the temp files issue I made earlier to document future changes.

Good. If we pass a temp file, the test will fail, and we just need to learn quickly that it is because of the temp file and not waste time debugging again.

mattmccutchen-cci requested a review from john-h-kastner December 31, 2020 19:25

mattmccutchen-cci mentioned this pull request Dec 31, 2020

Clean up test RUN commands #346

Open

13 tasks

john-h-kastner approved these changes Jan 5, 2021

View reviewed changes

mattmccutchen-cci added 2 commits January 5, 2021 14:04

mattmccutchen-cci force-pushed the root-cause-test-line-numbers branch from dc85bfb to cf652f5 Compare January 5, 2021 19:09

mattmccutchen-cci merged commit d6670af into main Jan 5, 2021

mattmccutchen-cci deleted the root-cause-test-line-numbers branch January 5, 2021 19:11

kyleheadley reviewed Jan 5, 2021

View reviewed changes

mattmccutchen-cci mentioned this pull request Jan 6, 2021

Ensure that 3c tool exits nonzero in all error cases in which it should #370

Open

mattmccutchen-cci mentioned this pull request Jan 28, 2021

Constrain the most common constructs in non-writable files to not change. #391

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Convert root_cause test to use Clang's built-in diagnostic verifier. #365

Convert root_cause test to use Clang's built-in diagnostic verifier. #365

Uh oh!

mattmccutchen-cci commented Dec 31, 2020 •

edited

Loading

Uh oh!

john-h-kastner commented Jan 4, 2021 •

edited

Loading

Uh oh!

mattmccutchen-cci commented Jan 4, 2021

Uh oh!

john-h-kastner commented Jan 4, 2021

Uh oh!

mattmccutchen-cci commented Jan 5, 2021

Uh oh!

john-h-kastner left a comment

Uh oh!

kyleheadley Jan 5, 2021

Uh oh!

mattmccutchen-cci Jan 5, 2021

Uh oh!

kyleheadley Jan 5, 2021

Uh oh!

mattmccutchen-cci Jan 5, 2021

Uh oh!

kyleheadley Jan 5, 2021

Uh oh!

mattmccutchen-cci Jan 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Convert root_cause test to use Clang's built-in diagnostic verifier. #365

Convert root_cause test to use Clang's built-in diagnostic verifier. #365

Uh oh!

Conversation

mattmccutchen-cci commented Dec 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

john-h-kastner commented Jan 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattmccutchen-cci commented Jan 4, 2021

Uh oh!

john-h-kastner commented Jan 4, 2021

Uh oh!

mattmccutchen-cci commented Jan 5, 2021

Uh oh!

john-h-kastner left a comment

Choose a reason for hiding this comment

Uh oh!

kyleheadley Jan 5, 2021

Choose a reason for hiding this comment

Uh oh!

mattmccutchen-cci Jan 5, 2021

Choose a reason for hiding this comment

Uh oh!

kyleheadley Jan 5, 2021

Choose a reason for hiding this comment

Uh oh!

mattmccutchen-cci Jan 5, 2021

Choose a reason for hiding this comment

Uh oh!

kyleheadley Jan 5, 2021

Choose a reason for hiding this comment

Uh oh!

mattmccutchen-cci Jan 5, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mattmccutchen-cci commented Dec 31, 2020 •

edited

Loading

john-h-kastner commented Jan 4, 2021 •

edited

Loading