ymandel added a comment. In D140696#4019810 <https://reviews.llvm.org/D140696#4019810>, @gribozavr2 wrote:
>> With this change, unions are modeled exactly as structs (as far as I can >> tell), which is unsound. > > Modeling just the storage of the union (but not the value) is sound. The > first revision of the patch was a targeted fix that allowed us to continue > maintaining the invariant that `this` is always modeled with a non-null > `StorageLocation`. > > About modeling values of unions as structs (and members of unions as struct > members), I can think of some unsoundness but nothing that affects what we > are modeling now (e.g., pointers to two struct members should compare > unequal, but pointers to two union members should compare equal). If the code > writes to one union member, but reads from the other, code has UB. So it > generally shouldn't be a problem, for the purpose of modeling, to model more > storage in the union that a program free of union-related UB should never > use. WDYT? Good point about UB! So, we can drop the soundness concern. That leaves the "this is a big change" concern. I'm ok with it, but I'd prefer it wait for a thorough test (running over a large variety of TUs). That said, you two can decide how best to proceed -- I don't want to block your progress. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D140696/new/ https://reviews.llvm.org/D140696 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
