LLVM Bugzilla is read-only and represents the historical archive of all LLVM issues filled before November 26, 2021. Use github to submit LLVM bugs

Bug 2680 - apparent union bug
Summary: apparent union bug
Status: RESOLVED FIXED
Alias: None
Product: new-bugs
Classification: Unclassified
Component: new bugs (show other bugs)
Version: unspecified
Hardware: PC Linux
: P normal
Assignee: Unassigned LLVM Bugs
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-08-14 21:44 PDT by John Regehr
Modified: 2008-08-16 23:27 PDT (History)
3 users (show)

See Also:
Fixed By Commit(s):


Attachments
x86 asm (1.25 KB, application/octet-stream)
2008-08-14 22:28 PDT, John Regehr
Details
llvm asm (2.10 KB, application/octet-stream)
2008-08-14 22:28 PDT, John Regehr
Details
proposed patch (977 bytes, patch)
2008-08-15 15:56 PDT, Dale Johannesen
Details

Note You need to log in before you can comment on or make changes to this bug.
Description John Regehr 2008-08-14 21:44:05 PDT
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;
}
Comment 1 Dale Johannesen 2008-08-14 22:07:05 PDT
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?


Comment 2 John Regehr 2008-08-14 22:28:31 PDT
Created attachment 1926 [details]
x86 asm
Comment 3 John Regehr 2008-08-14 22:28:45 PDT
Created attachment 1927 [details]
llvm asm
Comment 4 John Regehr 2008-08-14 22:29:17 PDT
Yes my llvm-gcc is from today.  Files attached, hope this helps.
Comment 5 Dale Johannesen 2008-08-14 23:59:45 PDT
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.
Comment 6 Duncan Sands 2008-08-15 02:42:06 PDT
So is this PR invalid?
Comment 7 Dale Johannesen 2008-08-15 12:03:57 PDT
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.

Comment 8 Duncan Sands 2008-08-15 13:03:50 PDT
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].
Comment 9 Dale Johannesen 2008-08-15 15:56:37 PDT
Created attachment 1935 [details]
proposed patch

John, could you see if this patch fixes it?
Comment 10 Dale Johannesen 2008-08-15 16:20:58 PDT
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.

Comment 11 John Regehr 2008-08-15 23:12:02 PDT
> John, could you see if this patch fixes it?

Yep, looks good.  I'll do more extensive testing over the weekend.
Comment 12 Dale Johannesen 2008-08-16 23:27:41 PDT
Should be fixed here (somewhat more restrictive patch).
http://llvm.org/viewvc/llvm-project?view=rev&revision=54867