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

merrymeerkat added a comment. Thanks for clarifying! I've gone ahead and landed the change. At least this should reduce the number of false negatives we get (hopefully without introducing too much complexity). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGd862f66221de: [clang][dataflow] Treat unions as structs. (authored by merrymeerkat). Changed prior to commit:

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

ymandel added a comment. In D140696#4020209 , @merrymeerkat wrote: > @ymandel - thank you for pointing all of this out! And no need to apologise > at all :) > > I agree that the changes made here are not ideal. But the alternative is also > not ideal.

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

merrymeerkat added a comment. @ymandel - thank you for pointing all of this out! And no need to apologise at all :) I agree that the changes made here are not ideal. But the alternative is also not ideal. The question becomes: is it better to model unions in a way that is lacking, or to not

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

merrymeerkat updated this revision to Diff 485693. merrymeerkat added a comment. Extend documentation comment on storage locations with a FIXME about unions. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D140696/new/

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

ymandel added a comment. In 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 >

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

gribozavr2 added a comment. > 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

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

ymandel added a comment. In D140696#4019589 , @merrymeerkat wrote: >> another thought: please verify that `createStorageLoction` and `createValue` >> can correctly handle union types. Otherwise, you'll likely end up with other >> (surprising) crashes

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

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. >

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

merrymeerkat updated this revision to Diff 485626. merrymeerkat added a comment. Resolving review comment. Add createValue functionality for unions. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D140696/new/ https://reviews.llvm.org/D140696 Files:

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

ymandel 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.

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

ymandel added a comment. > Since (afaik) lambdas don't have these constructors, the input to > builtinTransferInitializer shouldn't come from a lambda. I thought they might because they need to store captured variables. But, godbolt doesn't show any custom constructors so seems they don't. I

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

merrymeerkat added inline comments. Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:1551-1552 +const Value *FooVal = Env.getValue(*FooLoc); +// TODO: Initialise values inside unions, then change below to +// ASSERT_TRUE. +

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

ymandel accepted this revision. ymandel added inline comments. This revision is now accepted and ready to land. Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:1551-1552 +const Value *FooVal = Env.getValue(*FooLoc); +// TODO: Initialise values

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

merrymeerkat added inline comments. Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:1551-1552 +const Value *FooVal = Env.getValue(*FooLoc); +// TODO: Initialise values inside unions, then change below to +// ASSERT_TRUE. +

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

merrymeerkat updated this revision to Diff 485485. merrymeerkat added a comment. Add FIXME Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D140696/new/ https://reviews.llvm.org/D140696 Files:

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

merrymeerkat added inline comments. Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:1551-1552 +const Value *FooVal = Env.getValue(*FooLoc); +// TODO: Initialise values inside unions, then change below to +// ASSERT_TRUE. +

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

ymandel added inline comments. Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:1551-1552 +const Value *FooVal = Env.getValue(*FooLoc); +// TODO: Initialise values inside unions, then change below to +// ASSERT_TRUE. +

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

merrymeerkat added inline comments. Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:1551-1552 +const Value *FooVal = Env.getValue(*FooLoc); +// TODO: Initialise values inside unions, then change below to +// ASSERT_TRUE. +

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

merrymeerkat updated this revision to Diff 485407. merrymeerkat added a comment. Change TODO to FIXME Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D140696/new/ https://reviews.llvm.org/D140696 Files:

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

ymandel added inline comments. Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:1551-1552 +const Value *FooVal = Env.getValue(*FooLoc); +// TODO: Initialise values inside unions, then change below to +// ASSERT_TRUE. +

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

merrymeerkat added inline comments. Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:1551-1552 +const Value *FooVal = Env.getValue(*FooLoc); +// TODO: Initialise values inside unions, then change below to +// ASSERT_TRUE. +

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

ymandel added inline comments. Comment at: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp:1551-1552 +const Value *FooVal = Env.getValue(*FooLoc); +// TODO: Initialise values inside unions, then change below to +// ASSERT_TRUE. +

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

merrymeerkat created this revision. Herald added subscribers: martong, xazax.hun. Herald added a reviewer: NoQ. Herald added a project: All. merrymeerkat requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. This is a straightfoward way to