[PATCH] D111443: [Driver] Fix ToolChain::getSanitizerArgs

[Yaxun Liu via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
yaxunl marked an inline comment as done.
Closed by commit rG0309e50f33f6: [Driver] Fix ToolChain::getSanitizerArgs 
(authored by yaxunl).
Herald added a project: clang.

Changed prior to commit:
  https://reviews.llvm.org/D111443?vs=386518=386662#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111443

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Driver/SanitizerArgs.h
  clang/include/clang/Driver/ToolChain.h
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/SanitizerArgs.cpp
  clang/lib/Driver/ToolChain.cpp
  clang/lib/Driver/ToolChains/AIX.h
  clang/lib/Driver/ToolChains/AMDGPU.h
  clang/lib/Driver/ToolChains/BareMetal.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/ToolChains/CloudABI.cpp
  clang/lib/Driver/ToolChains/CloudABI.h
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/lib/Driver/ToolChains/CrossWindows.cpp
  clang/lib/Driver/ToolChains/CrossWindows.h
  clang/lib/Driver/ToolChains/Cuda.h
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/lib/Driver/ToolChains/Darwin.h
  clang/lib/Driver/ToolChains/FreeBSD.cpp
  clang/lib/Driver/ToolChains/FreeBSD.h
  clang/lib/Driver/ToolChains/Fuchsia.cpp
  clang/lib/Driver/ToolChains/Fuchsia.h
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/lib/Driver/ToolChains/Gnu.h
  clang/lib/Driver/ToolChains/HIP.cpp
  clang/lib/Driver/ToolChains/Haiku.h
  clang/lib/Driver/ToolChains/Linux.cpp
  clang/lib/Driver/ToolChains/Linux.h
  clang/lib/Driver/ToolChains/MSP430.h
  clang/lib/Driver/ToolChains/MSVC.cpp
  clang/lib/Driver/ToolChains/MSVC.h
  clang/lib/Driver/ToolChains/MinGW.cpp
  clang/lib/Driver/ToolChains/MinGW.h
  clang/lib/Driver/ToolChains/NetBSD.cpp
  clang/lib/Driver/ToolChains/OpenBSD.h
  clang/lib/Driver/ToolChains/PS4CPU.cpp
  clang/lib/Driver/ToolChains/PS4CPU.h
  clang/lib/Driver/ToolChains/TCE.cpp
  clang/lib/Driver/ToolChains/TCE.h
  clang/lib/Driver/ToolChains/VEToolchain.cpp
  clang/lib/Driver/ToolChains/VEToolchain.h
  clang/lib/Driver/ToolChains/WebAssembly.cpp
  clang/lib/Driver/ToolChains/WebAssembly.h
  clang/lib/Driver/ToolChains/XCore.cpp
  clang/lib/Driver/ToolChains/XCore.h
  clang/lib/Driver/ToolChains/ZOS.h
  clang/test/Driver/hip-sanitize-options.hip

Index: clang/test/Driver/hip-sanitize-options.hip
===
--- clang/test/Driver/hip-sanitize-options.hip
+++ clang/test/Driver/hip-sanitize-options.hip
@@ -1,47 +1,67 @@
 // REQUIRES: clang-driver, x86-registered-target, amdgpu-registered-target
 
-// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900 \
+// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900:xnack+ \
 // RUN:   -fsanitize=address \
 // RUN:   -nogpuinc --rocm-path=%S/Inputs/rocm \
-// RUN:   %s 2>&1 | FileCheck %s
+// RUN:   %s 2>&1 | FileCheck -check-prefixes=NORDC %s
 
-// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900 \
+// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900:xnack+ \
 // RUN:   -fsanitize=address -fno-gpu-sanitize \
 // RUN:   -nogpuinc --rocm-path=%S/Inputs/rocm \
 // RUN:   %s 2>&1 | FileCheck %s
 
-// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900 \
+// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900:xnack+ \
 // RUN:   -fsanitize=address -fgpu-sanitize \
 // RUN:   -nogpuinc --rocm-path=%S/Inputs/rocm \
 // RUN:   %s 2>&1 | FileCheck -check-prefixes=NORDC %s
 
-// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900 \
+// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900:xnack+ \
 // RUN:   -fsanitize=address -fgpu-sanitize -fgpu-rdc \
 // RUN:   -nogpuinc --rocm-path=%S/Inputs/rocm \
 // RUN:   %s 2>&1 | FileCheck -check-prefixes=RDC %s
 
-// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900 \
+// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900:xnack+ \
 // RUN:   -fsanitize=address -fgpu-sanitize \
 // RUN:   -nogpuinc --rocm-path=%S/Inputs/rocm-invalid \
 // RUN:   %s 2>&1 | FileCheck -check-prefixes=FAIL %s
 
 // RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900:xnack- \
-// RUN:   -fsanitize=address -fgpu-sanitize \
-// RUN:   -nogpuinc --rocm-path=%S/Inputs/rocm \
-// RUN:   %s 2>&1 | FileCheck -check-prefix=XNACK %s
+// RUN:   --offload-arch=gfx900:xnack+ --offload-arch=gfx906 -fsanitize=address -fgpu-sanitize \
+// RUN:   -fsanitize=leak -nogpuinc --rocm-path=%S/Inputs/rocm \
+// RUN:   %s 2>&1 | FileCheck -check-prefixes=XNACK %s
+
+// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900:xnack- \
+// RUN:   --offload-arch=gfx900:xnack+ --offload-arch=gfx906 -fsanitize=address -fgpu-sanitize \
+// RUN:   

[PATCH] D111443: [Driver] Fix ToolChain::getSanitizerArgs

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis accepted this revision.
eugenis added a comment.
This revision is now accepted and ready to land.

LGTM


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

https://reviews.llvm.org/D111443

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111443: [Driver] Fix ToolChain::getSanitizerArgs

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked an inline comment as done.
yaxunl added inline comments.



Comment at: clang/lib/Driver/ToolChain.cpp:124
+  }
+  return SanitizerArgs(*this, JobArgs, /*DiagnoseErrors=*/false);
 }

