[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/CGCall.cpp:2384
+    if ((CodeGenOpts.EnableNoundefAttrs ||
+         CodeGenOpts.SanitizeMemoryParamRetval) &&
+        ArgNoUndef)
----------------
eugenis wrote:
> vitalybuka wrote:
> > vitalybuka wrote:
> > > @eugenis Would be better to force CodeGenOpts.EnableNoundefAttrs if SanitizeMemoryParamRetval is provided?
> > > @eugenis Would be better to force CodeGenOpts.EnableNoundefAttrs if SanitizeMemoryParamRetval is provided?
> > 
> > To clarify, checking SanitizeMemoryParamRetval here as-is is LGTM, unless @eugenis or someone else thinks EnableNoundefAttrs reset is better.
> I don't think this is right at all. An argument being noundef has nothing to do at all with memory sanitization. It affects optimization, too. SanitizeMemoryParamRetval should only affect what MemorySanitizerPass does with noundef attributes.
Is the problem the form of the new code?
  - instead of introducing a single new flag to flip 4 others, should there be 1 flag to pick up the CodeGen changes, and another for the Sanitizer?  (Is 2 flags better than 4?)
  - another option is have the changes propagate in the other direction: use the flag (-fsanitize-memory-param-retval), to passes along '-mllvm -enable_noundef_analysis=1' to CodeGen (via SanitizerArgs.addArgs).

OR is there a problem forcing EnableNoundefAttrs based on an "unrelated" flag?
  - then leave existing code, just don't do anything fancy to change EnableNoundefAttrs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116633