[PATCH] D81699: MemorySanitizer: Add option to insert init checks at call site
Vitaly Buka via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jun 17 14:35:12 PDT 2020
vitalybuka added inline comments.
================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:889
+ static const llvm::StringRef ExpectUninitOperands[] = {
+ "__sanitizer_unaligned_store16", "__sanitizer_unaligned_store32", "__sanitizer_unaligned_store64",
+ };
----------------
this line is to long. please clang-format the patch
I usually do that by:
git clang-format -f --style=file HEAD^
================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1122
// must always return fully initialized values. For now, we hardcode "main".
- CheckReturnValue = SanitizeFunction && (F.getName() == "main");
+ CheckReturnValue = F.getName() == "main";
+ CheckReturnValue &= SanitizeFunction;
----------------
Please undo such irrelevant changes or move them into separate patches
================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1268
+
+ if (StructType *strct = dyn_cast<StructType>(Shadow->getType())) {
+ unsigned i = 0;
----------------
var names are inconsistent
same for i and it
llvm style is to start with uppercase
================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1270
+ unsigned i = 0;
+ for (auto it = strct->element_begin(); it != strct->element_end(); i++, it++) {
+ Value *ShadowItem;
----------------
so if we need integer index, then just iterate with getNumElements() getElementType(unsigned N) ?
range base loop and separate var is fine as well
BTW. the first "i" is 1, is it expected?
================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1271
+ for (auto it = strct->element_begin(); it != strct->element_end(); i++, it++) {
+ Value *ShadowItem;
+ {
----------------
declare and intialize variable at the same statement if possible.
I assume it's to destroy IRB before recursive call
maybe then extract lines in {} into a separate function?
================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1690
+ bool FArgPartialInit = FArg.hasAttribute(Attribute::PartialInit);
+ bool FArgCheck = ClEagerChecks && !FArgByVal && !FArgPartialInit;
unsigned Size =
----------------
FArgCheck -> FArgEagerCheck?
================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1749
+
+ if (!FArgCheck)
+ ArgOffset += alignTo(Size, kShadowTLSAlignment);
----------------
why do we need this?
================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:3442
+ bool PartialInit = CB.paramHasAttr(i, Attribute::PartialInit);
+ bool Check = ClEagerChecks && !ByVal && !PartialInit;
+
----------------
Check -> EagerCheck?
================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:3497
+
+ if (ClEagerChecks && !CB.hasRetAttr(Attribute::PartialInit)) {
+ setShadow(&CB, getCleanShadow(&CB));
----------------
ClEagerChecks && !PartialInit;
================
Comment at: llvm/test/Instrumentation/MemorySanitizer/msan_eager.ll:1
+; RUN: opt < %s -msan-check-access-address=0 -msan-track-origins=1 -msan-eager-checks -S -passes='module(msan-module),function(msan)' 2>&1 | \
+; RUN: FileCheck -allow-deprecated-dag-overlap -check-prefixes=CHECK,CHECK-ORIGINS %s
----------------
would you like to try go generate test with llvm/utils/update_analyze_test_checks.py
================
Comment at: llvm/test/Instrumentation/MemorySanitizer/msan_eager.ll:9-20
+; Check the presence and the linkage type of __msan_track_origins and
+; other interface symbols.
+; CHECK-NOT: @__msan_track_origins
+; CHECK-ORIGINS: @__msan_track_origins = weak_odr constant i32 1
+; CHECK-NOT: @__msan_keep_going = weak_odr constant i32 0
+; CHECK: @__msan_retval_tls = external thread_local(initialexec) global [{{.*}}]
+; CHECK: @__msan_retval_origin_tls = external thread_local(initialexec) global i32
----------------
llvm/utils/update_analyze_test_checks.py will likely drop this, but they are not important for this check
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81699/new/
https://reviews.llvm.org/D81699
More information about the llvm-commits
mailing list