Merge from 3C

[secure-sw-dev-bot]

This issue was copied from checkedc/checkedc-clang#930

---

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/checkedc-clang#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.

[secure-sw-dev-bot]

Comment from @microsoft-cla[bot]:

All CLA requirements met.

[secure-sw-dev-bot]

Comment from @mattmccutchen-cci:

This is ready for review. Enjoy!

[secure-sw-dev-bot]

Comment from @mgrang:

LLVM follows an 80 char line limit. See https://llvm.org/docs/CodingStandards.html#source-code-width

Please make sure lines in this PR follow the 80 char line limit. Also please ensure all comments are terminated by a period.

[secure-sw-dev-bot]

Comment from @mattmccutchen-cci:

I'm working on addressing the long lines in all 3C-specific files (not only the ones modified in this PR). [Update 2020-11-12: This is done.]

Reviewers, do you have any other comments in the meantime?

[secure-sw-dev-bot]

Comment from @mattmccutchen-cci:

Hey, I noticed that it looks like someone on this repository (@kkjeer?) is working on upgrading to LLVM 11. What are your plans for this? When will it land on your master? (Or is this already documented somewhere that I didn't see?) It looks like you had to make some changes to 3C files, so there are likely to be some semantic (if not syntactic) conflicts with this PR (I haven't looked carefully); I'll be happy to help resolve them when the time comes.

It looks like you squashed the entire diff between your current master and its baseline LLVM revision into a single commit on the LLVM 11 branch. Compared to a normal merge, this will make it much more work to trace the history of pieces of code across the upgrade. Did you see offsetting benefits of the squash, or problems with the merge? If you'd be open to doing a merge instead but could use help with it, I'd be happy to help.

If you proceed with the squash, have you thought about how it will interact with this PR? I assume that if LLVM 11 lands on master first, we'll archive the current history of the PR and replace it with a squash, and then any further additions we make to the PR would be on top of that squash. If this PR lands first, then you would apply a squash of it with any necessary syntactic and semantic conflict resolutions (possibly with our help) to the LLVM 11 branch, as you would with anything else you merge to master while the LLVM 11 branch is pending.

[secure-sw-dev-bot]

Comment from @mattmccutchen-cci:

Any more review comments on the PR while I'm working on the naming conventions (which may take another day or two to get everything sorted out)?

[secure-sw-dev-bot]

Comment from @mattmccutchen-cci:

@mgrang FYI, I am still working toward fixing our names in bulk using clang-tidy; it may take me a few more days. I know this is taking longer than we all would like. I'm working only 10 hours per week for CCI and have had to investigate several clang-tidy bugs and had a few other CCI responsibilities to deal with as well. I've added clang-tidy suppression comments for some things we don't want to rename (locally, not yet pushed), so I'm closer to reviewing the remaining renames proposed by clang-tidy to see if they are OK or there are other problems I need to address.

[secure-sw-dev-bot]

Comment from @mgrang:

@mgrang FYI, I am still working toward fixing our names in bulk using clang-tidy; it may take me a few more days. I know this is taking longer than we all would like. I'm working only 10 hours per week for CCI and have had to investigate several clang-tidy bugs and had a few other CCI responsibilities to deal with as well. I've added clang-tidy suppression comments for some things we don't want to rename (locally, not yet pushed), so I'm closer to reviewing the remaining renames proposed by clang-tidy to see if they are OK or there are other problems I need to address.

Adding @kkjeer since Katherine is working on the source upgrade. I guess it is still another week before we are ready to upgrade our sources.

[secure-sw-dev-bot]

Comment from @mgrang:

Thanks for addressing the review comments. I ran our ADO validations on your PR: https://dev.azure.com/msresearch/CheckedC/_build/results?buildId=591077&view=results

I see a couple of warnings while building the lit tests:

/mnt/work/9/s/clang/lib/3C/AVarBoundsInfo.cpp:901:23: warning: unused variable ‘TKVar’ [-Wunused-variable]
           ProgramVar *TKVar = BI->getProgramVar(TK);

/mnt/work/9/s/clang/lib/3C/ArrayBoundsInferenceConsumer.cpp:616:9: warning: unused variable ‘ABoundsInfo’ [-Wunused-variable]
   auto &ABoundsInfo = Info.getABoundsInfo();

[secure-sw-dev-bot]

Comment from @mattmccutchen-cci:

Thanks for addressing the review comments. I ran our ADO validations on your PR: https://dev.azure.com/msresearch/CheckedC/_build/results?buildId=591077&view=results

When I try to view that link, I'm asked to log in to a Microsoft account. Can I use any Microsoft account, do I need some permission, or will you just tell me anything I need to know from there?

I see a couple of warnings while building the lit tests:

/mnt/work/9/s/clang/lib/3C/AVarBoundsInfo.cpp:901:23: warning: unused variable ‘TKVar’ [-Wunused-variable]
           ProgramVar *TKVar = BI->getProgramVar(TK);

/mnt/work/9/s/clang/lib/3C/ArrayBoundsInferenceConsumer.cpp:616:9: warning: unused variable ‘ABoundsInfo’ [-Wunused-variable]
   auto &ABoundsInfo = Info.getABoundsInfo();

Fixed.

[secure-sw-dev-bot]

Comment from @mgrang:

I don't see any other issues with the ADO run. The changes look good to me.

@dtarditi Could you take a final look at the PR? Also do we have a plan for the 3C team to run their own ADO validations for their PRs in future?

[secure-sw-dev-bot]

Comment from @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.

[secure-sw-dev-bot]

Comment from @mattmccutchen-cci:

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

[secure-sw-dev-bot]

Comment from @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: checkedc/checkedc-clang#951. I will merge the fix once I validate it.

[secure-sw-dev-bot]

Comment from @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?

[secure-sw-dev-bot]

Comment from @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

[secure-sw-dev-bot]

Comment from @mgrang:

See the full failure list: Unit Test Failures.txt

[secure-sw-dev-bot]

Comment from @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.

[secure-sw-dev-bot]

Comment from @mgrang:

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

[secure-sw-dev-bot]

Comment from @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/checkedc-clang#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).