Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

union aggregate copies broken in some cases #1793

Closed
llvmbot opened this issue May 15, 2007 · 12 comments
Closed

union aggregate copies broken in some cases #1793

llvmbot opened this issue May 15, 2007 · 12 comments
Assignees
Labels
bugzilla Issues migrated from bugzilla llvm-tools All llvm tools that do not have corresponding tag miscompilation

Comments

@llvmbot
Copy link
Collaborator

llvmbot commented May 15, 2007

Bugzilla Link 1421
Resolution FIXED
Resolved on Feb 22, 2010 12:43
Version 1.0
OS All
Reporter LLVM Bugzilla Contributor

Extended Description

Testcase:

struct A {
char c;
int i;
};

struct B {
char c;
unsigned char x;
};

union U { struct A a; struct B b; };

int check (void) {
union U u, v;

v.b.x = 0xff;
u = v;
return (u.b.x == 0xff);
}

Compiled with -O2:
ret i32 0
Should return 1. The point is that the union takes its fields
from struct A. The second char x in struct B is in the empty
space (due to alignment) in struct A between char c and int i,
so is not copied when CopyAggregate copies by fields. Copying
by fields is completely bogus with respect to unions of structs
unless every field in every union member is covered by an LLVM
field. This cannot be done by converting only one union member,
as is done right now.

@llvmbot
Copy link
Collaborator Author

llvmbot commented May 15, 2007

assigned to @lattner

@llvmbot
Copy link
Collaborator Author

llvmbot commented May 16, 2007

Duncan,

Would it be possible for you to verify whether PR 1419 patch fixes this or not ? Thanks.

@llvmbot
Copy link
Collaborator Author

llvmbot commented May 16, 2007

I don't see how it can, since there is no padding in this case.
I will try it anyway though!

@llvmbot
Copy link
Collaborator Author

llvmbot commented May 16, 2007

The PR 1419 patch makes no difference.

@lattner
Copy link
Collaborator

lattner commented May 16, 2007

This is almost certainly a very long standing bug in CopyAggregate. There is this fixme in
EmitAggregateCopy:

  // FIXME: Is this always safe?  The LLVM type might theoretically have
  // holes or might be suboptimal to copy this way.  It may be better to
  // copy the structure by the GCCType's fields.

-Chris

@lattner
Copy link
Collaborator

lattner commented May 23, 2007

The solution for this bug is to change the behavior of EmitAggregateCopy. Right now, it calls
CopyAggregate if the type is "small", otherwise it uses memcpy.

The predicate should check to see if it is small, AND that the llvm type has fields that overlap all of the
GCC type fields. If this property doesn't hold, we should fall back to memcpy.

-Chris

@llvmbot
Copy link
Collaborator Author

llvmbot commented May 28, 2007

How about to just call "EmitBuiltinMemCopy" when the size is small?

And on EmitBuiltinMemCopy, if the copying size is small enough, just emit iteration of int assignments?

In my knowledge, access time for char and int is same or speed of int is a little faster.

Quho

@lattner
Copy link
Collaborator

lattner commented May 30, 2007

Here is another nice simple example:
http://lists.cs.uiuc.edu/pipermail/llvmdev/2007-May/009205.html

I guess I can take a crack at fixing this.

-Chris

@lattner
Copy link
Collaborator

lattner commented May 30, 2007

This fixes the llvm-gcc part:
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20070528/050081.html

Testcase here: test/CFrontend/2007-05-29-UnionCopy.c

-scalarrepl still needs a fix.

-Chris

@lattner
Copy link
Collaborator

lattner commented May 30, 2007

The bug in llvm-gcc is fixed here:
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20070528/050081.html

The bug in scalarrepl is fixed here:
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20070528/050083.html

Testcases here:
test/CFrontend/2007-05-29-UnionCopy.c
test/Transforms/ScalarRepl/2007-05-29-MemcpyPreserve.ll

-Chris

@lattner
Copy link
Collaborator

lattner commented May 30, 2007

Urg, reduced testcases work, but the full ones don't.

@lattner
Copy link
Collaborator

lattner commented May 30, 2007

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 3, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla llvm-tools All llvm tools that do not have corresponding tag miscompilation
Projects
None yet
Development

No branches or pull requests

2 participants