[PATCH] CloneFunctionInto incorrectly cloning attributes

[Joey Gouly]

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