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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
