[PATCH] D92440: [dfsan] Support passing non-i16 shadow values in TLS mode

[Matt Morehouse via Phabricator via llvm-commits]

morehouse added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:1074
+//
+// Shadow = ArgTLS+ArgOffset.
+Value *DFSanFunction::getArgTLS(unsigned ArgOffset, IRBuilder<> &IRB) {
----------------
This is a duplicate of the declaration comment.  Let's remove this one.  (Same for functions below).


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:1096
+        break;
+      }
+      continue;
----------------
Nit:  omit curly braces for one-line bodies.  (Same elsewhere)


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:1104
+      if (ArgOffset > kArgTLSSize) {
+        // ArgTLS overflows, uses a zero shadow.
+        break;
----------------
Nit: This comment can fit on the same line as the break. (Same below)


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:1117
+    IRBuilder<> IRB(ArgTLSPos);
+    Value *Base = getArgTLS(ArgOffset, IRB);
+    return IRB.CreateAlignedLoad(DFS.ShadowTy, Base, kShadowTLSAlignment);
----------------
Nit:  `Base` is a confusing name since it sounds like the base pointer to the TLS, not the offset.  Let's rename it something else, maybe `ArgShadowPtr`?


================
Comment at: llvm/test/Instrumentation/DataFlowSanitizer/arith.ll:7
   ; CHECK: @"dfs$add"
-  ; CHECK-DAG: %[[ALABEL:.*]] = load{{.*}}__dfsan_arg_tls, i64 0, i64 0
-  ; CHECK-DAG: %[[BLABEL:.*]] = load{{.*}}__dfsan_arg_tls, i64 0, i64 1
-  ; CHECK: %[[UNION:.*]] = call{{.*}}__dfsan_union(i16 zeroext %[[ALABEL]], i16 zeroext %[[BLABEL]])
-  ; CHECK: %[[ADDLABEL:.*]] = phi i16 [ %[[UNION]], {{.*}} ], [ %[[ALABEL]], {{.*}} ]
+  ; CHECK-DAG: %[[ALABEL:.*]] = load [[ST:.*]], [[ST]]* bitcast ([[VT:\[.*\]]]* @__dfsan_arg_tls to [[ST]]*), align [[ALIGN:.*]]
+  ; CHECK-DAG: %[[BLABEL:.*]] = load [[ST]], [[ST]]* inttoptr (i64 add (i64 ptrtoint ([[VT]]* @__dfsan_arg_tls to i64), i64 8) to [[ST]]*), align [[ALIGN]]
----------------
Should we verify that align is 8?


================
Comment at: llvm/test/Instrumentation/DataFlowSanitizer/arith.ll:8
+  ; CHECK-DAG: %[[ALABEL:.*]] = load [[ST:.*]], [[ST]]* bitcast ([[VT:\[.*\]]]* @__dfsan_arg_tls to [[ST]]*), align [[ALIGN:.*]]
+  ; CHECK-DAG: %[[BLABEL:.*]] = load [[ST]], [[ST]]* inttoptr (i64 add (i64 ptrtoint ([[VT]]* @__dfsan_arg_tls to i64), i64 8) to [[ST]]*), align [[ALIGN]]
+  ; CHECK: %[[UNION:.*]] = call zeroext [[ST]] @__dfsan_union([[ST]] zeroext %[[ALABEL]], [[ST]] zeroext %[[BLABEL]])
----------------
IIUC, this adds 8 to `__dfsan_arg_tls` to get `b`'s shadow.  This is because we have 8-byte alignment, right?

Why do we need 8 byte alignment?


================
Comment at: llvm/test/Instrumentation/DataFlowSanitizer/arith.ll:12
   ; CHECK: add i8
-  ; CHECK: store i16 %[[ADDLABEL]], i16* @__dfsan_retval_tls
+  ; CHECK: store [[ST]] %[[ADDLABEL]], [[ST]]* bitcast ([[VT]]* @__dfsan_retval_tls to [[ST]]*), align [[ALIGN]]
   ; CHECK: ret i8
----------------
The `ST` and `VT` names are a little confusing to me.  What do they mean?


================
Comment at: llvm/test/Instrumentation/DataFlowSanitizer/select.ll:9
+  ; TRACK_CONTROL_FLOW: %2 = load i16, {{.*}}@__dfsan_arg_tls{{.*}}
+  ; TRACK_CONTROL_FLOW: %3 = load i16, {{.*}}@__dfsan_arg_tls{{.*}}
   ; TRACK_CONTROL_FLOW: %4 = select i1 %c, i16 %2, i16 %1
----------------
Should we verify offsets in the TLS for each argument?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92440