eugenis wrote:
> SanitizerArgs SanArgs(*this, JobArgs, !SanitizerArgsChecked);
> SanitizerArgsChecked = true;
> return SanArgs;
> 
done



Comment at: clang/lib/Driver/ToolChains/FreeBSD.cpp:471
+bool FreeBSD::isPIEDefault(const llvm::opt::ArgList ) const {
+  return getSanitizerArgs(Args).requiresPIE();
+}

eugenis wrote:
> it looks like we do a lot of sanitizerargs recomputing. Is it worth it to try 
> and cache it?
In general, SanitizerArgs is per job arguments, not per toolchain. There is no 
good way to cache it.

On the other hand, I would expect time spent in argument parsing is trivial 
compared to time spent in compilation for common programs.


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

https://reviews.llvm.org/D111443

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111443: [Driver] Fix ToolChain::getSanitizerArgs

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 386518.
yaxunl marked an inline comment as done.
yaxunl added a comment.

Revised by Evgenii's comments


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

https://reviews.llvm.org/D111443

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Driver/SanitizerArgs.h
  clang/include/clang/Driver/ToolChain.h
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/SanitizerArgs.cpp
  clang/lib/Driver/ToolChain.cpp
  clang/lib/Driver/ToolChains/AIX.h
  clang/lib/Driver/ToolChains/AMDGPU.h
  clang/lib/Driver/ToolChains/BareMetal.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/ToolChains/CloudABI.cpp
  clang/lib/Driver/ToolChains/CloudABI.h
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/lib/Driver/ToolChains/CrossWindows.cpp
  clang/lib/Driver/ToolChains/CrossWindows.h
  clang/lib/Driver/ToolChains/Cuda.h
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/lib/Driver/ToolChains/Darwin.h
  clang/lib/Driver/ToolChains/FreeBSD.cpp
  clang/lib/Driver/ToolChains/FreeBSD.h
  clang/lib/Driver/ToolChains/Fuchsia.cpp
  clang/lib/Driver/ToolChains/Fuchsia.h
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/lib/Driver/ToolChains/Gnu.h
  clang/lib/Driver/ToolChains/HIP.cpp
  clang/lib/Driver/ToolChains/Haiku.h
  clang/lib/Driver/ToolChains/Linux.cpp
  clang/lib/Driver/ToolChains/Linux.h
  clang/lib/Driver/ToolChains/MSP430.h
  clang/lib/Driver/ToolChains/MSVC.cpp
  clang/lib/Driver/ToolChains/MSVC.h
  clang/lib/Driver/ToolChains/MinGW.cpp
  clang/lib/Driver/ToolChains/MinGW.h
  clang/lib/Driver/ToolChains/NetBSD.cpp
  clang/lib/Driver/ToolChains/OpenBSD.h
  clang/lib/Driver/ToolChains/PS4CPU.cpp
  clang/lib/Driver/ToolChains/PS4CPU.h
  clang/lib/Driver/ToolChains/TCE.cpp
  clang/lib/Driver/ToolChains/TCE.h
  clang/lib/Driver/ToolChains/VEToolchain.cpp
  clang/lib/Driver/ToolChains/VEToolchain.h
  clang/lib/Driver/ToolChains/WebAssembly.cpp
  clang/lib/Driver/ToolChains/WebAssembly.h
  clang/lib/Driver/ToolChains/XCore.cpp
  clang/lib/Driver/ToolChains/XCore.h
  clang/lib/Driver/ToolChains/ZOS.h
  clang/test/Driver/hip-sanitize-options.hip

