[clang] [clang][dataflow] Merge `RecordValue`s with different locations correctly. (PR #65319)

[via cfe-commits]

================
@@ -121,18 +121,18 @@ static Value *mergeDistinctValues(QualType Type, Value &Val1,
 
   Value *MergedVal = nullptr;
   if (auto *RecordVal1 = dyn_cast<RecordValue>(&Val1)) {
-    [[maybe_unused]] auto *RecordVal2 = cast<RecordValue>(&Val2);
-
-    // Values to be merged are always associated with the same location in
-    // `LocToVal`. The location stored in `RecordVal` should therefore also
-    // be the same.
-    assert(&RecordVal1->getLoc() == &RecordVal2->getLoc());
-
-    // `RecordVal1` and `RecordVal2` may have different properties associated
-    // with them. Create a new `RecordValue` without any properties so that we
-    // soundly approximate both values. If a particular analysis needs to merge
-    // properties, it should do so in `DataflowAnalysis::merge()`.
-    MergedVal = &MergedEnv.create<RecordValue>(RecordVal1->getLoc());
+    auto *RecordVal2 = cast<RecordValue>(&Val2);
+
+    if (&RecordVal1->getLoc() == &RecordVal2->getLoc())
+      // `RecordVal1` and `RecordVal2` may have different properties associated
+      // with them. Create a new `RecordValue` without any properties so that we
+      // soundly approximate both values. If a particular analysis needs to
+      // merge properties, it should do so in `DataflowAnalysis::merge()`.
+      MergedVal = &MergedEnv.create<RecordValue>(RecordVal1->getLoc());
+    else
+      // If the locations for the two records are different, need to create a
+      // completely new value.
----------------
martinboehme wrote:

Not sure exactly what you mean...

Whether the locations are the same or not doesn't really depend on the order in which we process CFG blocks. Instead, locations are guaranteed to be the same if the `RecordValue` is stored in `ExprToLoc`, and they are usually (but not always) different if the `RecordValue` is stored in `ExprToVal`.

The order in which we process CFG blocks affects whether we actually see different `RecordValue`s or not when the merge happens -- which makes merges difficult to trigger reliably from an integration test ([example](https://github.com/google/crubit/blob/3ab9d49fde5a59262903c589bb6963b8db4c0541/nullability/test/merge.cc#L342)). But I'm not sure how relevant that is to the situation here?

Anyway, my hope is that all of this will go away relatively soon because we're now working on eliminating the `RecordStorageLocation` from `RecordValue`.

https://github.com/llvm/llvm-project/pull/65319