[PATCH] D100967: [dfsan] Track origin at loads

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

stephan.yichao.zhao added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:256-262
 // Controls how to track origins.
 // * 0: do not track origins.
 // * 1: track origins at memory store operations.
-// * 2: TODO: track origins at memory store operations and callsites.
+// * 2: track origins at memory load and store operations.
 static cl::opt<int> ClTrackOrigins("dfsan-track-origins",
                                    cl::desc("Track origins of labels"),
                                    cl::Hidden, cl::init(0));
----------------
gbalats wrote:
> Using different integer values to encode the level of tracking is hard to understand without looking at this exact comment right here. Why can't we use an enum instead with descriptive names?
> 
> E.g.,
> 
> 
> ```
> enum OriginTrackingLevel {
>   None,
>   StoresOnly,
>   LoadsAndStores
> };
> ```
> 
> https://llvm.org/docs/CommandLine.html#selecting-an-alternative-from-a-set-of-possibilities
Thank you. 

This enum is helpful. It uses optimization levels as an example.
If we considered our use case as some debug levels, it would work
1) track the basic traces
2) track more chains to get more details but slow. In the next step, maybe callsites can also be added into this option. I updated the comments to mention this.

Changing this from int to enum will change existing test cases. if we wanted it to use enum, I prefer to doing so in a different CL.

MSan's msan-track-origins is defined like dfsan-track-origins, with 0, 1 and 2 to control different levels.
At least they are consistent for the time being.


================
Comment at: llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:644
+  /// Generates IR to load shadow and origin corresponding to bytes [Addr,
+  /// Addr+Size), where Addr has alignment Align, and take the union of each of
+  /// those shadows. The returned shadow always has primitive type.
----------------
gbalats wrote:
> You mean InstAlignment? By the way, for documenting specific arguments of functions, I think the special `\param` Doxygen syntax would be clearer.
> 
> https://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments
Thank you,


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100967