[PATCH] D68816: [NFC] Replace a linked list in LiveDebugVariables pass with a DenseMap

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando marked an inline comment as done.
Orlando added a subscriber: jmorse.
Orlando added inline comments.


================
Comment at: llvm/lib/CodeGen/LiveDebugVariables.cpp:1141
+  if (auto *UserVals = lookupVirtReg(OldReg))
+    for (auto *UV : *UserVals)
+      for (unsigned i = 0; i != NewRegs.size(); ++i)
----------------
bjope wrote:
> The mapVirtReg call below adds elements to the DenseMap. When a DenseMap grows/shrinks it might be re-allocated. So the pointer to the SmallVector (stored inside the map elements), called `UserVals` here, may be a dangling pointer after the first call to mapVirtReg and the vector it points to may have been moved.
> 
> So the iteration over *UserVals here is not safe, and asan complains about accessing deallocated memory.
> 
> A quick workaround that I'm trying downstream is to copy the vector like this:
> 
> ```
> diff --git a/llvm/lib/CodeGen/LiveDebugVariables.cpp b/llvm/lib/CodeGen/LiveDebugVariables.cpp
> index 6032bfe48af..4659ebea254 100644
> --- a/llvm/lib/CodeGen/LiveDebugVariables.cpp
> +++ b/llvm/lib/CodeGen/LiveDebugVariables.cpp
> @@ -1144,10 +1144,16 @@ void LDVImpl::splitRegister(unsigned OldReg, ArrayRef<unsigned> NewRegs) {
>      return;
>  
>    // Map all of the new virtual registers.
> -  if (auto *UserVals = lookupVirtReg(OldReg))
> -    for (auto *UV : *UserVals)
> +  if (auto *UserVals = lookupVirtReg(OldReg)) {
> +    // Make a copy of the vector. This is needed because VirtRegToUserVals is
> +    // updated in the loop below, so the pointer into the VirtRegToUserVals
> +    // DenseMap may become invalid in case the map is reallocated.
> +    SmallVector<UserValue *, 4> UserValsCopy;
> +    UserValsCopy.assign(UserVals->begin(), UserVals->end());
> +    for (auto *UV : UserValsCopy)
>        for (unsigned i = 0; i != NewRegs.size(); ++i)
>          mapVirtReg(NewRegs[i], UV);
> +  }
>  }
> ```
> 
> No idea if this impacts the performance negatively (considering that the idea with the refactoring into using a DenseMap seem to be to improve compile speed maybe there are better solutions). But right now we the code is broken so I guess we need to submit a workaround or a revert.
> A quick workaround that I'm trying downstream is to copy the vector like this

Replacing the DenseMap LDVImpl::VirtRegToUserVals uses with a std::unordered_map looks like it should also work.

I started work on this patch because @jmorse found that during an asan build LDV consumed ~30% CPU time when building AMDGPUDisassembler.cpp. This worst-case is reduced to <3% for me with my faulty patch and there is no noticeable difference when adding either of these changes. I'll look at the build time impact of these two approaches on self host builds when I get the chance but I'm leaning towards the unordered_map because it makes it harder to run into this again.

Thanks again for the heads up!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68816