Index: clang/test/Driver/hip-sanitize-options.hip
===
--- clang/test/Driver/hip-sanitize-options.hip
+++ clang/test/Driver/hip-sanitize-options.hip
@@ -1,47 +1,67 @@
 // REQUIRES: clang-driver, x86-registered-target, amdgpu-registered-target
 
-// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900 \
+// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900:xnack+ \
 // RUN:   -fsanitize=address \
 // RUN:   -nogpuinc --rocm-path=%S/Inputs/rocm \
-// RUN:   %s 2>&1 | FileCheck %s
+// RUN:   %s 2>&1 | FileCheck -check-prefixes=NORDC %s
 
-// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900 \
+// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900:xnack+ \
 // RUN:   -fsanitize=address -fno-gpu-sanitize \
 // RUN:   -nogpuinc --rocm-path=%S/Inputs/rocm \
 // RUN:   %s 2>&1 | FileCheck %s
 
-// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900 \
+// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900:xnack+ \
 // RUN:   -fsanitize=address -fgpu-sanitize \
 // RUN:   -nogpuinc --rocm-path=%S/Inputs/rocm \
 // RUN:   %s 2>&1 | FileCheck -check-prefixes=NORDC %s
 
-// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900 \
+// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900:xnack+ \
 // RUN:   -fsanitize=address -fgpu-sanitize -fgpu-rdc \
 // RUN:   -nogpuinc --rocm-path=%S/Inputs/rocm \
 // RUN:   %s 2>&1 | FileCheck -check-prefixes=RDC %s
 
-// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900 \
+// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900:xnack+ \
 // RUN:   -fsanitize=address -fgpu-sanitize \
 // RUN:   -nogpuinc --rocm-path=%S/Inputs/rocm-invalid \
 // RUN:   %s 2>&1 | FileCheck -check-prefixes=FAIL %s
 
 // RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900:xnack- \
-// RUN:   -fsanitize=address -fgpu-sanitize \
-// RUN:   -nogpuinc --rocm-path=%S/Inputs/rocm \
-// RUN:   %s 2>&1 | FileCheck -check-prefix=XNACK %s
+// RUN:   --offload-arch=gfx900:xnack+ --offload-arch=gfx906 -fsanitize=address -fgpu-sanitize \
+// RUN:   -fsanitize=leak -nogpuinc --rocm-path=%S/Inputs/rocm \
+// RUN:   %s 2>&1 | FileCheck -check-prefixes=XNACK %s
+
+// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900:xnack- \
+// RUN:   --offload-arch=gfx900:xnack+ --offload-arch=gfx906 -fsanitize=address -fgpu-sanitize \
+// RUN:   -fsanitize=leak -nogpuinc --rocm-path=%S/Inputs/rocm \
+// RUN:   %s 2>&1 | FileCheck -check-prefixes=XNACKNEG %s
 
 // CHECK-NOT: {{"[^"]*clang[^"]*".* "-fcuda-is-device".* "-fsanitize=address"}}
+// CHECK-NOT: {{"[^"]*clang[^"]*".* "-fcuda-is-device".* "-mlink-bitcode-file" 

[PATCH] D111443: [Driver] Fix ToolChain::getSanitizerArgs

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added inline comments.



Comment at: clang/lib/Driver/ToolChain.cpp:124
+  }
+  return SanitizerArgs(*this, JobArgs, /*DiagnoseErrors=*/false);
 }

