llvm-gcc won't build on linux

[Quuxplusone]

Bugzilla Link	PR2629
Status	RESOLVED FIXED
Importance	P normal
Reported by	Nick Lewycky (nicholas@mxc.ca)
Reported on	2008-08-03 09:37:52 -0700
Last modified on	2008-08-07 18:22:29 -0700
Version	unspecified
Hardware	PC Linux
CC	anton@korobeynikov.info, baldrick@free.fr, efriedma@quicinc.com, evan.cheng@apple.com, isanbard@gmail.com, llvm-bugs@lists.llvm.org
Fixed by commit(s)
Attachments	c-common.bad.ll.gz (324161 bytes, application/octet-stream) c-common.good.ll.gz (324048 bytes, application/octet-stream)
Blocks
Blocked by
See also

It's not immediately clear to me whether this is due to a miscompiled xgcc or a
change in the implementation of atomic primitives in Apple's gcc. Either way,
here's the compile failure:

/usr/local/google/home/nlewycky/llvm-gcc/build/./gcc/xgcc -
B/usr/local/google/home/nlewycky/llvm-gcc/build/./gcc/ -
B/usr/local/google/home/nlewycky/llvm-gcc/install/i686-pc-linux-gnu/bin/ -
B/usr/local/google/home/nlewycky/llvm-gcc/install/i686-pc-linux-gnu/lib/ -
isystem /usr/local/google/home/nlewycky/llvm-gcc/install/i686-pc-linux-
gnu/include -isystem /usr/local/google/home/nlewycky/llvm-gcc/install/i686-pc-
linux-gnu/sys-include -DHAVE_CONFIG_H -I. -I../../../src/libgomp -I. -
I../../../src/libgomp/config/linux/x86 -I../../../src/libgomp/config/linux -
I../../../src/libgomp/config/posix -I../../../src/libgomp -Wall -Werror -ftls-
model=initial-exec -march=i486 -pthread -mtune=i686 -O2 -g -O2 -MT alloc.lo -MD
-MP -MF .deps/alloc.Tpo -c ../../../src/libgomp/alloc.c -o alloc.o
In file included from ../../../src/libgomp/libgomp.h:50,
                 from ../../../src/libgomp/alloc.c:32:
../../../src/libgomp/config/linux/sem.h: In function 'gomp_sem_wait':
../../../src/libgomp/config/linux/sem.h:45: error: incompatible type for
argument 1 of '__sync_bool_compare_and_swap'
../../../src/libgomp/config/linux/sem.h: In function 'gomp_sem_post':
../../../src/libgomp/config/linux/sem.h:52: error: incompatible type for
argument 1 of '__sync_bool_compare_and_swap'
In file included from ../../../src/libgomp/libgomp.h:51,
                 from ../../../src/libgomp/alloc.c:32:
../../../src/libgomp/config/linux/mutex.h: In function 'gomp_mutex_lock':
../../../src/libgomp/config/linux/mutex.h:47: error: incompatible type for
argument 1 of '__sync_bool_compare_and_swap'
../../../src/libgomp/config/linux/mutex.h: In function 'gomp_mutex_unlock':
../../../src/libgomp/config/linux/mutex.h:54: error: incompatible type for
argument 1 of '__sync_lock_test_and_set'
make[4]: *** [alloc.lo] Error 1
make[4]: Leaving directory `/usr/local/google/home/nlewycky/llvm-gcc/build/i686-
pc-linux-gnu/libgomp'
make[3]: *** [all-recursive] Error 1
make[3]: Leaving directory `/usr/local/google/home/nlewycky/llvm-gcc/build/i686-
pc-linux-gnu/libgomp'
make[2]: *** [all] Error 2
make[2]: Leaving directory `/usr/local/google/home/nlewycky/llvm-gcc/build/i686-
pc-linux-gnu/libgomp'
make[1]: *** [all-target-libgomp] Error 2
make[1]: Leaving directory `/usr/local/google/home/nlewycky/llvm-gcc/build'
make: *** [all] Error 2

