Fix the diagnostic verifier, plus a few other fixes.

[mattmccutchen-cci]

Fixing the diagnostic verifier was hard for all the reasons Kyle previously reported, but not impossible!

I'm submitting a draft PR now so the team knows this is coming and to satisfy anyone's curiosity about the basic ideas of the implementation. I know the change needs a lot of cleanup before it is ready for review, and I'll likely end up breaking it into more than one PR. Please do not get up in arms yet. I removed my attempted fix for the root-cause warnings from this PR and will submit it in a separate PR later; #609 tracks follow-up on that. I think everything else is reasonable to go into a single PR.

Fixes #503.
Fixes #515.
Fixes #536.

See the individual commit messages for more details. We can decide during code review what would be appropriate to put in the squash commit message.

[kyleheadley]

Good to know the assertion error on exit didn't take much to get past. I had tried multiple strategies in a limited time period and that was what broke my most promising one. Your code is cleaner than my attempts and I don't think there will be much trouble getting it accepted.

[kyleheadley]

What's left to upgrade this from draft status?

[mattmccutchen-cci]

What's left to upgrade this from draft status?

I still want to improve a bunch of the comments, and yesterday, I studied the code some more and realized I could make significant simplifications. I have another revision in progress and hope to push it in the next few hours. In the meantime, I'm going ahead and answering the comments you posted so far. Thanks for your interest!

[mattmccutchen-cci]

The revision of the code I just pushed is much closer to what I want, though I still have some cleanup to go...

[mattmccutchen-cci]

I think this PR is finally ready. Kyle, I assume you're going to be the one to review it. If anything, there may be too many comments now; I decided to go ahead and write them, but I'll be happy to remove any you feel are unnecessary. Also let me know if you'd like regression tests of particular unusual paths through the _3CInterface logic. (I didn't go ahead and write them because they can be a pain, but I'll be happy to write them on request. I did run the benchmark workflow on a slightly older revision of this PR, and it looked like 3C still ran successfully on each benchmark; in particular, Icecast still worked using the new workaround for #515 in place of the old one.)

[mattmccutchen-cci]

Good to know the assertion error on exit didn't take much to get past. I had tried multiple strategies in a limited time period and that was what broke my most promising one.

Having researched the history a bit more, I'm curious what strategy you tried that ran into this assertion, although it's not important that we take the time to discuss it. From a667ace, it looks like the assertion was originally intended to ensure that the DiagnosticConsumer "below" the VerifyDiagnosticConsumer had been freed, but at some point it stopped serving the intended purpose (or maybe it never served it in the first place) and just started tripping up any code that frees a VerifyDiagnosticConsumer while the DiagnosticsEngine still owns a "higher" DiagnosticConsumer like mine.

Plain old clang -Xclang -verify doesn't hit the assertion because it leaves the VerifyDiagnosticConsumer alone until it is destructed as part of destruction of the DiagnosticsEngine. Here's the DiagnosticsEngine destructor:

checkedc-clang/clang/lib/Basic/Diagnostic.cpp

Lines 91 to 95 in e549c91

	DiagnosticsEngine::~DiagnosticsEngine() {
	// If we own the diagnostic client, destroy it first so that it can access the
	// engine from its destructor.
	setClient(nullptr);
	}

That setClient(nullptr) call ends up doing Owner.reset(nullptr), and std::unique_ptr::reset nulls out the Owner (which previously pointed to the VerifyDiagnosticConsumer) and then calls the VerifyDiagnosticConsumer destructor, where the assertion sees that the Owner is null and passes. AFAICT, if std::unique_ptr::reset took those two steps in the reverse order, the assertion would fail. Apparently the ordering of the steps is actually guaranteed, but IMO it's crazy for code to rely on something this obscure. This was probably a case of someone refactoring a large codebase, seeing that the tests still pass, and calling it a day even though the code no longer made sense. (We may well have similar things in the 3C codebase.) IMO, the assertion should just be deleted, but I don't think I want to take the time to pursue that with upstream Clang.

In the original version of this PR, I thought I had to free the VerifyDiagnosticConsumer in order to force any diagnostic verification errors to be sent to its underlying DiagnosticConsumer, but it appears that the EndSourceFile callback is enough: that's the same thing that plain clang uses. So the current version of the PR leaves the VerifyDiagnosticConsumer alone the same way that plain clang does and no longer needs to explicitly work around the assertion.

