[PATCH] D83337: [MSAN] Instrument libatomic load/store calls

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added inline comments.


================
Comment at: llvm/include/llvm/Analysis/TargetLibraryInfo.def:266
+/// void __atomic_load(size_t size, void *mptr, void *vptr, int smodel);
+TLI_DEFINE_ENUM_INTERNAL(atomic_load)
+TLI_DEFINE_STRING_INTERNAL("__atomic_load")
----------------
Can you move TargetLibraryInfo extension into a separate patch


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:3546
+    auto SrcShadowOriginPair =
+        getShadowOriginPtr(SrcPtr, NextIRB, NextIRB.getInt8Ty(), AlignOne,
+                           /*isStore*/ false);
----------------
I'd use ", Align(1), " without temp var


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:3553
+
+    NextIRB.CreateMemCpy(DstShadowPtr, AlignOne, SrcShadowOriginPair.first,
+                         AlignOne, Size);
----------------
DstShadowPtr.first for consistency


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:3544
+    IRBuilder<> NextIRB(CB.getNextNode());
+    Align AlignOne = assumeAligned(1);
+    auto SrcShadowOriginPair =
----------------
guiand wrote:
> Is it valid to assume any alignment other than 1 here? I can't think of a way, but I'd like to make sure.
Can we do MemCpy of shadow here at all? not all bits of shadow bytes correspond to the src/dst


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83337