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

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added inline comments.


================
Comment at: clang/lib/CodeGen/CGCall.cpp:2384
+    if ((CodeGenOpts.EnableNoundefAttrs ||
+         CodeGenOpts.SanitizeMemoryParamRetval) &&
+        ArgNoUndef)
----------------
vitalybuka wrote:
> vitalybuka wrote:
> > kda wrote:
> > > 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.
> > > 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.
> > 
> > What is the point of running SanitizeMemoryParamRetval==1 && EnableNoundefAttrs==0?
> > Are we going to show warning or error and ask to set thee redundant options? Then why not just automate this for the user?
> > Also please note that HasStrictReturn already check SanitizerKind::Memory and adjust NoUndef attributes.
> > 
> > On the other hand, if EnableNoundefAttrs is going to be true by default in future, this conversation is not important, we can have two flags.
> > 
> > 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.
> 
> I believe @eugenis point that user should proved both -fsanitize-memory-param-retval and -enable-noundef-analysis to clang (we don't need -mllvm for -enable-noundef-analysis already).
> 
> 
Sorry, I think I misunderstood the code. It makes sense for -fsanitize-memory-param-retval to implicitly enable noundef attrs. I agree with Vitaly that it is better to change the value of EnableNoundefAttrs, to avoid the risk of enabling only part of that feature (now or with some future change).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116633