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