(Side remark: Sometime for fun, you can try compiling and linking a program with an "expected" error, e.g., clang -Xclang -verify -o foo foo.c. The compile step doesn't create foo.o because of the error, but it exits successfully because diagnostic verification succeeded, so the driver proceeds to the link step, which fails because foo.o is missing. I guess nobody cared about making that sequence work properly because compile diagnostics can be tested without linking.)

[kyleheadley]

I'm curious what strategy you tried that ran into this assertion

I've no doubt it was a crude version of what you did here, but without time to research the ownership model (and it's convoluted use in the Clang diagnostics, to what? save a few lines of code? Keep to the old model of everything internalized?). If it was triggered by not passing the EndSource, that's what I did, probably because I didn't have your state machine that closes on error.

[kyleheadley]

This looks good.

The comments are fine because it was a complex fix and a hack that you were able to get to look simpler than it is. The code is clean enough that if there is a bug for an unusual pass through the interface, it shouldn't be too hard to manage. Congrats on avoiding Clang asserts by carefully doing things in the right order.

I know little about file canonicalization, and it seems overly complex to me. But this isn't the issue for that and what you have is at least reasonably situated and comment delineated.

I agree with your documentation suggestions: a note in 3c -help only to say that 3c augments the verifier, and only where we'd mention -- options; a paragraph in the testing sections of any usage guides we maintain, with explanation of how per-TU verification might not be intuitive at first.

Thanks for getting this to work. I didn't like finding out that my update #488 disabled it, and almost didn't merge because of that. Then after a few quick hacks failed I didn't know if I'd ever get approval to spend the time needed to re-enable it. An if I had my code wouldn't have been as refined as yours.

[mattmccutchen-cci]

Thanks for the review and the kind words!

I know little about file canonicalization, and it seems overly complex to me.

It's complex but, unfortunately, necessary to avoid/mitigate the issues that I cited (#515 and #529/#604) (at least without making other changes to the 3C PersistentSourceLoc code that are too risky for this PR). If you'd prefer, we can find someone else to review this part of the PR. Or if you're interested in learning more yourself, hopefully those issues contain all the information you need.

I agree with your documentation suggestions: a note in 3c -help only to say that 3c augments the verifier, and only where we'd mention -- options; a paragraph in the testing sections of any usage guides we maintain, with explanation of how per-TU verification might not be intuitive at first.

My proposal was actually to not add anything to 3c -help and describe the behavior only in our testing documentation in CONTRIBUTING.md. In my opinion, the distraction to normal users of showing the information unconditionally in 3c -help is a greater evil than the risk that a user will be confused by the behavior of this specialized internal testing option and will be unable to find our explanation in CONTRIBUTING.md. I don't agree that we have a categorical obligation for 3c -help to describe any changes that 3C makes to the behavior of existing compiler options, regardless of their relevance to normal users. (As I said, if we could make the information display only in 3c -help-hidden, I think that would be a good compromise, but unfortunately it doesn't look like the LLVM command line parsing library supports that.) But if you feel strongly about having the information in 3c -help and the rest of the team doesn't care, I'm willing to add the note to 3c -help rather than continue debating the issue.

Anyway, I will go ahead and add the information to CONTRIBUTING.md. I guess I meant to do that earlier and I forgot.

[kyleheadley]

Nevermind the 3c -help then, my mistake.

Did you make significant changes to the canonicalization? It looked like you moved it into the new init section and tweaked some things. I didn't mind glossing over it because you have been active with that part of the code for a while now and I trust that you think any changes are important and will keep adjusting if you notice something else. The rest of the code I expect to be stable right away. I only mentioned it because I keep seeing it in reviews, but that's probably coincidence and the fact that it's an active project of yours.

[mattmccutchen-cci]

Did you make significant changes to the canonicalization? It looked like you moved it into the new init section and tweaked some things.

That's about right. The code for -I paths is new to the 3c tool (it replaces code that was previously in convert_project), but as you probably saw, it's similar to the code that the 3c tool already had for the main source file. I don't think there are any behavior changes in the cases we know are important, though there might be behavior changes in unusual cases.

I didn't mind glossing over it because you have been active with that part of the code for a while now and I trust that you think any changes are important and will keep adjusting if you notice something else.

Yes, that's my plan.

Anyway: I've now added the documentation to CONTRIBUTING.md. Will you please review it at your convenience?

[kyleheadley]

LGTM