SanitizerArgs SanArgs(*this, JobArgs, !SanitizerArgsChecked);
SanitizerArgsChecked = true;
return SanArgs;




Comment at: clang/lib/Driver/ToolChains/FreeBSD.cpp:471
+bool FreeBSD::isPIEDefault(const llvm::opt::ArgList ) const {
+  return getSanitizerArgs(Args).requiresPIE();
+}

it looks like we do a lot of sanitizerargs recomputing. Is it worth it to try 
and cache it?


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

https://reviews.llvm.org/D111443

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111443: [Driver] Fix ToolChain::getSanitizerArgs

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

@eugenis Any further changes needed? Thanks.


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

https://reviews.llvm.org/D111443

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111443: [Driver] Fix ToolChain::getSanitizerArgs

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

I'll defer to @eugenis. Overall it looks OK to be.


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

https://reviews.llvm.org/D111443

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111443: [Driver] Fix ToolChain::getSanitizerArgs

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

@tra Any further concerns? Thanks.


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

https://reviews.llvm.org/D111443

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111443: [Driver] Fix ToolChain::getSanitizerArgs

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

The approach looks reasonable to me.


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

https://reviews.llvm.org/D111443

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111443: [Driver] Fix ToolChain::getSanitizerArgs

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

ping. I have made changes so that the diagnostics about sanitizer args are only 
emitted once. Any further changes or concerns? Thanks.


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

https://reviews.llvm.org/D111443

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111443: [Driver] Fix ToolChain::getSanitizerArgs

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked an inline comment as done.
yaxunl added a comment.

ping


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

https://reviews.llvm.org/D111443

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111443: [Driver] Fix ToolChain::getSanitizerArgs

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 379806.
yaxunl edited the summary of this revision.
yaxunl added a comment.

diagnose sanitizer args only once.


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

https://reviews.llvm.org/D111443

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Driver/SanitizerArgs.h
  clang/include/clang/Driver/ToolChain.h
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/SanitizerArgs.cpp
  clang/lib/Driver/ToolChain.cpp
  clang/lib/Driver/ToolChains/AIX.h
  clang/lib/Driver/ToolChains/AMDGPU.h
  clang/lib/Driver/ToolChains/BareMetal.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/ToolChains/CloudABI.cpp
  clang/lib/Driver/ToolChains/CloudABI.h
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/lib/Driver/ToolChains/CrossWindows.cpp
  clang/lib/Driver/ToolChains/CrossWindows.h
  clang/lib/Driver/ToolChains/Cuda.h
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/lib/Driver/ToolChains/Darwin.h
  clang/lib/Driver/ToolChains/FreeBSD.cpp
  clang/lib/Driver/ToolChains/FreeBSD.h
  clang/lib/Driver/ToolChains/Fuchsia.cpp
  clang/lib/Driver/ToolChains/Fuchsia.h
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/lib/Driver/ToolChains/Gnu.h
  clang/lib/Driver/ToolChains/HIP.cpp
  clang/lib/Driver/ToolChains/Haiku.h
  clang/lib/Driver/ToolChains/Linux.cpp
  clang/lib/Driver/ToolChains/Linux.h
  clang/lib/Driver/ToolChains/MSP430.h
  clang/lib/Driver/ToolChains/MSVC.cpp
  clang/lib/Driver/ToolChains/MSVC.h
  clang/lib/Driver/ToolChains/MinGW.cpp
  clang/lib/Driver/ToolChains/MinGW.h
  clang/lib/Driver/ToolChains/NetBSD.cpp
  clang/lib/Driver/ToolChains/OpenBSD.h
  clang/lib/Driver/ToolChains/PS4CPU.cpp
  clang/lib/Driver/ToolChains/PS4CPU.h
  clang/lib/Driver/ToolChains/TCE.cpp
  clang/lib/Driver/ToolChains/TCE.h
  clang/lib/Driver/ToolChains/VEToolchain.cpp
  clang/lib/Driver/ToolChains/VEToolchain.h
  clang/lib/Driver/ToolChains/WebAssembly.cpp
  clang/lib/Driver/ToolChains/WebAssembly.h
  clang/lib/Driver/ToolChains/XCore.cpp
  clang/lib/Driver/ToolChains/XCore.h
  clang/lib/Driver/ToolChains/ZOS.h
  clang/test/Driver/hip-sanitize-options.hip

