[PATCH] D115236: [msan] Implement -msan-no-sanitize-all.

[Alexander Potapenko via Phabricator via llvm-commits]

glider added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:311
+static cl::opt<bool>
+    ClNoSanitizeAll("msan-no-sanitize-all",
+                    cl::desc("Apply no_sanitize to the whole file"), cl::Hidden,
----------------
melver wrote:
> glider wrote:
> > vitalybuka wrote:
> > > I don't see value in -all suffix on the flag applied to compilation unit. It's obvious that it's apply to "all"
> > > "msan-no-sanitize-all" -> "msan-no-sanitize"
> > > 
> > > Maybe, but I am not sure: it's also a boolean flag why not just 
> > > ```
> > > static cl::opt<bool>
> > > ClSanitize("msan-sanitize",
> > > ...
> > > cl::init(true));
> > > ```
> > > 
> > > Are you going to add fronted flag? Usually they have -fsanitize-memory- prefix:
> > > -fsanitize-memory-???? Also not sure what this could be.
> > > 
> > > -fsanitize-memory-ignore?
> > > I don't see value in -all suffix on the flag applied to compilation unit. It's obvious that it's apply to "all"
> > > "msan-no-sanitize-all" -> "msan-no-sanitize"
> > 
> > I imagine this being read as "apply no_sanitize to all functions", and "msan-no-sanitize" actually sounds very much like -fnosanitize=memory. Perhaps we need a better name...
> > 
> > > Maybe, but I am not sure: it's also a boolean flag why not just 
> > > ```
> > > static cl::opt<bool>
> > > ClSanitize("msan-sanitize",
> > > ...
> > > cl::init(true));
> > > ```
> > > 
> > > Are you going to add fronted flag? Usually they have -fsanitize-memory- prefix:
> > > -fsanitize-memory-???? Also not sure what this could be.
> > Good question, it didn't come to my mind. Not necessarily needed for KMSAN, but as long as we have the backend flag, might be a good idea.
> > 
> > > -fsanitize-memory-ignore?
> > 
> > -fsanitize-memory-ignore-all-checks, maybe?
> > 
> > (and -mllvm -msan-ignore-all-checks ?)
> Probably, like Vitaly pointed out, the "all" might be redundant.
> 
> The other question is, are checks "ignored", or "disabled".
> 
> Perhaps one of the following:
> ```
> -fsanitize-memory-ignore-checks / -mllvm -msan-ignore-checks
> -fsanitize-memory-disable-checks / -mllvm -msan-disable-checks
> ```
> 
> whichever you find to be clearest.
> 
> Probably, like Vitaly pointed out, the "all" might be redundant.
> 
> The other question is, are checks "ignored", or "disabled".
> 
> Perhaps one of the following:
> ```
> -fsanitize-memory-ignore-checks / -mllvm -msan-ignore-checks
> -fsanitize-memory-disable-checks / -mllvm -msan-disable-checks
> ```
> 
> whichever you find to be clearest.
> 

I think "disable" should be better.
I'll go ahead and rename the -mllvm flag then.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115236