[PATCH] D114718: [analyzer] Implement a new checker for Strict Aliasing Rule.

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

I haven't checked the code, but only the tests and expectations for scalar values.
I'll dive deeper if we settled on those.

A couple of remarks:

1. I would rather use top-level function parameters of the given type instead of declaring an instance of that type and taking the address of that object. I think it would reflect more a real-world scenario when we don't see the allocation of the object, thus we don't know the dynamic/effective type of that object.
2. Using an alias or the underlying type does not matter in this context; You should not duplicate the tests for this reason. A single test demonstrating that the checker is able to look through type aliases (by using the canonical type) is enough.
3. Marking signed an already signed type won't introduce new coverage. unlike for `char` you can be sure that `short`, `int`, `long`, `long long` are `signed` by default.
4. Constness does not matter in this context, since type similarity effectively removes any CVR anyway. I would rather remove those test cases, and leave a single one demonstrating that we thought about them and it works the same way as without `const`.
5. Please, also test when you have two pointers with a different number of indirections: `long *` and `long **`; `gcc` expects `*p` and `*q` not aliasing.
6. Consider adding all the test cases from Detecting Strict Aliasing Violations in the Wild <https://www.trust-in-soft.com/wp-content/uploads/2017/01/vmcai.pdf> paper by Pascal Cuoq et al.

They cover a lot of really interesting cases. BTW in the paper, they already set up the godbolt links demonstrating their findings: https://godbolt.org/g/ggZzQo

I think if we are done with these, we can move on to the more complicated cases involving standard layout types with/without inheritance, unions with common prefixes, and other interesting cases like function pointers, pointers to members, pointers to arrays.

---

The test expectations for `(int, Class)`, `(int, ClassInt)`, `(int, UnionUchar)`, `(int, UnionInt)`, `(int, std::byte)`, `(int, std::OtherByte)`, `(int, EnumInt)`, `(int, char)`, `(int, unsigned char)`, `(int, signed char)`, `(int, short)`, `(int, unsigned short)`, `(int, int)`, `(int, volatile unsigned int)`, `(int, long)`, `(int, unsigned long)`, `(int, long long)`, `(int, unsigned long long)`, `(int, float)`, `(int, double)`, `(int, long double)`, `(int, unsigned)`, look right.

I'll continue the review when I can spare some time for this.

I would like to see this as an `alpha.core` checker displayed as two checkers: `StrictAliasingModeling` and `StrictAliasingChecker`. You could implement this as a single checker, similarly to how the `CStringChecker` does for example. Do the reporting only if the `StrictAliasingChecker` is enabled, but emit the sink node unconditionally, restricting the exploration space.
To get this checker into the `core` package we will need a couple of checker options, relaxing the strict-aliasing rules for //unions// and for example the //signed char// as well; since it seems like both `gcc` and `clang` respects aliasing involving them via extensions.

Lastly, I'd like to thank you for working on this. I'm really excited about covering these types of issues.



================
Comment at: clang/test/Analysis/Checkers/StrictAliasingChecker/strict-aliasing.cpp:2
+// RUN: %clang_cc1 -std=c++20 -analyze -analyzer-config eagerly-assume=false -analyzer-checker=debug.ExprInspection,alpha.core.StrictAliasing -verify %s
+// NOTE: -relaxed-aliasing flag disables StrictAliasing checker.
+
----------------
I think `fno-strict-aliasing` should achieve this, by which the driver should transform that flag into the `-relaxed-aliasing` flag consumed by the backend.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114718/new/

https://reviews.llvm.org/D114718