[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added inline comments.


================
Comment at: clang/lib/CodeGen/CGCall.cpp:2535
         ArgAttrs[FirstIRArg + i] =
             llvm::AttributeSet::get(getLLVMContext(), Attrs);
     }
----------------
sammccall wrote:
> fhahn wrote:
> > nlopes wrote:
> > > fhahn wrote:
> > > > nlopes wrote:
> > > > > ab wrote:
> > > > > > Hmm, if I'm reading this right, this overwrites the `nonnull dereferenceable align` attributes separately computed for `this` around l2335, right? (or `inalloca` and `sret` before that)
> > > > > > 
> > > > > > It sounds like an ancient bug, that's only exposed because `noundef` ends up triggering this logic much more often?
> > > > > > 
> > > > > > Many of our downstream tests hit this. The hacked up patch seems to do the job; ideally we'd feed the previously-computed attrs when constructing the AttrBuilder (which would also fix the padding argument), but we'd need to match up the IR args first.  Maybe that's fine as a special-case for arg 0 though
> > > > > nice catch! It's an ancient bug where arg 0 is overwritten.
> > > > Is anybody looking into a fix or should we revert the patch until this can be properly addressed?
> > > I would recommend against reverting this patch because of all the followup patches. There were quite a few commits afterwards plus fixes to buildbot configurations, so it's a non-trivial overhead to revert everything.
> > > I was assuming @ab would fix it as he already root-caused the bug..
> > FWIW it seems a bit unfortunate that there are some clear regressions when the tests got update, e.g. https://github.com/llvm/llvm-project/commit/1b1c8d83d3567a60280291c0adb95d1d60335509#diff-7e80d593f26f6f6fb24765c6a169884d7350685d565ee970b0a7b9abaf0fb205L355
> > 
> > I'll work with @ab to fix this though rather than reverting, because another revert would cause even more conflicts for us downstream.
> > FWIW it seems a bit unfortunate that there are some clear regressions when the tests got update, e.g. https://github.com/llvm/llvm-project/commit/1b1c8d83d3567a60280291c0adb95d1d60335509#diff-7e80d593f26f6f6fb24765c6a169884d7350685d565ee970b0a7b9abaf0fb205L355
> > 
> > I'll work with @ab to fix this though rather than reverting, because another revert would cause even more conflicts for us downstream.
> 
> Just a reminder that the 14 release is cut soon (1 feb: https://llvm.discourse.group/t/llvm-14-0-0-release-schedule/5846)
> 
> I don't know this code; if a clean fix is ready soon and unlikely to have a ripple effect then great. But it does seem risky to be treating such configuration changes as "too big to fail" at this point in the release cycle.
Thank you Florian!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105169