[PATCH] D116634: enable msan-eager-checks with -fsanitize-memory-param-retval.

[Kevin Athey via Phabricator via cfe-commits]

kda added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Instrumentation/MemorySanitizer.h:28
   bool Recover;
+  bool EagerChecks;
 };
----------------
vitalybuka wrote:
> maybe we should use CheckParamRetVal something here?
> 
How about EagerChecksRequested?


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:496
         Recover(Options.Recover) {
+    if (Options.EagerChecks) {
+      ClEagerChecks = Options.EagerChecks;
----------------
vitalybuka wrote:
> mllvm is lower level flag, so it should be opposite
> -mllvm flag should override MemorySanitizerOptions
> 
> with getOptOrDefault below you don't need anything here
I disagree.  The variable ClEagerChecks is referenced in the code.  If it is not configured based on EagerChecksRequested, there is no way to change the behavior based on the presence of the flag.

I will admit that with the change below, it is a bit circular, because Options.EagerChecksRequested prefers the -msan-eager-checks flag, but may default to EagerChecksRequested parameter, and then here the flag variable (ClEagerChecks) is set based on Options.EagerChecksRequested.

An alternative is to have a member variable of MemorySanitizer that is consulted in the code.  It is configured based on Options.EagerChecksRequested.  The flag (-msan-eager-checks) is only consulted below with the call to getOptOrDefault.  (I coded this up: https://reviews.llvm.org/D116855)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116634