Index: clang/test/Driver/hip-sanitize-options.hip
===
--- clang/test/Driver/hip-sanitize-options.hip
+++ clang/test/Driver/hip-sanitize-options.hip
@@ -1,47 +1,67 @@
 // REQUIRES: clang-driver, x86-registered-target, amdgpu-registered-target
 
-// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900 \
+// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900:xnack+ \
 // RUN:   -fsanitize=address \
 // RUN:   -nogpuinc --rocm-path=%S/Inputs/rocm \
-// RUN:   %s 2>&1 | FileCheck %s
+// RUN:   %s 2>&1 | FileCheck -check-prefixes=NORDC %s
 
-// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900 \
+// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900:xnack+ \
 // RUN:   -fsanitize=address -fno-gpu-sanitize \
 // RUN:   -nogpuinc --rocm-path=%S/Inputs/rocm \
 // RUN:   %s 2>&1 | FileCheck %s
 
-// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900 \
+// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900:xnack+ \
 // RUN:   -fsanitize=address -fgpu-sanitize \
 // RUN:   -nogpuinc --rocm-path=%S/Inputs/rocm \
 // RUN:   %s 2>&1 | FileCheck -check-prefixes=NORDC %s
 
-// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900 \
+// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900:xnack+ \
 // RUN:   -fsanitize=address -fgpu-sanitize -fgpu-rdc \
 // RUN:   -nogpuinc --rocm-path=%S/Inputs/rocm \
 // RUN:   %s 2>&1 | FileCheck -check-prefixes=RDC %s
 
-// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900 \
+// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900:xnack+ \
 // RUN:   -fsanitize=address -fgpu-sanitize \
 // RUN:   -nogpuinc --rocm-path=%S/Inputs/rocm-invalid \
 // RUN:   %s 2>&1 | FileCheck -check-prefixes=FAIL %s
 
 // RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900:xnack- \
-// RUN:   -fsanitize=address -fgpu-sanitize \
-// RUN:   -nogpuinc --rocm-path=%S/Inputs/rocm \
-// RUN:   %s 2>&1 | FileCheck -check-prefix=XNACK %s
+// RUN:   --offload-arch=gfx900:xnack+ --offload-arch=gfx906 -fsanitize=address -fgpu-sanitize \
+// RUN:   -fsanitize=leak -nogpuinc --rocm-path=%S/Inputs/rocm \
+// RUN:   %s 2>&1 | FileCheck -check-prefixes=XNACK %s
+
+// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900:xnack- \
+// RUN:   --offload-arch=gfx900:xnack+ --offload-arch=gfx906 -fsanitize=address -fgpu-sanitize \
+// RUN:   -fsanitize=leak -nogpuinc --rocm-path=%S/Inputs/rocm \
+// RUN:   %s 2>&1 | FileCheck -check-prefixes=XNACKNEG %s
 
 // CHECK-NOT: {{"[^"]*clang[^"]*".* "-fcuda-is-device".* "-fsanitize=address"}}
+// CHECK-NOT: {{"[^"]*clang[^"]*".* "-fcuda-is-device".* "-mlink-bitcode-file" 

[PATCH] D111443: [Driver] Fix ToolChain::getSanitizerArgs

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

In D111443#3062139 , @eugenis wrote:

> Right, but a cache for SanitizerArgs is not enough to avoid repeated 
> diagnostics, is it? Ex. if I request a non-existing sanitizer, I think I 
> would get errors from host arg parsing, as well as from each of device1 and 
> device2, because each device will have a unique ArgList.
>
> Is it even possible for the driver to introduce new diagnostics in offload 
> SanitizerArgs parsing? Is it possible to catch those cases ahead of time, 
> when parsing SanitizerArgs for the first time, by looking at a target triple 
> or something? That would be the most user friendly approach.

I think it makes sense to assume offloading toolchain will not introduce extra 
diagnostics inside SanitizerArgs than the default toolchain, therefore it 
should be OK to let the default toolchain check sanitizer args for once. I will 
try make that change.


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

https://reviews.llvm.org/D111443

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111443: [Driver] Fix ToolChain::getSanitizerArgs

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

Right, but a cache for SanitizerArgs is not enough to avoid repeated 
diagnostics, is it? Ex. if I request a non-existing sanitizer, I think I would 
get errors from host arg parsing, as well as from each of device1 and device2, 
because each device will have a unique ArgList.

