nlopes added inline comments.
================
Comment at: clang/lib/CodeGen/CGCall.cpp:2535
ArgAttrs[FirstIRArg + i] =
llvm::AttributeSet::get(getLLVMContext(), Attrs);
}
----------------
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..
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D105169/new/
https://reviews.llvm.org/D105169
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
