[PATCH] CloneFunctionInto incorrectly cloning attributes

[Silviu Baranga]

Hi Joey,

> The comment was incorrect, in the sense that the arguments could be
> mapped
> elsewhere, not just deleted.

Ok, this makes perfect sense. LGTM.

Cheers,
Silviu



> -----Original Message-----
> From: Joey Gouly
> Sent: 09 April 2013 15:25
> To: Silviu Baranga; Kristof Beyls; llvm-commits at cs.uiuc.edu
> Subject: RE: [PATCH] CloneFunctionInto incorrectly cloning attributes
>
> Thanks Silviu.
>
> > How would we end up in the situation where the number of arguments is
> the
> > same, but they are of different types? The old comment was:
> > -    //Some arguments were deleted with the VMap. Copy arguments one
> by
> one
> > Was the comment incorrect?
>
> If you're cloning function A into B, you could map A's arguments into
> (for
> example)
> alloca's inside B. However, before this patch, this mapping would be
> ignored
> if A and B
> had the same number of arguments and A's argument attributes would be
> copied
> over onto B's arguments.
> Don't think of it is changing the types, just think of it as cloning
> into an
> unrelated function that
> happens to have the same number of arguments!
>
> The comment was incorrect, in the sense that the arguments could be
> mapped
> elsewhere, not just deleted.
>
> > You should keep the existing indentation style:
> > -    NewFunc->setAttributes(NewFunc->getAttributes()
> > -                           .addAttributes(NewFunc->getContext(),
> > -                                          AttributeSet::ReturnIndex,
> > -                                          OldFunc-
> >getAttributes()));
> > -    NewFunc->setAttributes(NewFunc->getAttributes()
> > -                           .addAttributes(NewFunc->getContext(),
> > -
> AttributeSet::FunctionIndex,
> > -                                          OldFunc-
> >getAttributes()));
> >
> > seems more readable than
> >
> > +  NewFunc->setAttributes(NewFunc->getAttributes().addAttributes(
> > +      NewFunc->getContext(), AttributeSet::ReturnIndex,
> > +      OldFunc->getAttributes().getRetAttributes()));
> > +  NewFunc->setAttributes(NewFunc->getAttributes().addAttributes(
> > +      NewFunc->getContext(), AttributeSet::FunctionIndex,
> > +      OldFunc->getAttributes().getFnAttributes()));
>
> I used that clang-format tool, but I agree it's more readable, I'll
> change
> this back.
> I'll also bring it up with Manuel and Daniel, see if they want to fix
> the
> formatter for this.
>
> Thanks,
> Joey
>
> > Cheers,
> > Silviu
>
> > -----Original Message-----
> > From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits-
> > bounces at cs.uiuc.edu] On Behalf Of Joey Gouly
> > Sent: 09 April 2013 12:17
> > To: Joey Gouly; Kristof Beyls; llvm-commits at cs.uiuc.edu
> > Subject: RE: [PATCH] CloneFunctionInto incorrectly cloning attributes
> >
> > Ping.
> >
> > OK to commit?
> >
> > Joey
> >
> > -----Original Message-----
> > From: llvm-commits-bounces at cs.uiuc.edu
> > [mailto:llvm-commits-bounces at cs.uiuc.edu] On Behalf Of Joey Gouly
> > Sent: 04 April 2013 12:19
> > To: Kristof Beyls; llvm-commits at cs.uiuc.edu
> > Subject: RE: [PATCH] CloneFunctionInto incorrectly cloning attributes
> >
> > Hi Kristof,
> >
> > A good idea, I reattached the patch with an added unit test.
> >
> > Thanks,
> > Joey
> >
> > -----Original Message-----
> > From: Kristof Beyls
> > Sent: 04 April 2013 10:27
> > To: Joey Gouly; llvm-commits at cs.uiuc.edu
> > Subject: RE: [PATCH] CloneFunctionInto incorrectly cloning attributes
> >
> > Hi Joey,
> >
> > If possible, I think the patch could use a regression test.
> >
> > Thanks,
> > Kristof
> >
> > > -----Original Message-----
> > > From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits-
> > > bounces at cs.uiuc.edu] On Behalf Of Joey Gouly
> > > Sent: 04 April 2013 10:10
> > > To: llvm-commits at cs.uiuc.edu
> > > Subject: [PATCH] CloneFunctionInto incorrectly cloning attributes
> > >
> > > Hi all,
> > >
> > > Attached is a patch to fix a bug in llvm::CloneFunctionInto.
> > >
> > > Firstly, the amount of arguments are compared, and if they are
> equal
> > the
> > > attributes are copied over directly.
> > > This is wrong if you are cloning into a function with the same
> number
> > of
> > > arguments, but if they are not actually
> > > the same arguments.
> > >
> > > After the first change, the attributes were still being cloned
> > incorrectly.
> > > So the second change is using
> > > get(Ret/Fn)Attributes() instead of getAttributes(). This may not be
> > the
> > > right way to fix this, I'm hoping
> > > Brian Wendling can say if this is a bug here, or in the Attributes
> > API.
> > >
> > > Example of how these bugs happen:
> > >
> > > define void @x(i32* nocapture %a, i32 %b) {
> > >   ret void
> > > }
> > >
> > > declare void @y(i64 %a, i64 %b)
> > >
> > > Without my changes, after CloneFunctionInto(@y, @x):
> > >
> > > define void @x(i32* nocapture %a, i32 %b) {
> > >   ret void
> > > }
> > >
> > > define void @y(i64 nocapture %c, i64 %d) {
> > >   ret void
> > > }
> > >
> > > Which is wrong, as it has put the nocapture attribute on %c,
> because
> > they
> > > have the same number of arguments.
> > >
> > > Any comments? Good to commit?
> > >
> > > Thanks,
> > > Joey
> >
> >
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>
>


-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium.  Thank you.