Is it even possible for the driver to introduce new diagnostics in offload 
SanitizerArgs parsing? Is it possible to catch those cases ahead of time, when 
parsing SanitizerArgs for the first time, by looking at a target triple or 
something? That would be the most user friendly approach.


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

https://reviews.llvm.org/D111443

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111443: [Driver] Fix ToolChain::getSanitizerArgs

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

In D111443#3052697 , @tra wrote:

> In D111443#3052381 , @yaxunl wrote:
>
>> In D111443#3052099 , @tra wrote:
>>
>>> I'm curious why we need the cache at all. While the construction of 
>>> sanitizer args is hairy, it's only called few times from the driver and 
>>> will be lost in the noise compared to everything else.
>>
>> It is for avoiding repeated diagnostics. For example, -fsanitize=xxx is 
>> passed to clang and ld. If it is parsed twice, the diagnostics are emitted 
>> twice. There are lit tests to prevent the repeated diagnostics.
>> For this reason, using job action as cache key will not work since it will 
>> not prevent repeated diagnostics.
>
> Fair enough. Perhaps that's what we need to refactor first. We could separate 
> processing of the arguments from diagnosing issues there. I.e. something like 
> `getSanitizerArgs(const llvm::opt::ArgList , bool diag = false)` and 
> call it once with diag=true. Or just make diag a static local var and just 
> enable diags on the first call.



>> For most non-offloading toolchain, ld and clang job action will not create 
>> new ArgList, therefore they share the same ArgList and avoid repeated 
>> parsing. The ArgList persists for the whole life span of driver, therefore 
>> no life time issue.
>
> It may work now, but to me it looks like a dependence on an implementation 
> detail. I do not think it's reasonable to assume that ArgList pointer is 
> immutable and that it coexists with particular job.
>
> @eugenis -- WDYT?



In D111443#3055522 , @eugenis wrote:

> Don't we want to diagnose the problems in the job's command line? What kind 
> of changes can the driver do there that would affect SanitizerArgs?
>
> I wonder if diagnostics should still be performed on the job args, but 
> presented to the user differently, mainly because we can no longer refer to 
> the user-provided command line?

For offloading toolchain, the driver could remove -fsanitize for certain 
devices if they do not support them. Consider the following use case:

-fsanitize=address is passed by the user to clang driver, clang driver 
constructs 4 jobs by passing translated arguments to the tools:

1. clang for host: -fsanitize=address is passed to clang tool

2. ld for host: -fsantize=address is passed to host linker tool

3. clang for device 1: -fsanitize=address is not passed to clang tool

4. clang for device 2: -fsanitize=address is passed to clang tool

This demonstrates the necessity to parse the sanitizer arguments per job 
instead of per toolchain, since the same toolchain may parse different 
sanitizer arguments for different jobs.

If we need not avoid repeated diagnostics, we just need to parse the arguments 
for each job.

However, if we passed an invalid sanitizer argument to jobs 1, 2, and 4, we 
will get 3 identical diagnostics. This is annoying. That is why there are lit 
tests to prevent that.

Basically we need a way to know if the same argument list has been diagnosed. 
The simplest way is to check the pointer to the argument list since toolchain 
will reuse the original argument list if it does not change it. In this case, 
jobs 1, 2 and 4 share the same pointer to argument list, therefore they will be 
diagnosed only once. Since they are the same, they can be cached by pointer to 
the argument list.

This is also why separating diagnosing and parsing of sanitizer args does not 
help avoid repeated diagnostics for different jobs, since different jobs may 
have different sanitizer args. Then you have to have a map IsDiagnosed[JobArg] 
to track whether a job arg has been diagnosed. However, if you have that, then 
it is not too much different than having a cache for SanitizerArgs.


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

https://reviews.llvm.org/D111443

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111443: [Driver] Fix ToolChain::getSanitizerArgs

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

Don't we want to diagnose the problems in the job's command line? What kind of 
changes can the driver do there that would affect SanitizerArgs?

I wonder if diagnostics should still be performed on the job args, but 
presented to the user differently, mainly because we can no longer refer to the 
user-provided command line?


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

https://reviews.llvm.org/D111443

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111443: [Driver] Fix ToolChain::getSanitizerArgs