The code it's complaining about looks like this:

extern void gomp_mutex_lock_slow (gomp_mutex_t *mutex);
static inline void gomp_mutex_lock (gomp_mutex_t *mutex)
{
  if (!__sync_bool_compare_and_swap (mutex, 0, 1))
    gomp_mutex_lock_slow (mutex);
}

and those __sync* functions are defined here: http://gcc.gnu.org/onlinedocs/gcc-
4.3.0/gcc/Atomic-Builtins.html

[Quuxplusone]

This is known to be due to a miscompilation of llvm-gcc by itself
(deduced from the fact it only happens when bootstrapping).

[Quuxplusone]

Known bad: LLVM (r54160) + llvm-gcc HEAD (r54312).
Known good: LLVM HEAD (r54312) + llvm-gcc (r54160+54240+54246).

[Quuxplusone]

This exact problem showed up recently. I thought that Mon Ping had a fix for it. At least it was fixed on Darwin...

[Quuxplusone]

Mon Ping observed that llvm-gcc is miscompiling itself,
and found a way of tweaking the miscompiled code so it
is not miscompiled.  That just papers over the problem,
which is that llvm-gcc is miscompiling stuff, so I asked
him to revert his tweak, which he did (I was afraid the
issue would be forgotten otherwise).  People can still
build a working llvm-gcc by not bootstrapping.

[Quuxplusone]

We need to understand whether the bug is in llvm-gcc or llvm proper? Does it build without the recent llvm-gcc changes merged from Apple gcc mainline?

[Quuxplusone]

What was the miscompiled code?

[Quuxplusone]

Take a look at revision 54267, which reverted the "avoid the
miscompile" tweak.

[Quuxplusone]

Okay. I'll take a look and see if I can come up with anything. I'm having
trouble building on the only Linux box I have access to. Maybe you can help?
Here's the error message:

In file included from /usr/include/features.h:354,
                 from /usr/include/stdio.h:28,
                 from ../../llvm-gcc.src/gcc/tsystem.h:90,
                 from ../../llvm-gcc.src/gcc/crtstuff.c:68:
/usr/include/gnu/stubs.h:7:27: error: gnu/stubs-32.h: No such file or directory

The system is 64-bit, AFAIK. I tried everything I knew to get llvm-gcc to build
64-bit, but to no avail.

[Quuxplusone]

Hi Bill,

Is it pure-64 or a mixed 64/32 system? I usually force 64-bitters to configure as 32-bit if they support that:

../src/configure --disable-shared --disable-multilib --enable-checking --enable-llvm=/usr/local/src/llvm --prefix=/usr/local/src/llvm-gcc/install --target=i686-pc-linux-gnu --host=i686-pc-linux-gnu --build=i686-pc-linux-gnu --enable-languages=c,c++

[Quuxplusone]

I don't know, but I think it's a pure-64 bit system. When I got it to compile for 64-bit (using --disable-multilib), I wasn't able to reproduce the error. I'll see if I can find another Linux machine to try it out on, but I'm not hopeful. If you or someone else can reduce this problem, it would help a lot.

[Quuxplusone]

As a sanity check, please try these two tags to see if they build for you on
Linux:

  http://llvm.org/svn/llvm-project/llvm/tags/Apple/llvmCore-2060
  http://llvm.org/svn/llvm-project/llvm-gcc-4.2/tags/Apple/llvmgcc42-2060

These were the last tags before the merges started going in.

[Quuxplusone]

Okay. I got it to fail with TOT. Here's a reduced testcase:

$ cat t.i
int bar(int *sem) {
  return !__sync_bool_compare_and_swap (sem, 1, 0);
}

[Quuxplusone]

Okay. It looks as if c-common.c is being messed up in some way. The stage1-gcc/c-common.o (the one built by GCC) binary works but prev-gcc/c-common.o (the one built by the LLVM-GCC binary) fails. It passes if compiled at -O0, but fails at -O1 and above.

[Quuxplusone]

