[PATCH] CloneFunctionInto incorrectly cloning attributes

[Joey Gouly]

Thanks Silviu, r179169!

-----Original Message-----
From: Silviu Baranga 
Sent: 10 April 2013 11:05
To: Joey Gouly; Kristof Beyls; llvm-commits at cs.uiuc.edu
Subject: RE: [PATCH] CloneFunctionInto incorrectly cloning attributes

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
> 
> 
> 
>