[Artem Belevich via Phabricator via cfe-commits]

tra added a reviewer: eugenis.
tra added a subscriber: eugenis.
tra added a comment.

In D111443#3052381 , @yaxunl wrote:

> In D111443#3052099 , @tra wrote:
>
>> I'm curious why we need the cache at all. While the construction of 
>> sanitizer args is hairy, it's only called few times from the driver and will 
>> be lost in the noise compared to everything else.
>
> It is for avoiding repeated diagnostics. For example, -fsanitize=xxx is 
> passed to clang and ld. If it is parsed twice, the diagnostics are emitted 
> twice. There are lit tests to prevent the repeated diagnostics.
> For this reason, using job action as cache key will not work since it will 
> not prevent repeated diagnostics.

Fair enough. Perhaps that's what we need to refactor first. We could separate 
processing of the arguments from diagnosing issues there. I.e. something like 
`getSanitizerArgs(const llvm::opt::ArgList , bool diag = false)` and 
call it once with diag=true. Or just make diag a static local var and just 
enable diags on the first call.

> For most non-offloading toolchain, ld and clang job action will not create 
> new ArgList, therefore they share the same ArgList and avoid repeated 
> parsing. The ArgList persists for the whole life span of driver, therefore no 
> life time issue.

It may work now, but to me it looks like a dependence on an implementation 
detail. I do not think it's reasonable to assume that ArgList pointer is 
immutable and that it coexists with particular job.

@eugenis -- WDYT?


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

https://reviews.llvm.org/D111443

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111443: [Driver] Fix ToolChain::getSanitizerArgs

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

In D111443#3052099 , @tra wrote:

> I'm curious why we need the cache at all. While the construction of sanitizer 
> args is hairy, it's only called few times from the driver and will be lost in 
> the noise compared to everything else.

It is for avoiding repeated diagnostics. For example, -fsanitize=xxx is passed 
to clang and ld. If it is parsed twice, the diagnostics are emitted twice. 
There are lit tests to prevent the repeated diagnostics.

For this reason, using job action as cache key will not work since it will not 
prevent repeated diagnostics. For most non-offloading toolchain, ld and clang 
job action will not create new ArgList, therefore they share the same ArgList 
and avoid repeated parsing. The ArgList persists for the whole life span of 
driver, therefore no life time issue.


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

https://reviews.llvm.org/D111443

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111443: [Driver] Fix ToolChain::getSanitizerArgs

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

I'm curious why we need the cache at all. While the construction of sanitizer 
args is hairy, it's only called few times from the driver and will be lost in 
the noise compared to everything else.




Comment at: clang/include/clang/Driver/ToolChain.h:165
 
-  mutable std::unique_ptr SanitizerArguments;
+  mutable std::map>
+  SanArgs;

Using argument list as a proxy for a job is somewhat questionable. ArgList may 
not necessarily be related to a particular job at all.

Can we use JobAction as the key instead and default to full reconstruction of 
sanitizer args where it's not available? Or just always construct them without 
caching?


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

https://reviews.llvm.org/D111443

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D111443: [Driver] Fix ToolChain::getSanitizerArgs

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl created this revision.
yaxunl added a reviewer: tra.
Herald added subscribers: abrachet, kerbowa, mstorsjo, jgravelle-google, 
sbc100, nhaehnle, jvesely, dschuff, emaste.
yaxunl requested review of this revision.
Herald added a subscriber: aheejin.

The driver uses class SanitizerArgs to store parsed sanitizer arguments. It 
keeps a cached
SanitizerArgs object in ToolChain and uses it for different jobs. This does not 
work if
the sanitizer options are different for different jobs, which could happen when 
an
offloading toolchain translates the options for different jobs.

To fix this, SanitizerArgs should be created by using the actual arguments 
passed
to jobs instead of the original arguments passed to the driver, since the 
toolchain
may change the original arguments. And the cache should be per job instead of
per toolchain.

This patch also fixes HIP toolchain for handling -fgpu-sanitize: a warning is 
emitted
for GPU's not supporting sanitizer and skipped. This is for backward 
compatibility
with existing -fsanitize options.


https://reviews.llvm.org/D111443

