[PATCH] D116633: Add -fsanitize-address-param-retval to clang.

[Kevin Athey via Phabricator via cfe-commits]

kda added inline comments.


================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:362
+      MemorySanitizerOptions{TrackOrigins, Recover, CompileKernel,
+                             CGOpts.SanitizeMemoryParamRetval != 0}));
 
----------------
vitalybuka wrote:
> we use implicit cast mostly, e.g. addAddressSanitizerPasses
I agree, AND the compiler complains of needing a static_cast<bool>.
Of course, below (line 1167) where the constructor is called explicitly, it works fine.
I think this is a case of 2 implicit casts and hence the compiler balks.

There are 2 other possibilities aside from the static_cast<bool>:
 - assign to a bool, then use the bool
 - construct the options on the stack (like line 1167)

I found my solution to be the most direct, clear and simple.


================
Comment at: clang/test/CodeGen/param-retval-eager-checks.c:1
+// RUN: %clang -target x86_64-linux-gnu -S -emit-llvm -fsanitize=memory -o - %s | \
+// RUN:     FileCheck %s --check-prefix=CLEAN
----------------
vitalybuka wrote:
> vitalybuka wrote:
> > Would you like to remove "| \" ?
> > 80 char limit is not enforced on tests, multi line RUN: is even harder to read then long line
> This dir contains tests for a lot of different componets
> can you please add msan in name
> 
> e.g. "msan-param-retval.c"
> Would you like to remove "| \" ?
> 80 char limit is not enforced on tests, multi line RUN: is even harder to read then long line

No. I broke the lines there as it makes it easier to see the differences, as the flags and the FileCheck are aligned.  (It wasn't about 80 characters.)




================
Comment at: clang/test/CodeGen/param-retval-eager-checks.c:5
+// RUN:     FileCheck %s --check-prefixes=NOUNDEF,NOUNDEF_ONLY
+// RUN: %clang -target x86_64-linux-gnu -S -emit-llvm -fsanitize=memory -Xclang -enable-noundef-analysis -mllvm -msan-eager-checks -o - %s | \
+// RUN:     FileCheck %s --check-prefixes=NOUNDEF,EAGER
----------------
vitalybuka wrote:
> we probably don't need mllvm test here
No, not needed.
And I would like to keep it, as it shows the equivalency between the flags.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116633