[PATCH] D78850: Memory corruption issure for DenseStringElementsAttr

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: mlir/lib/IR/AttributeDetail.h:614
+    // Otherwise, this is a splat.
+    return KeyTy(ty, data, hashVal, /*isSplat=*/true);
   }
----------------
mehdi_amini wrote:
> mehdi_amini wrote:
> > I don't understand the fix, can you elaborate? Seems like the ArrayRef is passed by value to the KeyTy so I don't get the lifetime issue?
> We were creating an ArrayRef pointing to the local variable firstElt before, but since it is a reference it should be fine. So there is likely a copy of the StringRef in the implicit conversion to ArrayRef.
> 
> The change here is making us taking the hash of the entire array instead of the first element, I don't think this is necessary, something like this should work as well:
> 
> ```
>     return KeyTy(ty, ArrayRef<StringRef>{&firstElt, 1}, hashVal, /*isSplat=*/true);
> ```
> 
> 
Actually nicer and equivalent I think:
```
return KeyTy(ty, data.take_front(), hashVal, /*isSplat=*/true);
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78850/new/

https://reviews.llvm.org/D78850