This transformation in instcombine is causing the problems.

  { // (icmp ugt/ult A, C) & (icmp B, C) --> (icmp (A|B), C)
    // where C is a power of 2
    Value *A, *B;
    ConstantInt *C1, *C2;
    ICmpInst::Predicate LHSCC, RHSCC;
    if (match(&I, m_And(m_ICmp(LHSCC, m_Value(A), m_ConstantInt(C1)),
                        m_ICmp(RHSCC, m_Value(B), m_ConstantInt(C2)))))
      if (C1 == C2 && LHSCC == RHSCC && C1->getValue().isPowerOf2() &&
          (LHSCC == ICmpInst::ICMP_ULT || LHSCC == ICmpInst::ICMP_UGT)) {
        Instruction *NewOr = BinaryOperator::CreateOr(A, B);
        InsertNewInstBefore(NewOr, I);
        return new ICmpInst(LHSCC, NewOr, C1);
      }
  }

I don't think that this is a valid transformation in all cases. Consider the
code that's failing:

  %s1 = sub i8 %x, 6
  %t1 = icmp ugt i8 %s1, 2
  %s2 = sub i8 %x, 10
  %t2 = icmp ugt i8 %s2, 2
...
  %r1 = and i1 %t1, %t2

is transformed into this:

  %s1 = sub i8 %x, 6
  %t1 = icmp ugt i8 %s1, 2
  %s2 = sub i8 %x, 10
  %t2 = icmp ugt i8 %s2, 2
...
  %r1 = or i8 %s1, %s2
  ... = icmp ugt i8 %r1, 2

Or, in human readable format:

  BOTH x - 6 > 2 AND x - 10 > 2

is transformed into

  EITHER x - 6 > 2 OR x - 10 > 2

There's a major semantic difference between the two.

[Quuxplusone]

To be more concrete. Make x == 11 in the equations. Without this
transformation, we get:

  x - 6 = 5 > 2 is true
  x - 10 = 1 > 2 is false
  result true & false = false

With the transform:

  x - 6 = 5
  x - 10 = 1
  5 | 1 = 5 > 2
  result true

[Quuxplusone]

Reverted this patch:

http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20080707/064740.html

Nick, Could you please revise your patch? I'm going to add the .bc files from c-common. The code that is broken in these files by your patch is in bb41 of @resolve_overloaded_builtin.

[Quuxplusone]

Attached c-common.bad.ll.gz (324161 bytes, application/octet-stream): File after instcombine is run on it.

[Quuxplusone]

Attached c-common.good.ll.gz (324048 bytes, application/octet-stream): File before instcombine is run on it.

[Quuxplusone]

Added testcase test/Transforms/InstCombine/2008-08-05-And.ll

[Quuxplusone]

Thanks for tracking this down! I think it works with a|b and u<, but I'll make sure I get it right next time.

[Quuxplusone]

What do people think of:

// (icmp ult A, C) & (icmp ult B, C) --> (icmp (A|B), C)
// (icmp ugt A, C) | (icmp ugt B, C) --> (icmp (A|B), C)
// where C is a power of 2

?

I'm quite sure it's right, and I'm running bootstrap + nightly to check double-
before committing.

[Quuxplusone]

(In reply to comment #21)
> What do people think of:
>
> // (icmp ult A, C) & (icmp ult B, C) --> (icmp (A|B), C)

Looks fine to me.

> // (icmp ugt A, C) | (icmp ugt B, C) --> (icmp (A|B), C)

Take A > 2 | B > 2, and suppose A = 2 and B = 1.  The original statement is
false, but (A|B) > 2 is true.

[Quuxplusone]

(In reply to comment #21)
> // (icmp ult A, C) & (icmp ult B, C) --> (icmp (A|B), C)

This seems logically correct, at least. :-)

> // (icmp ugt A, C) | (icmp ugt B, C) --> (icmp (A|B), C)

Eli pointed out the flaw in this one.

[Quuxplusone]

Hah! Okay, I give up. I'm going to commit the ult part and forget about dealing with ugt.