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

Wed Dec 28 05:12:46 PST 2022

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