[llvm-commits] [llvm-gcc-4.2] r54240 - /llvm-gcc-4.2/trunk/gcc/llvm-convert.cpp

[Matthijs Kooijman]

Hi,

> You just used Ty[0] uninitialized.
This bug shows up in the llvm-gcc build, giving:

/home/kooijman/src/llvm-gcc/obj/./gcc/xgcc -B/home/kooijman/src/llvm-gcc/obj/./gcc/ -B/home/kooijman/src/llvm-gcc/obj/../install/i686-pc-linux-gnu/bin/ -B/home/kooijman/src/llvm-gcc/obj/../install/i686-pc-linux-gnu/lib/ -isystem /home/kooijman/src/llvm-gcc/obj/../install/i686-pc-linux-gnu/include -isystem /home/kooijman/src/llvm-gcc/obj/../install/i686-pc-linux-gnu/sys-include -DHAVE_CONFIG_H -I. -I../../../llvm-gcc-4.2-trunk/libgomp -I. -I../../../llvm-gcc-4.2-trunk/libgomp/config/linux/x86 -I../../../llvm-gcc-4.2-trunk/libgomp/config/linux -I../../../llvm-gcc-4.2-trunk/libgomp/config/posix -I../../../llvm-gcc-4.2-trunk/libgomp -Wall -Werror -ftls-model=initial-exec -march=i486 -pthread -mtune=i686 -O2 -g -O2 -MT critical.lo -MD -MP -MF .deps/critical.Tpo -c ../../../llvm-gcc-4.2-trunk/libgomp/critical.c  -fPIC -DPIC -o .libs/critical.o
cc1: Constants.cpp:1854: static llvm::Constant* llvm::ConstantExpr::getIntegerCast(llvm::Constant*, const llvm::Type*, bool): Assertion `C->getType()->isInteger() && Ty->isInteger() && "Invalid cast"' failed.
../../../llvm-gcc-4.2-trunk/libgomp/critical.c: In function 'GOMP_critical_name_start':
../../../llvm-gcc-4.2-trunk/libgomp/critical.c:54: internal compiler error: Aborted

(backtrace from gdb shows that Ty is indeed unitialized)

The fix seems simply, from looking at the other similar pieces of code this should be:

>      const Type *OrigTy = cast<PointerType>(C[0]->getType())->getElementType();
> -    const Type* Ty = OrigTy;
> -    if (isa<PointerType>(Ty)) 
> -      Ty = TD.getIntPtrType();
> -
> -    C[0] = Builder.CreateBitCast(C[0], PointerType::getUnqual(Ty));
> -    C[1] = Builder.CreateIntCast(C[1], Ty, "cast");
> -    C[2] = Builder.CreateIntCast(C[2], Ty, "cast");
> +    const Type* Ty[2];
  +    Ty[0] = OrigTy;
> +    if (isa<PointerType>(Ty[0])) 
> +      Ty[0] = TD.getIntPtrType();
> +    Ty[1] = C[0]->getType();
> +
> +    C[0] = Builder.CreateBitCast(C[0], PointerType::getUnqual(Ty[0]));
> +    C[1] = Builder.CreateIntCast(C[1], Ty[0], "cast");
> +    C[2] = Builder.CreateIntCast(C[2], Ty[0], "cast");
>  
>      Result = 
>        Builder.CreateCall(Intrinsic::getDeclaration(TheModule, 
>                                                     Intrinsic::atomic_cmp_swap, 
> -                                                   &Ty, 1),
> +                                                   Ty, 2),
>        C, C + 3);

I was going to write a piece here about the weirdness of this code, but
halfway through this I suddenly saw what it was doing. Actually makes a lot of
sense once you understand it (and realize that getIntPtrType() doesn't return
a pointer-to-int type, but an int-that-fits-a-pointer type).

One thing I am still wondering about: Doesn't the above code throw away the
address space qualifer of C[0], if any? Preserving that was the orginal idea
behind these changes, right? Or don't these builtin gcc function support
different address spaces?


I'm currently testing the one-line change above, if it works I'll commit it.

> > @@ -4555,15 +4556,17 @@
> >        Emit(TREE_VALUE(TREE_CHAIN(arglist)), 0)
> >      };
> >      const Type *OrigTy = cast<PointerType>(C[0]->getType())->getElementType();
> > -    const Type* Ty = OrigTy;
> > -    if (isa<PointerType>(Ty)) 
> > -      Ty = TD.getIntPtrType();     
> > -    C[0] = Builder.CreateBitCast(C[0], PointerType::getUnqual(Ty));
> > -    C[1] = Builder.CreateIntCast(C[1], Ty, "cast");
> > +    const Type* Ty[2];
> > +    Ty[0] = OrigTy;
> > +    if (isa<PointerType>(Ty[0])) 
> > +      Ty[0] = TD.getIntPtrType();
> > +    Ty[1] = C[0]->getType();
> 
> Here you may well have used C[0] uninitialized.  More cases
> of this occur in the rest of the patch.
Why is that? C is initalized when it is declared:

    Value* C[2] = {
      Emit(TREE_VALUE(arglist), 0),
      Emit(TREE_VALUE(TREE_CHAIN(arglist)), 0)
    };

or can Emit return uninitialized values?

> Also, how about factorizing this code while you are there?
> It seems to be many copies of the same thing.
That seems like a nice idea :-)

Gr.

Matthijs
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20080731/94bd8d42/attachment.sig>