xazax.hun added inline comments.
================ Comment at: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h:706-711 +/// This function is primarily intended for use by checks that set custom +/// properties on `StructValue`s to model the state of these values. Such checks +/// should avoid modifying the properties of an existing `StructValue` because +/// these changes would be visible to other `Environment`s that share the same +/// `StructValue`. Instead, call `refreshStructValue()`, then set the properties +/// on the new `StructValue` that it returns. Typical usage: ---------------- mboehme wrote: > xazax.hun wrote: > > I think this is fine for now, but I wonder if we should come up with an API > > where errors like this cannot happen. One potential way would be to no > > longer include these properties in the `StructValue`s, but have a separate > > mapping `StructValue => Properties`. So, one can update the mapping in an > > environment without any unintended consequences in other environments. > > I think this is fine for now, but I wonder if we should come up with an API > > where errors like this cannot happen. > > Thanks for bringing this up. > > I agree that this needs more improvement in the future. The underlying issue > is that `StructValue` isn't really a very useful concept. > > `Value` works great for scalar values, where we do actually treat it as the > immutable value that it's intended to be. Attaching properties to values > makes a lot of sense in this context: If we're modelling a property of an > immutable value, then that property is presumably itself immutable. > > However, the `Value` concept has never worked well for structs / records > because, when we mutate fields, we don't update the `StructValue`. We could > try to change this, but I don't think this would be worth it because > `StructValue` isn't a useful concept in C++ anyway. As the value categories > RFC discusses, prvalues of class type are a very niche concept in C++. The > only thing you can do with them is to initialize a result object -- you can't > otherwise perform any operations on them (i.e. access member variables or > call member functions). This has been part of the motivation for reducing > their importance as part of the value categories work. > > I think we should continue down that path and, ultimately, maybe eliminate > `StructValue` entirely. So while introducing a mapping `StructValue => > Properties` would be a good option, I think an even better one would be to > start attaching properties to `AggregateStorageLocation`s instead of > `StructValue`s. Because `AggregateStorageLocation`s are in essence, mutable, > the mapping of `AggregateStorageLocation`s to property values would need to > be reflected in the `Environment` in some form. I think a natural way to do > this would be to model properties in a similar way to fields: An > `AggregateStorageLocation` would have a mapping `PropertyName => > StorageLocation`, and the values of the properties would then be tracked > through the existing `StorageLocation => Value` mapping in the environment. > > Benefits: > > - As already noted, this makes properties on `AggregateStorageLocation`s very > similar to fields, which is what they are typically used to model > - If a particular analysis needs a storage location for a property, we > already have one. For example, UncheckedOptionalAccessModel.cpp currently > models the "value" property as a `PointerValue` so that it has a > `StorageLocation` that it can use as the return value of `optional::value()`. > Under the approach I'm proposing, the storage location would be available > from the framework, and the model for a specific analysis wouldn't have to do > anything extra. > - We don't need any additional logic for joins / widening; the existing join > / widening logic for the `StorageLocation => Value` mapping would also handle > properties. > > So this is where I think we can go in the future, and I agree that > `refreshStructValue()` should only be a stopgap as we evolve the framework. This sounds great, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155204/new/ https://reviews.llvm.org/D155204 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