Files:
  clang/include/clang/Driver/ToolChain.h
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChain.cpp
  clang/lib/Driver/ToolChains/AIX.h
  clang/lib/Driver/ToolChains/AMDGPU.h
  clang/lib/Driver/ToolChains/BareMetal.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/ToolChains/CloudABI.cpp
  clang/lib/Driver/ToolChains/CloudABI.h
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/lib/Driver/ToolChains/CrossWindows.cpp
  clang/lib/Driver/ToolChains/CrossWindows.h
  clang/lib/Driver/ToolChains/Cuda.h
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/lib/Driver/ToolChains/Darwin.h
  clang/lib/Driver/ToolChains/FreeBSD.cpp
  clang/lib/Driver/ToolChains/FreeBSD.h
  clang/lib/Driver/ToolChains/Fuchsia.cpp
  clang/lib/Driver/ToolChains/Fuchsia.h
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/lib/Driver/ToolChains/Gnu.h
  clang/lib/Driver/ToolChains/HIP.cpp
  clang/lib/Driver/ToolChains/Haiku.h
  clang/lib/Driver/ToolChains/Linux.cpp
  clang/lib/Driver/ToolChains/Linux.h
  clang/lib/Driver/ToolChains/MSP430.h
  clang/lib/Driver/ToolChains/MSVC.cpp
  clang/lib/Driver/ToolChains/MSVC.h
  clang/lib/Driver/ToolChains/MinGW.cpp
  clang/lib/Driver/ToolChains/MinGW.h
  clang/lib/Driver/ToolChains/NetBSD.cpp
  clang/lib/Driver/ToolChains/OpenBSD.h
  clang/lib/Driver/ToolChains/PS4CPU.cpp
  clang/lib/Driver/ToolChains/PS4CPU.h
  clang/lib/Driver/ToolChains/TCE.cpp
  clang/lib/Driver/ToolChains/TCE.h
  clang/lib/Driver/ToolChains/VEToolchain.cpp
  clang/lib/Driver/ToolChains/VEToolchain.h
  clang/lib/Driver/ToolChains/WebAssembly.cpp
  clang/lib/Driver/ToolChains/WebAssembly.h
  clang/lib/Driver/ToolChains/XCore.cpp
  clang/lib/Driver/ToolChains/XCore.h
  clang/lib/Driver/ToolChains/ZOS.h
  clang/test/Driver/hip-sanitize-options.hip

Index: clang/test/Driver/hip-sanitize-options.hip
===
--- clang/test/Driver/hip-sanitize-options.hip
+++ clang/test/Driver/hip-sanitize-options.hip
@@ -1,47 +1,60 @@
 // REQUIRES: clang-driver, x86-registered-target, amdgpu-registered-target
 
-// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900 \
+// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900:xnack+ \
 // RUN:   -fsanitize=address \
 // RUN:   -nogpuinc --rocm-path=%S/Inputs/rocm \
 // RUN:   %s 2>&1 | FileCheck %s
 
-// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900 \
+// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900:xnack+ \
 // RUN:   -fsanitize=address -fno-gpu-sanitize \
 // RUN:   -nogpuinc --rocm-path=%S/Inputs/rocm \
 // RUN:   %s 2>&1 | FileCheck %s
 
-// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900 \
+// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900:xnack+ \
 // RUN:   -fsanitize=address -fgpu-sanitize \
 // RUN:   -nogpuinc --rocm-path=%S/Inputs/rocm \
 // RUN:   %s 2>&1 | FileCheck -check-prefixes=NORDC %s
 
-// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900 \
+// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900:xnack+ \
 // RUN:   -fsanitize=address -fgpu-sanitize -fgpu-rdc \
 // RUN:   -nogpuinc --rocm-path=%S/Inputs/rocm \
 // RUN:   %s 2>&1 | FileCheck -check-prefixes=RDC %s
 
-// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900 \
+// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900:xnack+ \
 // RUN:   -fsanitize=address -fgpu-sanitize \
 // RUN:   -nogpuinc --rocm-path=%S/Inputs/rocm-invalid \
 // RUN:   %s 2>&1 | FileCheck -check-prefixes=FAIL %s
 
 // RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-arch=gfx900:xnack- \
-// RUN:   -fsanitize=address -fgpu-sanitize \
+// RUN:   --offload-arch=gfx900:xnack+ --offload-arch=gfx906 -fsanitize=address -fgpu-sanitize \
+// RUN:   -nogpuinc --rocm-path=%S/Inputs/rocm \
+//