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

apparent union bug #3052

Closed
regehr opened this issue Aug 15, 2008 · 12 comments
Closed

apparent union bug #3052

regehr opened this issue Aug 15, 2008 · 12 comments
Labels
bugzilla Issues migrated from bugzilla

Comments

@regehr
Copy link
Contributor

regehr commented Aug 15, 2008

Bugzilla Link 2680
Resolution FIXED
Resolved on Aug 16, 2008 23:27
Version unspecified
OS Linux

Extended Description

This is seen on r54780 on Ubuntu Feisty on ia32. Removing any field of cjt5 makes the problem go away.

[regehr@babel tmp8]$ llvm-gcc -O -Wall small.c -o small ; ./small
small: small.c:26: main: Assertion `cj41.cj35 == cj43.cj35' failed.
Aborted (core dumped)

[regehr@babel tmp8]$ cat small.c
#include <assert.h>

union cjt7
{
double *cj35;
struct cjt5 {
float cj27;
signed int cj29:7;
signed int cj30:7;
signed int cj31:5;
} cj37;
};

union cjt7 cj41 = {
(double *) 2140832546U,
};

union cjt7 callee_cj0f(void)
{
return cj41;
}

int main(void)
{
union cjt7 cj43 = callee_cj0f();
assert(cj41.cj35 == cj43.cj35);
return 0;
}

@llvmbot
Copy link
Collaborator

llvmbot commented Aug 15, 2008

This works for me on x86-32 Darwin. It could be related to 54503. Is your llvm-gcc up to date as well as llvm? If it is, could you post the assembly code here?

@regehr
Copy link
Contributor Author

regehr commented Aug 15, 2008

x86 asm

@regehr
Copy link
Contributor Author

regehr commented Aug 15, 2008

llvm asm

@regehr
Copy link
Contributor Author

regehr commented Aug 15, 2008

Yes my llvm-gcc is from today. Files attached, hope this helps.

@llvmbot
Copy link
Collaborator

llvmbot commented Aug 15, 2008

This is a cute one; I suppose the good news is that this has nothing to do with 54503. The code is copying 2140832546U == 0x7f9a8322 by
flds _cj41
fstps (%eax)
movl _cj41+4, %ecx
movl %ecx, 4(%eax)
(it's this way in the IR). Unfortunately 0x7f9a considered as single-precision is SNaN, and the hardware helpfully changes it to QNaN. Gotta love copy instructions that don't copy. Bottom line, 387 FP copies can't be relied on to do bitwise copies.

@llvmbot
Copy link
Collaborator

llvmbot commented Aug 15, 2008

So is this PR invalid?

@llvmbot
Copy link
Collaborator

llvmbot commented Aug 15, 2008

Not at all. The active element of the union at the time it's copied is a pointer, and we therefore can't use floating point operations to copy it (at least on x87). It is probably an easy fix in the llvm-gcc FE to do a bitwise copy (memcpy) instead in this case (returning a value). However, I'm concerned this might be a symptom of a more widespread problem. The IR represents this type as
%struct.cjt5 = type { float, i32 }
%struct.cjt7 = type { %struct.cjt5 }
so it's likely there's other code around that thinks it is OK to use floating point operations on it, IMO. The IR has lost the information about the overlapping pointer field. Perhaps we should add a was-union or must-be-copied-bitwise flag.

@llvmbot
Copy link
Collaborator

llvmbot commented Aug 15, 2008

Thoughts: (1) if I bitcast a float* to an int*, then store to it,
the optimizers should not turn this into a float write unless they
can prove that the value corresponds to an ordinary float value.
Might be worth testing that the optimizers handle this right.
(2) I don't remember how C bitfields work, but doesn't struct cjt5
have holes in it? So how come it is being used to do the copy and
not the double*?
(3) In the case of
union U { float a; int b; }
what do the C language rules say about copying U? Is it allowed
to be copied by copying the float? [If so, that means the standard
allows undefined behaviour if U holds a funky float value].

@llvmbot
Copy link
Collaborator

llvmbot commented Aug 15, 2008

proposed patch
John, could you see if this patch fixes it?

@llvmbot
Copy link
Collaborator

llvmbot commented Aug 15, 2008

Re (2), the C struct cjt5 has holes in it; the LLVM IR type of that name does not. I don't know how the converter constructs a type to represent a union (except that it must be the same size and alignment as the real union).
Re (3), I don't think copying is supposed to change the value; using fld/fstp is wrong for that union.
But of course the as-if rule comes into play, and a QNaN will generally behave the same as an SNaN on targets that don't actually trap on SNaN (most of them). So using fld/fstp will generally be OK for struct fields, or when the active field of your union object is actually the FP object, not some overlapping int object. In general the compiler won't be able to tell that's the case, so it should not use fld/fstp.

@regehr
Copy link
Contributor Author

regehr commented Aug 16, 2008

John, could you see if this patch fixes it?

Yep, looks good. I'll do more extensive testing over the weekend.

@llvmbot
Copy link
Collaborator

llvmbot commented Aug 17, 2008

Should be fixed here (somewhat more restrictive patch).
http://llvm.org/viewvc/llvm-project?view=rev&revision=54867

@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
Projects
None yet
Development

No branches or pull requests

2 participants