Merge from 3C

[mattmccutchen-cci]

Hi! As I trust @mwhicks1 has previously discussed with you, here is a pull request of Correct Computation's entire master branch with improvements to 3C.

As you can see, many files have been both renamed and heavily edited due to correctcomputation#299. To make this PR easier to review, here are links to two diffs, which make up this PR when applied in sequence:

• File renames.
• All other changes. GitHub is not very good at displaying this, so you may prefer to run:

  git remote add -f correctcomputation https://github.com/correctcomputation/checkedc-clang
  git diff correctcomputation/file-renames-for-microsoft-202010 9494e6ab9448c61517f08b85dbbe36b87f1d454d
  

  You can use the --stat option to git diff to help find changed non-3C-specific files.

Please disregard the old review comments from our repository that are showing up on this thread; we don't know why that is happening and haven't found a way to stop it.

[mattmccutchen-cci]

Looks great, thank you for the contributions again! I have a few minor suggestions for improvements for the future. @mgrang , could you merge this?

Great! If you can hang on an hour so until after my other meeting, I'll slip a fix for the 5C thing into this PR. I also want to merge your master into this PR once more and re-run our tests; a problem is unlikely, but just to be safe.

[mattmccutchen-cci]

I merged from your master and our tests pass. Ready to go!

[mgrang]

This is breaking our Windows x64 builds with the error:
D:\work\13\s\clang\lib\3C\RewriteUtils.cpp : fatal error C1128: number of sections exceeded object file format limit: compile with /bigobj

It seems we had a fix for this on master which seems to have been removed in this commit. I have a fix to add it back: #951. I will merge the fix once I validate it.

[mattmccutchen-cci]

It seems we had a fix for this on master which seems to have been removed in this commit.

Sorry about that. I'm researching how it happened; it looks like it was in a period before I joined CCI when our git history was a mess.

But outright mistakes may not be the only things we do that break your Windows build in the future. Is there a way you could test our future PRs before merging them so we don't have to try to set up an environment that can reproduce the problems?

[mgrang]

Matt, several 3C unit tests are failing on the X86 Window build.

Failing Tests (9):
    Clang :: 3C/3d-allocation.c
    Clang :: 3C/basic.c
    Clang :: 3C/definedType.c
    Clang :: 3C/extstructfields.c
    Clang :: 3C/graphs.c
    Clang :: 3C/graphs2.c
    Clang :: 3C/linkedlist.c
    Clang :: 3C/multivardecls.c
    Clang :: 3C/root_cause.c

[mgrang]

See the full failure list: Unit Test Failures.txt

[mattmccutchen-cci]

Matt, several 3C unit tests are failing on the X86 Window build.

If the set of tests that fail is consistent, can you try marking them UNSUPPORTED: windows (see documentation) until we have time to come up with a real fix? I'd send you a PR, but it's probably faster if you are the one to iterate on getting the syntax for this right.

Next time, whatever tests you plan to run on your master after merging, can you please run them on our PR branch before merging or give us a way to do so? (I have been ensuring that the PR branch is up to date with your master so the content of the merge will equal the PR branch; you can also check that yourself.) We can't drop everything for a rush fix to a bunch of unforeseeable failures on your master branch. There was some discussion of us setting up a Windows test environment, but I don't know if we can ever be certain that it will exactly match yours. This time, if the problems are bad enough, it's fine with me if you revert the whole thing and we send another PR once we have the fix.

[mgrang]

I have a PR to disable the failing unit tests: #952
I have also filed an issue to track this: correctcomputation#345

[mattmccutchen-cci]

It seems we had a fix for [the /bigobj issue] on master which seems to have been removed in this commit.

Sorry about that. I'm researching how it happened; it looks like it was in a period before I joined CCI when our git history was a mess.

My conclusions, finally:

I confirmed that the reversion of both fixes from #917 (/bigobj and #include <cctype>) and a third erroneous change we discovered in our own work all occurred in our merge in 4c4eed3, and the root cause of all three errors was the fact that #891 was squash-merged. In this merge, the left side included the source commit of #891 (740d758) and not the squash merge of #891 (3f68113), while the opposite was true of the right side.

If #891 had been merged with a normal merge commit, then Git would have been able to follow the edge from the merge commit to the source commit, the merge base for 4c4eed3 would have been 7fc9292, and the automated textual merge in 4c4eed3 would have done the right thing in all three cases. But since Git was unaware of the relationship between the squash merge and the source commit, it went all the way back to 999bba6 and 2531895 as merge bases. Each change between (999bba6 / 2531895) and 7fc9292 appeared in its original form on the left side of 4c4eed3 and as part of the squash of #891 on the right side of 4c4eed3. I'll call such a change a "duplicated" change.

In the /bigobj and #include <cctype> cases, we had made a duplicated change. The left side of 4c4eed3 had the original change, while the right side had the other copy of the change plus the subsequent changes in #917. This generated a conflict, and our manual resolution of this conflict was incorrect. In the third case, the sequence of events was different: we made a duplicated change and then reverted it after submitting #891, so the left side saw no net change while the right side saw the squashed copy of the forward change. The automated textual merge silently took the forward change, which was not what we intended.

These three errors spooked me enough that I conducted an investigation to try to find any more latent errors all at once rather than having them cause problems for us one at a time. Specifically, I used a custom tool to analyze every merge we ever performed, checking whether the outcome might have been different if all the 3C PRs from CCI's repository to Microsoft's had been merged normally instead of squash-merged. The tool identified several merges that were potentially incorrect and proposed patches that could have corrected them at the time they were originally performed. In a few cases, it was clear to me that a patch addressed an error that still exists and the patch should be applied now (#962 and correctcomputation#364). But a quick look at the rest of the patches suggested they were probably no longer relevant: we may have later rewritten the affected code, scrutinized it thoroughly enough that we would have corrected any problems we could conceivably care about, or even noticed and corrected the specific merge error in a later commit (the tool did not check for this). I didn't think it was worth spending the effort to research them more thoroughly on the off chance that there might be something we might want.

To prevent future problems of this nature, Microsoft's agreement to perform normal rather than squash merges of future omnibus 3C PRs (contingent on us keeping the commit sequences of those PRs reasonably clean) should be sufficient. (Since squash merging still seems to be the habit, e.g., for #956, I'll be sure to remind you multiple times!) I also plan to keep an eye on things for the next few months, and if your team does squash-merge an omnibus 3C PR by mistake, there is a step I can take to prevent it from causing later merging mistakes (as I did for #930 in b23b287).