enable full use of -verify in 3C

[kyleheadley]

Clang (cc1) has a nice -verify option that uses comments in the input files to check that diagnostics (errors, warnings, etc) have occurred as intended. Clang's tools interface allows use of verify as well, but 3C only has it set up for the rewrite phase, and few regression tests make use of it. We should make it available to the full program in preparation for constructing better user interfaces.

To get it to work, we at least need to enable the option in tooling (like here), produce diagnostics (like here), and exit from 3C with success. The diagnostics and lack of output become the failure conditions, rather than an assert crash or stderr message with exit.

The main work for this issue will be to restructure these error cases. After each phase (most likely), 3C will need to check if it is in an error state and exit, the diagnostics having run previously when the error was noticed. Asserts that were intended to end the program with an error message need to be refactored, and we need an alternate way to end the program immediately for any of these that might lead to program corruption. Mike has suggested exceptions as a quick alternative.

Additionally, tests which currently grep for diagnostic messages need to be updated to use -verify.

[mattmccutchen-cci]

I'd like to provide a little more information:

3C only has it set up for the rewrite phase, and few regression tests make use of it. We should make it available to the full program in preparation for constructing better user interfaces.

I know exactly what needs to be done to enable the verifier for additional phases: just decide on a different "check prefix" (as in FileCheck) for each phase, change newFrontendActionFactoryA to accept a check prefix rather than just a boolean, and change each phase to call newFrontendActionFactoryA with the correct check prefix. I'd prefer to wait to do this until we actually have diagnostics in other phases that we want to test, rather than adding unused code.

To get it to work, we at least need to enable the option in tooling (like here), produce diagnostics (like here), and exit from 3C with success. The diagnostics and lack of output become the failure conditions, rather than an assert crash or stderr message with exit.

The main work for this issue will be to restructure these error cases. After each phase (most likely), 3C will need to check if it is in an error state and exit, the diagnostics having run previously when the error was noticed.

The way this currently works is that each phase invokes a Clang "tool" in the LibTooling framework (example) that returns an exit code. If at least one error diagnostic was reported during execution of the Clang tool, then the "tool exit code" will be nonzero. And if the tool exit code is nonzero, our code passes the failure up to the main function of the 3c tool, which itself exits nonzero. The diagnostic verifier hooks into the process as follows: it captures the diagnostics that our code originally reports, processes the check comments, and then issues its own error diagnostic if the verification failed. (I'm not sure exactly when this happens -- it might be once per translation unit or once for the whole tool run -- but this detail has not mattered to us so far.) Thus, a verification error diagnostic causes 3c to exit nonzero via the same code path as an ordinary error diagnostic when the verifier is not enabled. That is, we already have the "checking" code you mention.

This doesn't work when 3c exits immediately after reporting a diagnostic (as it currently does for declaration merging failures), rather than allowing the diagnostic verifier to run and the tool execution to return with an exit code so our main function can act accordingly. As you said, it would probably work to throw an exception to break out of the tricky parts of our program analysis, provided that we catch the exception before the point at which the diagnostic verifier is called.

Another case in which we currently can't use the diagnostic verifier is when a problem is detected outside of a tool execution, such as during command line parsing. At that point, we don't have access to a diagnostic engine at all (AFAIK; maybe there is some tricky way to do it). Clang itself has some "non-tool" errors (the one that comes to my mind is "cannot specify -o when generating multiple output files"), so we could look to its regression tests for precedent in how to test non-tool errors. For that particular error message, I found only one regression test, and it works by running FileCheck on the stderr, just as our root_cause test did before I converted it to the diagnostic verifier. I think that (or a plain grep if FileCheck has no practical advantage over it) is probably a reasonable approach for us too, at least for now.

Are there any non-tool errors that we wanted to regression-test but didn't because we didn't know how? If so, we could add those tests as part of this issue. Other than that, I think the only work for us to consider at this time is somehow changing the declaration merging diagnostics so that they work with the verifier, and that is essentially the same thing that is already tracked by #283. Over time, I expect we'll develop a better understanding of general techniques to safely break out of the program analysis when different kinds of serious problems are detected, but for now I suggest we focus on the declaration merging case and not try to speculate about other kinds of scenarios.

[kyleheadley]

This issue is made more difficult by #488. Without the clang parser running every 3c phase, the diagnostic verifier does not get access to the verification comments in code. We could try parsing out the comments ourselves, but this may still be difficult since clang diagnostics were not meant for multiple passes or input outside the compiler. For the near future, an alternative is recommented.

[kyleheadley]

@mattmccutchen-cci following #598 is there any reason left to keep this issue open? Should we move any additional features to new issues?

[mattmccutchen-cci]

I agree, there is nothing left to track in this issue.