This is an archive of the discontinued LLVM Phabricator instance.

[ASAN][AMDGPU] Add support for accesses to global and constant addrspaces
ClosedPublic

Authored by rksharma on Mar 22 2021, 5:54 AM.

Details

Summary

TL;DR This patch adds address sanitizer instrumentation support for accesses to global and constant address spaces in AMDGPU. It strictly avoids instrumenting the stack and assumes x86 as the host.

This patch adds address sanitizer instrumentations to global accesses (addrspace 1 and 4) for the device/GPU code. These addresses are accessible from both the host/CPU and the device/GPU therefore we use the static shadow mapping from the host. For addresses explicitly in addrspace 1 and 4, we can follow the exact same instrumentation that is done for the host.
There are generic/flat addresses (addrspace 0) that can point to global memory (addrspace 1 and 4). We add runtime calls (llvm.amdgcn.is.shared and llvm.amdgcn.is.private) to check if the flat address is a global address at runtime. If it is then we instrument it as we would do for explicit accesses to addrspace 1 or 4.

Diff Detail

Event Timeline

rksharma created this revision.Mar 22 2021, 5:54 AM
rksharma requested review of this revision.Mar 22 2021, 5:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2021, 5:54 AM
rksharma updated this revision to Diff 332630.Mar 23 2021, 5:11 AM

clang-formatted

Could you please upload with "arc" tool so we can see context here?

Can you please point to some high level overview how this should work?
If it does not exists, could you please describe that in summary?

rksharma updated this revision to Diff 332928.Mar 24 2021, 5:23 AM
rksharma edited the summary of this revision. (Show Details)

Thanks! Added missing context and tried to explicitly state what we are trying to do in the description. I can make it more verbose if required. Please let me know if I can clarify or add something.

vitalybuka added subscribers: kda, eugenis.

LGTM with few minor comments

@eugenis and @kda mostly FYI

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
1725–1733

can you please move more AMD specific code into instrumentAMDGPUGenericAddress?

1901

Can you please extract all these AddressSpace checks into some functions with self explaining names?

maybe isUnsupportedAmdAddressSpace()
same for 3,5

2426–2429

can you sent this as a separate NFC patch?

llvm/lib/Transforms/Utils/ModuleUtils.cpp
92 ↗(On Diff #332928)

can you sent this as a separate NFC patch?
does it affect any existing tests?

rksharma updated this revision to Diff 340829.Apr 27 2021, 7:13 AM

Thanks for the review, I've addressed your comments, I've moved more AMDGPU specific code out of instrumentAddress and added a function to check unsupported address spaces.

rksharma marked an inline comment as done.Apr 27 2021, 7:17 AM
rksharma added inline comments.
llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
1725–1733

I tried and moved some more code out of instrumentAddress

1901

I've added one for 3 and 5 which are unsupported

Can you please clang-format the diff?

vitalybuka added inline comments.Apr 28 2021, 1:28 PM
llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
1400–1401

Why can we use isUnsupportedAMDGPUAddrspacehere(PtrTy)?

1705

And the rest of the patch.

1715

Please collect getOrInsertFunction in AddressSanitizer::initializeCallbacks

vitalybuka added inline comments.Apr 28 2021, 1:33 PM
llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
953–964
rksharma updated this revision to Diff 341408.Apr 28 2021, 10:43 PM

Addressed the review comments.

rksharma added inline comments.Apr 28 2021, 10:49 PM
llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
177–178

Is it fine that I've created variables for them or shall I directly use function names as string inside getOrInsertFunction?

1400–1401

We can use isUnsupported but we can not get rid of line:1395 because asan needs to check if the address is in a non-zero addrspace for non-amdgpu targets.

vitalybuka accepted this revision.Apr 30 2021, 1:51 AM
vitalybuka added inline comments.
llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
177–178

variable lgtm

1330–1336
1699–1701
1705

"." after comments is not done?

1900
This revision is now accepted and ready to land.Apr 30 2021, 1:51 AM
rksharma updated this revision to Diff 341810.Apr 30 2021, 2:20 AM

Updated all the comments in the patch, I'm sorry, initially, I misunderstood your review comment to be about something else. Thanks for pointing it out.

This revision was landed with ongoing or failed builds.May 2 2021, 8:36 PM
This revision was automatically updated to reflect the committed changes.

Are declarations for @llvm.amdgcn.is.shared supposed to be showing up in my IR, even when my target triple is _not_ amdgcn-amd?

Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2022, 9:42 AM

Are declarations for @llvm.amdgcn.is.shared supposed to be showing up in my IR, even when my target triple is _not_ amdgcn-amd?

That's surprising. What is your triple? What targets have you built for?

Are declarations for @llvm.amdgcn.is.shared supposed to be showing up in my IR, even when my target triple is _not_ amdgcn-amd?

@nickdesaulniers thanks for letting us know about this, I would like to understand more about this. Is it possible for you to share a simple test case (along with which target) and build script to me so that I can reproduce it on my end.

Are declarations for @llvm.amdgcn.is.shared supposed to be showing up in my IR, even when my target triple is _not_ amdgcn-amd?

@nickdesaulniers thanks for letting us know about this, I would like to understand more about this. Is it possible for you to share a simple test case (along with which target) and build script to me so that I can reproduce it on my end.

$ cd llvm-project
$ clang -cc1 -internal-isystem llvm/build/lib/clang/15.0.0/include -nostdsysteminc -std=gnu2x -triple x86_64-linux-gnu clang/test/CodeGen/attr-function-return.c -emit-llvm -o - -mfunction-return=thunk-extern -fsanitize=address

Are declarations for @llvm.amdgcn.is.shared supposed to be showing up in my IR, even when my target triple is _not_ amdgcn-amd?

@nickdesaulniers thanks for letting us know about this, I would like to understand more about this. Is it possible for you to share a simple test case (along with which target) and build script to me so that I can reproduce it on my end.

$ cd llvm-project
$ clang -cc1 -internal-isystem llvm/build/lib/clang/15.0.0/include -nostdsysteminc -std=gnu2x -triple x86_64-linux-gnu clang/test/CodeGen/attr-function-return.c -emit-llvm -o - -mfunction-return=thunk-extern -fsanitize=address

Ok thanks