[PATCH] D98734: [dfsan] Add -dfsan-fast-8-labels flag

[stephan.yichao.zhao via Phabricator via llvm-commits]

stephan.yichao.zhao added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:40-41
+///
+/// With fast-8 mode, each byte of application memory is backed by a single
+/// shadow memory byte. On Linux/x86_64, the memory layout becomes:
+///
----------------
If users read top to down, it seems helpful if we move this sentence above the first layout. 
I suggest we first explain there are two kinds of shadow layouts.  One uses 2 bytes bit for fast-16-labels mode with 16 labels and legacy mode with 2^16 labels, and the other uses 1 byte for fast-8-labels mode with 8 labels. Then we show the two layouts.


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:56
+/// +--------------------+ 0x100000008000 (kShadowAddr)
+/// | reserved by kernel |
+/// +--------------------+ 0x000000000000
----------------
Addresses below 0x000000010000 is for kernel usage.
We can mark unused from 0x000000010000 to 0x100000008000.


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:2050
+
+  // The shadow size in bytes is a multiple of 8, except for the case of load32
+  // in fast-8 mode when it's 4. For the latter case, we can fit the wide shadow
----------------
A comment with more context could be
"LLVM bitcode supports loading integers with byte size 1, 12, 20....
But loadFast16ShadowFast only optimizes normal cases loading i32 i64 i128 etc.. 
so it is fine to support only size=4 or size % 8 == 0. "


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:2105
   // Fast path for the common case where each byte has identical shadow: load
   // shadow 64 bits at a time, fall out to a __dfsan_union_load call if any
   // shadow is non-equal.
----------------
64 -> 64 or 32


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:746
     const std::vector<std::string> &ABIListFiles) {
+  if (ClFast8Labels && ClFast16Labels) {
+    report_fatal_error(
----------------
gbalats wrote:
> stephan.yichao.zhao wrote:
> > When releasing fast8labels, we set ClFast8Labels = true by default.
> > If users wanted to use 16bit mode by -dfsan-fast-16-labels=true, this assert happens. So they have to do -dfsan-fast-16-labels=true -dfsan-fast-8-labels=false.
> > This message can suggest this if we found both are true. Something like:
> > "cannot set both -dfsan-fast-8-labels and -dfsan-fast-16-labels. -dfsan-fast-8-labels is true by default. 
> > 16mode will be deprecated. To use 16 mode, set -dfsan-fast-16-labels=true -dfsan-fast-8-labels=false."
> > 
> > Or when -dfsan-fast-16-labels is true, or we found -dfsan-fast-8-labels=false, this code can print out the deprecation warning.
> I'm not sure I follow. Did you mean ClFast16Labels perhaps, since the ClFast8Labels has just been introduced by this path? Where is it set? The default value of both flags is false, so the statement "-dfsan-fast-*-labels is true by default" isn't accurate.
> 
> I think the deprecation warning should be introduced after fast8 is properly supported by the dfsan runtime.
thought about this again. Since fast-8-labels is not fully supported yet, and set to false by default, we do not have the issue for now.
When we set its default to true, this message could be updated accordingly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98734