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

[George Balatsouras via Phabricator via llvm-commits]

gbalats 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));
----------------
stephan.yichao.zhao wrote:
> 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.
Addressing this in separate change SGTM. Thanks!


================
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.
----------------
stephan.yichao.zhao wrote:
> 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,
I'm not entirely sure you can use `\param` inlined within the text. Or, if you have to add a separate line per parameter. Most cases I see do the former.
Please check.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100967