Page MenuHomePhabricator

[CUDA] "Support" ASAN arguments in CudaToolChain
ClosedPublic

Authored by jhen on Dec 1 2016, 11:57 AM.

Details

Reviewers
kcc
jlebar
Summary

This fixes a bug that was introduced in rL287285. The bug made it illegal to pass -fsanitize=address during CUDA compilation because the CudaToolChain class was switched from deriving from the Linux toolchain class to deriving directly from the ToolChain toolchain class. When CudaToolChain derived from Linux, it used Linux's getSupportedSanitizers method, and that method allowed ASAN, but when it switched to deriving directly from ToolChain, it inherited a getSupportedSanitizers method that didn't allow for ASAN.

This patch fixes that bug by creating a getSupportedSanitizers method for CudaToolChain that supports ASAN.

This patch also fixes the test that checks that -fsanitize=address is passed correctly for CUDA builds. That test didn't used to notice if an error message was emitted, and that's why it didn't catch this bug when it was first introduced. With the fix from this patch, that test will now catch any similar bug in the future.

Event Timeline

jhen updated this revision to Diff 79963.Dec 1 2016, 11:57 AM
jhen retitled this revision from to [CUDA] Filter out dirver sanitizer args for NVPTX.
jhen updated this object.
jhen added a reviewer: jlebar.
jhen added a subscriber: cfe-commits.
jhen added a reviewer: kcc.Dec 1 2016, 1:04 PM

Before this patch, the following command would fail:

clang++ -fsanitize=address --cuda-gpu-arch=sm_20 -c example.cu

with error:

clang-4.0: error: unsupported option '-fsanitize=address' for target 'nvptx64-nvidia-cuda'

After this patch, there is no such compilation error.

I don't understand this part of the driver code very well, so I would appreciate any advice on whether this is the right way to fix this bug, and where would be the right place for me to put a test to check that it is working (i.e. ASAN still works for CUDA host code).

jlebar edited edge metadata.Dec 1 2016, 1:27 PM

where would be the right place for me to put a test to check that it is working (i.e. ASAN still works for CUDA host code).

I just ran the test from rL281680, and it passes because it doesn't notice clang raising the "unsupported option '-fsanitize=address" error. :)

I don't understand this part of the driver code very well

What I would probably do (other than ask Kostya, who may just Know) is first identify which commit caused the regression. That will probably give us some idea of whether or not this is the right fix (and whether or not the check in rL281680 is still necessary).

lib/Driver/SanitizerArgs.cpp
214 ↗(On Diff #79963)

Nit, remove braces.

kcc edited edge metadata.Dec 1 2016, 3:14 PM

at the very least this requires a test... Let me thing about the logic a bit more...

jlebar added a comment.Dec 1 2016, 3:15 PM
In D27316#611160, @kcc wrote:

at the very least this requires a test... Let me thing about the logic a bit more...

I think we just need to fix the test from the previous CL, which is buggy and didn't catch this regression.

kcc added a comment.Dec 1 2016, 3:22 PM

I am not sure this is going to work.
You essentially break on the first iteration in the beginning of the loop.
Why not exit the function before the loop?
Also, the loop "claims" the args and by breaking early you leave the args unclaimed.

I don't remember this code well enough to argue about it w/o tests. :(

jhen added a comment.Dec 1 2016, 3:29 PM
In D27316#611162, @kcc wrote:

I am not sure this is going to work.
You essentially break on the first iteration in the beginning of the loop.
Why not exit the function before the loop?
Also, the loop "claims" the args and by breaking early you leave the args unclaimed.

I don't remember this code well enough to argue about it w/o tests. :(

Thanks for taking a look. I just wanted to make sure nobody knew of a simple solution for this problem before I dug in further. Since none of us seems too familiar with this code, I'll do more investigation to try to find the right way to fix this. Once I do that I'll post it to this review (with a test or two to make sure it's working).

tra added a subscriber: tra.Dec 1 2016, 3:38 PM
jhen updated this revision to Diff 80003.Dec 1 2016, 5:38 PM
jhen edited edge metadata.
  • "Support" ASAN in CudaToolChain
jlebar accepted this revision.Dec 1 2016, 5:39 PM
jlebar edited edge metadata.
This revision is now accepted and ready to land.Dec 1 2016, 5:39 PM
jlebar added a comment.Dec 1 2016, 5:40 PM

This takes it back to how it used to be before I regressed it, which I think is good enough for now. If we want to revisit how we make this work for offloading toolchains in general, we can do that separately.

jhen retitled this revision from [CUDA] Filter out dirver sanitizer args for NVPTX to [CUDA] "Support" ASAN arguments in CudaToolChain.Dec 1 2016, 5:47 PM
jhen updated this object.
jhen edited edge metadata.
jhen closed this revision.Dec 2 2016, 9:48 AM
jhen marked an inline comment as done.