[PATCH] D140696: [clang][dataflow] Treat unions as structs.

[Dani Ferreira Franco Moura via Phabricator via cfe-commits]

merrymeerkat added a comment.

> another thought: please verify that `createStorageLoction` and `createValue` can correctly handle union types. Otherwise, you'll likely end up with other (surprising) crashes in the system. E.g. 
> https://github.com/llvm/llvm-project/blob/781eabeb40b8e47e3a46b0b927784e63f0aad9ab/clang/lib/Analysis/FlowSensitive/Transfer.cpp#L457
> assumes that the base of a member expression is an `AggregateStorageLocation`, which means that in the case that `this` is such a base and it's a union type, you need to be sure that `createStorageLocation` has allocated an `AggregateStorageLocation`.
>
> Unfortunately, we don't have written down anywhere (aside from the code itself) the invariants we require.

Good point!

Based on the assertions in TransferTest.UnionThisMember, we are creating storage locations for unions. So, the code you linked to should not crash (at least on unions as simple as that in the test).

We weren't creating values yet though. Turns out that was because `createValue` calls `createValueUnlessSelfReferential`, which creates values only for certain types, returning a nullptr otherwise: https://github.com/llvm/llvm-project/blob/8eb3698b940c4064b772f3ff5848d45f28523753/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp#L618-L699 . I've just changed this function's if-statement on structs and classes to accept union types too, and now we are creating values for unions. So, values should hopefully not be a problem either.

There was already another unit test relating to unions (TransferTest.AssignToUnionMember), but it was only partly implemented. Now that we initialize locations and values for union types, I implemented the rest of the test. (thank you @gribozavr2 for the help debugging!)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140696