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: > > merrymeerkat wrote: > > > 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. > > Hi @ymandel! Apologies for the late reply. > > > > To clarify, were you suggesting that I remove the support for union and add > > a null check at the site of crash, or keep the support and add the null > > check? I had assumed the former, but now I re-read your comment and I am > > not sure. > > > > I thought more about this and I think I'd actually prefer to add support > > for unions and skip the null check. The support could be useful for users. > > Now, as for why I think we should skip the null check: in l. 226 of > > `Analysis/FlowSensitive/DataflowEnvironment.cpp`, we check for methods that > > are not static. These are the prerequisites for something having a "this" > > pointer*. So, since we initialise the `ThisPointeeLoc` there, any > > intializer we're processing in > > `TypeErasedDataflowAnalysis::builtinTransferInitializer` should already > > have a not-null "this" pointer, making the check unnecessary. What do you > > think? > > > > *lambdas could also have a this pointer, but I will leave them to be the > > subject of another revision (I've added a FIXME for that). And, in any > > case, lambdas wouldn't be passed to `builtinTransferInitializer`, so the > > old crash is avoided anyway. > > > Sounds good! I had meant the former. But, I think you make a compelling case > for adding the support and skipping the null check. > > > And, in any case, lambdas wouldn't be passed to builtinTransferInitializer, > > so the old crash is avoided anyway. > Why is this? I'm not sure of the structure of lambda's constructors offhand, > so I don't have a sense one way or another. Thanks for the review! > Why is this? I'm not sure of the structure of lambda's constructors offhand, > so I don't have a sense one way or another. ``` `builtinTransferInitializer` takes in a `CFGInitializer`, i.e., the base/member initializer in a constructor initialization list. For example, in ``` struct S { int t; S(int i):t(i) {} }; ``` `t(i)` corresponds to a CFGInitializer. Since (afaik) lambdas don't have these constructors, the input to `builtinTransferInitializer` shouldn't come from a lambda. 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
