[PATCH] D39334: [Driver] Add Scudo as a possible -fsanitize= option

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad added inline comments.


================
Comment at: lib/Driver/SanitizerArgs.cpp:386
+      std::make_pair(Scudo, Thread), std::make_pair(Scudo, Memory),
+      std::make_pair(Scudo, KernelAddress), std::make_pair(Scudo, Undefined) };
   for (auto G : IncompatibleGroups) {
----------------
alekseyshl wrote:
> It's a pair of SanitizerMasks, why not add std::make_pair(Scudo, Address | Leak | Thread | Memory | KernelAddress | Undefined)?
I'll see if it can be done, I went the way the previous checks were previously made.


================
Comment at: lib/Driver/ToolChains/CommonArgs.cpp:575
       }
     }
+    if (SanArgs.needsScudoRt())
----------------
alekseyshl wrote:
> Minor: add an empty line here or remove the one you added before needsUbsanRt.
Just to make sure: there are currently no empty line before needsUbsanRt (it was removed for consistency).
So currently there is 0 empty line: would you rather 1 on each side of needsUbsanRt?


================
Comment at: test/Driver/fsanitize.c:622
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=scudo,undefined %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-SCUDO-UBSAN
+// CHECK-SCUDO-UBSAN: invalid argument '-fsanitize=scudo' not allowed with '-fsanitize=undefined'
----------------
alekseyshl wrote:
> Why checking just these two combinations?
I figured the ASan one was to verify one incompatibility case, and the undefined one is here as well because it will end up being allowed when I figure out how to make the 2 cooperate nicely. I'll add them all.


https://reviews.llvm.org/D39334