[PATCH] CloneFunctionInto incorrectly cloning attributes

[Silviu Baranga]

Hi Joey,

The patch looks good, but I have some minor questions.

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?

The get(Ret/Fn)Attributes() change seems sane to me.

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()));

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