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

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

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.
+        ASSERT_FALSE(isa_and_nonnull<IntegerValue>(FooVal));
----------------
ymandel wrote:
> merrymeerkat wrote:
> > ymandel wrote:
> > > Why push this off to another patch?
> > good point! I was pushing it because this is just a quick fix to avoid a crash, and the current changes are sufficient for that
> SGTM. Please use FIXME instead of TODO, though. But, glad to see this is enough to avoid the crashing!
> 
> That said, can you expand on where the actual crash was happening? I'm concerned that its possible that the crash site should be more robust and that this patch is hiding that weakness.
Done!

The crash was happening because of a null pointer cast in the builtin transfer function for CFGInitializers: https://github.com/llvm/llvm-project/blob/781eabeb40b8e47e3a46b0b927784e63f0aad9ab/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp#L316

Hmm so do you think it'd be helpful to add a null check in the file above? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140696