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: > > > 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? > Thanks, that's quite helpful! Yes, I think that would be a better fix. It's a > matter of perspective (and opinion) so feel free to push back but I'd say > that the code you pointed to is buggy -- it assumes the `this` loc is > populated, but *by design* it's not populated for unions (I didn't know that > unions could ever have `this` so #TIL?). I think we're admitting that we > have a class of initializers for which we knowingly don't create the this > pointer and therefore should express that in the code. > > Alternatively, you could argue that the complete class of `this` pointer > generating code is structs and unions, so if we just make sure (like your > fix) to generate a this pointer in both cases, we can assert it's > non-nullness (via the cast) and be done. > > Given that the framework in general is riddled with places where we don't > know if a value or storage location or... is initialized and we have to > check, I think that's the better (and consistent) approach. Moreover, given > that we're not actually adding support for unions, just trying to avoid a > crash, I think changing that code site better expresses that intent. > > Still, it's a matter of opinion, so I'll leave it to you to decide. WDYT? Thanks for the comment! What you say makes sense. I guess we introduced the union initialization because it could also be useful in the future, but I don't know if it makes sense to add a feature that doesn't have any semantic use yet. If the framework does these kinds of null checks in lots of other places, then I agree that it'd be good to have it here too for consistency. I'm leaning towards making the change you suggested. 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
