[analyzer] Add std::variant checker

[spaits]

As my BSc thesis I've implemented a checker for std::variant and std::any, and in the following weeks I'll upload a revised version of them here.

Prelude

@Szelethus and I sent out an email with our initial plans here: https://discourse.llvm.org/t/analyzer-new-checker-for-std-any-as-a-bsc-thesis/65613/2 We also created a stub checker patch here: https://reviews.llvm.org/D142354.

Upon the recommendation of @haoNoQ , we explored an option where instead of writing a checker, we tried to improve on how the analyzer natively inlined the methods of std::variant and std::any. Our attempt is in this patch https://reviews.llvm.org/D145069, but in a nutshell, this is what happened: The analyzer was able to model much of what happened inside those classes, but our false positive suppression machinery erroneously suppressed it. After months of trying, we could not find a satisfying enhancement on the heuristic without introducing an allowlist/denylist of which functions to not suppress.

As a result (and partly on the encouragement of @Xazax-hun) I wrote a dedicated checker!

The advantage of the checker is that it is not dependent on the standard's implementation and won't put warnings in the standard library definitions. Also without the checker it would be difficult to create nice user-friendly warnings and NoteTags -- as per the standard's specification, the analysis is sinked by an exception, which we don't model well now.

Design ideas

The working of the checker is straightforward: We find the creation of an std::variant instance, store the type of the variable we want to store in it, then save this type for the instance. When retrieving type from the instance we check what type we want to retrieve as, and compare it to the actual type. If the two don't march we emit an error.

Distinguishing variants by instance (e.g. MemRegion *) is not the most optimal way. Other checkers, like MallocChecker uses a symbol-to-trait map instead of region-to-trait. The upside of using symbols (which would be the value of a variant, not the variant itself itself) is that the analyzer would take care of modeling copies, moves, invalidation, etc, out of the box. The problem is that for compound types, the analyzer doesn't create a symbol as a result of a constructor call that is fit for this job. MallocChecker in contrast manipulates simple pointers.

My colleges and I considered the option of making adjustments directly to the memory model of the analyzer, but for the time being decided against it, and go with the bit more cumbersome, but immediately viable option of simply using MemRegions.

Current state and review plan

This patch contains an already working checker that can find and report certain variant/any misuses, but still lands it in alpha. I plan to upload the rest of the checker in later patches.

The full checker is also able to "follow" the symbolic value held by the std::variant and updates the program state whenever we assign the value stored in the variant. I have also built a library that is meant to model union-like types similar to variant, hence some functions being a bit more multipurpose then is immediately needed.

I also intend to publish my std::any checker in a later commit.

[NagyDonat]

Very promising checker with straightforward and clean logic! I have lots of comments, but they are all minor/cosmetic issues.

[steakhal]

This is my first round of review. So far it looks good.
I didn't have much time, but that's all I had.

I think it would make sense to have tests for variants containing variants and playing with the active members. That would be interesting to see.

[spaits]

Thank you for your time and effort for editing my commit @Szelethus and for reviewing my commit @donatnagye @steakhal .

I have edited my commit based on your comments. These are the thing I have added or changed:

• Added a function to decide 'a' or 'an' should be used in the reports.
• Added test cases to verify that the
  an std::variant holds a std::variant is handled well variantHoldingVariant,
  a multiple active type changes are handled well typeChangeThreeTimes, assignmentOperator.
• Replaced std::string to StringRef wherever I found that possible
• Removed the IIFEs that made code readability worse.
• Removed asserts and llvm_unreachable where it was used improperly to "test" the correctness of input.
• Used llvm coding conventions (hopefully everywhere)
• Using dyn_cast_or_null instead of checking for nullptr and then casting.
• Spell checking and correcting sentences that were not clear or misguiding.

[NagyDonat]

Yet another round of review, with even less significant remarks :)

[spaits]

@steakhal what do you think about the current version of this commit?

[steakhal]

@donatnagye I can see you all have a lot of PRs. I have limited time, but I think I can do two this week. Maybe one next week.
In what order should I go through the PRs?
I guess, this one should be in the lucky two.

[NagyDonat]

@donatnagye I can see you all have a lot of PRs. I have limited time, but I think I can do two this week. Maybe one next week. In what order should I go through the PRs? I guess, this one should be in the lucky two.

The most urgent ones are #66207 (a "real" review) and #66647 (where nothing significant changed since you reviewed it last time). I hope that you can do those and this one.

Moreover, I'm planning to merge the commit #67572 (soon, probably without your review) because it's just an NFC cleanup and @gamesh411 already reviewed it.

[spaits]

@steakhal we have also run the checker on multiple open source projects (a few weeks ago) and it did not crash. (It did not have any findings. There were only two C++ 17 projects and they had retrieved value from std::variant at most 20 times).

Could you please take another look at the commit and give feedback, when you'll have time for it?

[steakhal]

I had a quick look once again.
This this time I focused on the library boundaries and APIs.

[spaits]

@steakhal thank you for the review. I have made the changes you have asked for.

• Changed the name of header guard macro to reflect the path of the file
• Used the new namespace opening syntax
• Changed a function's argument, so it takes reference instead of pointer
  • It may would be a good task for me to see why the checkRegionChanges callback uses pointer instead of reference.
• Moved a few functions into the class CallEvent
  • There are other functions I did not move into CallEvent because they are called on a CallEvent, cast it to a subclass then tell things about it. It would not be nice to have a function like this in the CallEvent super class. But if you think I should move them there then I can do that.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[spaits]

gentle ping @steakhal

[spaits]

@steakhal I talked with @donatnagye and @dkrupp . We came to the conclusion, that the checker is only an alpha checker, would also not mean breaking changes to the analyzer and would not bring false positives. It also has been reviewed by @donatnagye . I would like to merge it in this form and later iterate on it. Would it be okay for you?

[steakhal]

Yes, why not.

EDIT: I didn't mean to be passive-aggressive. I'm really busy these days. Sorry I could not review this.