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

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka added a comment.

we probably should to move clang parts from D116634 <https://reviews.llvm.org/D116634> into this patch?



================
Comment at: clang/include/clang/Basic/CodeGenOptions.def:234
                                              ///< destructors are emitted.
+CODEGENOPT(SanitizeMemoryParamRetval, 1, 0) ///<p Eager param-retval uninitialized use detection
+                                             ///< in MemorySanitizer
----------------
Same comment here?


================
Comment at: clang/lib/CodeGen/CGCall.cpp:2384
+    if ((CodeGenOpts.EnableNoundefAttrs ||
+         CodeGenOpts.SanitizeMemoryParamRetval) &&
+        ArgNoUndef)
----------------
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.



================
Comment at: clang/lib/CodeGen/CGCall.cpp:2384
+    if ((CodeGenOpts.EnableNoundefAttrs ||
+         CodeGenOpts.SanitizeMemoryParamRetval) &&
+        ArgNoUndef)
----------------
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).




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116633