[PATCH] D99071: [ASAN][AMDGPU] Add support for accesses to global and constant addrspaces

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added reviewers: kda, eugenis.
vitalybuka added subscribers: kda, eugenis.
vitalybuka added a comment.

LGTM with few minor comments

@eugenis and @kda mostly FYI



================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:1703-1711
+  if (TargetTriple.isAMDGPU()) {
+    Type *PtrTy = cast<PointerType>(Addr->getType()->getScalarType());
+    unsigned int AddrSpace = PtrTy->getPointerAddressSpace();
+    if (AddrSpace == 0)
+      InsertBefore = instrumentAMDGPUGenericAddress(
+          OrigIns, InsertBefore, Addr, TypeSize, IsWrite, SizeArgument);
+    if (AddrSpace == 3 || AddrSpace == 5)
----------------
can you please move more AMD specific code into instrumentAMDGPUGenericAddress?



================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:1882
+      !(TargetTriple.isAMDGPU() &&
+        (G->getAddressSpace() == 1 || G->getAddressSpace() == 4)))
+    return false;
----------------
Can you please extract all these AddressSpace checks into some functions with self explaining names?

maybe isUnsupportedAmdAddressSpace()
same for 3,5


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:2406-2408
+    GlobalVariable *NewGlobal = new GlobalVariable(
+        M, NewTy, G->isConstant(), Linkage, NewInitializer, "", G,
+        G->getThreadLocalMode(), G->getAddressSpace());
----------------
can you sent this as a separate NFC patch?



================
Comment at: llvm/lib/Transforms/Utils/ModuleUtils.cpp:92
   for (auto *V : Values) {
-    Constant *C = ConstantExpr::getBitCast(V, Int8PtrTy);
+    Constant *C = ConstantExpr::getPointerBitCastOrAddrSpaceCast(V, Int8PtrTy);
     if (InitAsSet.insert(C).second)
----------------
can you sent this as  a separate NFC patch?
does it affect any existing tests?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99071