Addressed Alexey's comments.
Regarding the missing tests, I think it's best to add them at opt level, but I
can't do that now because there's no command line switch that enables KASan
(this mode only depends on the bool passed to the pass factory). I can add such
a switch, but then it can be used to work around the "ASan xor KASan"
restriction. WDYT?
================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:1359
@@ -1349,3 +1358,3 @@
bool Changed = false;
----------------
ygribov wrote:
> samsonov wrote:
> > Wait a second.... Do you do *anything* in AddressSanitizerModule pass in
> > `CompileKernel` mode? You don't add module constructor, and you don't
> > instrument globals. Maybe, we should just avoid creating this pass in the
> > first place?
> Or perhaps instrumentation of globals will follow shortly so it's ok to have
> this pass at hand.
> Or perhaps instrumentation of globals will follow shortly so it's ok to have
> this pass at hand.
Exactly, my intention was to add it soon.
================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:1379
@@ -1368,2 +1378,3 @@
const std::string ExpStr = Exp ? "exp_" : "";
+ const std::string SuffixStr = CompileKernel ? "N" : "_n";
const Type *ExpType = Exp ? Type::getInt32Ty(*C) : nullptr;
----------------
ygribov wrote:
> Perhaps use "_noabort" suffix to match gcc?
Looks like I've been comparing to the wrong gcc version, 4.9 didn't have the
'noabort' suffixes. Fixed.
================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:1405
@@ -1393,1 +1404,3 @@
+ const std::string MemIntrinCallbackPrefix =
+ CompileKernel ? std::string("") : ClMemoryAccessCallbackPrefix;
AsanMemmove = checkSanitizerInterfaceFunction(M.getOrInsertFunction(
----------------
samsonov wrote:
> Hm... So you don't have __asan_memset() in the kernel. Maybe, we shouldn't
> instrument mem intrinsics in this case at all, not replace intrinisics with
> call to plain memset()?
mm/kasan/kasan.c redefines memset() et al. making them call
__asan_{load,store}N (see
https://github.com/ramosian-glider/kasan/blob/kasan_llvmlinux/mm/kasan/kasan.c#L263),
so we just replace the intrinsics with calls to memset().
================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:1441
@@ +1440,3 @@
+ if (!CompileKernel) {
+ std::tie(AsanCtorFunction, AsanInitFunction) =
+ createSanitizerCtorAndInitFunctions(M, kAsanModuleCtorName,
----------------
samsonov wrote:
> You will access uninitialized memory in `AddressSanitizer::runOnFunction`
> (potentially in `AddressSanitizer::maybeInsertAsanInitAtFunctionEntry` as
> well, but the latter is relevant only for Mac).
In fact I didn't fully understand this piece before. I've looked std::tie up
and it's more clear to me now.
Moved the assignment outside the condition (or shall we NULL-initialize these
fields instead?)
================
Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:1530
@@ -1515,3 +1529,3 @@
- bool UseCalls = false;
+ bool UseCalls = CompileKernel;
if (ClInstrumentationWithCallsThreshold >= 0 &&
----------------
samsonov wrote:
> bool UseCalls = CompileKernel || (ClInstrumentationWithCallsThreshold >= 0
> && ...);
Done.
================
Comment at: tools/clang/include/clang/Basic/Sanitizers.def:44
@@ -43,1 +43,3 @@
+// Kernel AddressSanitizer (KASAN)
+SANITIZER("kernel-address", KernelAddress)
----------------
samsonov wrote:
> What is the "correct" way to capitalize it? KASAN? KASan?
It hasn't been established yet, but Dima says we prefer "KASan" (which
surprised me). Fixed here and in tools/clang/lib/CodeGen/CodeGenModule.cpp
================
Comment at: tools/clang/lib/AST/Decl.cpp:3684
@@ -3683,2 +3683,3 @@
ASTContext &Context = getASTContext();
- if (!Context.getLangOpts().Sanitize.has(SanitizerKind::Address) ||
+ if (!(Context.getLangOpts().Sanitize.has(SanitizerKind::Address) ||
+ Context.getLangOpts().Sanitize.has(SanitizerKind::KernelAddress)) ||
----------------
samsonov wrote:
> Wow, that's hard to parse. I'd vote for adding a method to `SanitizerSet`
> if (!Context.getLangOpts().Sanitize.hasOneOf(SanitizerKind::Address,
> SanitizerKind::KernelAddress)
> ...
>
> That would also be useful in several places below.
I've added SanitizerSet::hasAsanOrKasan() -- did you mean that?
================
Comment at: tools/clang/lib/CodeGen/BackendUtil.cpp:211
@@ +210,3 @@
+ PM.add(createAddressSanitizerFunctionPass(/*CompileKernel*/true));
+ PM.add(createAddressSanitizerModulePass(/*CompileKernel*/true));
+}
----------------
samsonov wrote:
> See note above - I'm not sure we need a module pass for that.
Can we keep it provided that we'll have module-level instrumentation soon?
================
Comment at: tools/clang/lib/CodeGen/CodeGenFunction.cpp:619
@@ -619,2 +618,3 @@
+ if (SanOpts.has(SanitizerKind::Address) ||
SanOpts.has(SanitizerKind::KernelAddress))
Fn->addFnAttr(llvm::Attribute::SanitizeAddress);
if (SanOpts.has(SanitizerKind::Thread))
----------------
Note I'm using the same attribute for both userspace and kernel
instrumentation. I think we don't need to distinguish between them.
http://reviews.llvm.org/D10411
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
