[ASAN] NFC: Use addrspace cast for pointers in non-zero addrspace

[rksharma]

Summary

Pointers in non-zero address spaces need to be address space casted before appending to the used list.

[dblaikie]

Is this testable? Was it already tested and the test was asserting in some case? Is it not testable for some reason?

[rksharma]

The change in this patch is implicitly tested by all the tests introduced in https://reviews.llvm.org/D99071
asan-module pass will append globals to the used list and for AMDGPU these globals will be in non-zero addrspaces.

[dblaikie]

In D101363#2732717, @rksharma wrote:

The change in this patch is implicitly tested by all the tests introduced in https://reviews.llvm.org/D99071
asan-module pass will append globals to the used list and for AMDGPU these globals will be in non-zero addrspaces.

In the future - semantic changing patches should be part of commits that test them, not committed separately.

If this change was separable, it should've included separate tests with it - such as an IR test with a global in the used list, by the sounds of it?

[rksharma]

It does make sense. Like other Utils, I've created ModuleUtilsTest where I've added tests for appending into UsedList. Please take a look, https://reviews.llvm.org/D101746

[vitalybuka]

Sorry, somehow I missed that the function is not just internal Asan utility.
David, thanks for rising this up.

In D101363#2732772, @dblaikie wrote:

In D101363#2732717, @rksharma wrote:

The change in this patch is implicitly tested by all the tests introduced in https://reviews.llvm.org/D99071
asan-module pass will append globals to the used list and for AMDGPU these globals will be in non-zero addrspaces.

In the future - semantic changing patches should be part of commits that test them, not committed separately.

If this change was separable, it should've included separate tests with it - such as an IR test with a global in the used list, by the sounds of it?

In D101363#2732772, @dblaikie wrote:

In D101363#2732717, @rksharma wrote:

The change in this patch is implicitly tested by all the tests introduced in https://reviews.llvm.org/D99071
asan-module pass will append globals to the used list and for AMDGPU these globals will be in non-zero addrspaces.

In the future - semantic changing patches should be part of commits that test them, not committed separately.

If this change was separable, it should've included separate tests with it - such as an IR test with a global in the used list, by the sounds of it?

llvm/lib/Transforms/Utils/ModuleUtils.cpp

if (GV->hasInitializer()) {
           Init.push_back(C);
       }
     }
     GV->eraseFromParent();
   }
 
   Type *Int8PtrTy = llvm::Type::getInt8PtrTy(M.getContext());
   for (auto *V : Values) {
-    Constant *C = ConstantExpr::getBitCast(V, Int8PtrTy);
+    Constant *C = ConstantExpr::getPointerBitCastOrAddrSpaceCast(V, Int8PtrTy);
     if (InitAsSet.insert(C).second)
       Init.push_back(C);
   }
 
   if (Init.empty())
     return;
 
   ArrayType *ATy = ArrayType::get(Int8PtrTy, Init.size());