[PATCH] CloneFunctionInto incorrectly cloning attributes

[Joey Gouly]

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