Weak alias assumed to be non-null results in wrong code for unwind-dw2.c

[llvmbot]

Bugzilla Link	1646
Resolution	FIXED
Resolved on	Sep 10, 2007 18:48
Version	2.0
OS	Linux
Attachments	testcase .ll
Reporter	LLVM Bugzilla Contributor
CC	@asl

Extended Description

The attached testcase is the code used by gcc to test
whether a program was linked with the pthreads library.
LLVM optimizes it to

define i32 @​__gthread_active_p() {
entry:
ret i32 1
}

which is wrong. Probably it thinks that the weak alias
@​__gthrw_pthread_cancel = alias weak i32 (i32)* @​pthread_cancel
is necessarily non-null. The original code was:

static __typeof(pthread_cancel) __gthrw_pthread_cancel attribute ((weakref("pthread_cancel")));

static inline int
__gthread_active_p (void)
{
static void *const __gthread_active_ptr
= extension (void *) &__gthrw_pthread_cancel;
return __gthread_active_ptr != 0;
}

[llvmbot]

assigned to @lattner

[nlewycky]

reduced testcase; run -instcombine on it
This is due to instcombine. Look at this excerpt of InstCombiner::visitICmpInst:

4694 // icmp of GlobalValues can never equal each other as long as they aren't
4695 // external weak linkage type.
4696 if (GlobalValue GV0 = dyn_cast(Op0))
4697 if (GlobalValue GV1 = dyn_cast(Op1))
4698 if (!GV0->hasExternalWeakLinkage() || !GV1->hasExternalWeakLinkage())
4699 return ReplaceInstUsesWith(I, ConstantInt::get(Type::Int1Ty,
4670 !isTrueWhenEqual(I)));
4671
4672 // icmp <global/alloca/null>, <global/alloca/null> - Global/Stack value
4673 // addresses never equal each other! We already know that Op0 != Op1.
4674 if ((isa(Op0) || isa(Op0) ||
4675 isa(Op0)) &&
4676 (isa(Op1) || isa(Op1) ||
4677 isa(Op1)))
4678 return ReplaceInstUsesWith(I, ConstantInt::get(Type::Int1Ty,
4679 !isTrueWhenEqual(I)));

The bug is in the second transformation. If one side of an icmp is global and the other is null, it doesn't check for external weak linkage the way the check right above it does.

[nlewycky]

testcase that actually assembles
My analysis was flawed, and my testcase was bitten by #2017 .

The problem might actually be in constant folding. By line 957 of ConstantFold.cpp, we've established that we're doing a comparison of a GlobalValue to a ConstantPointerNull:

957 // GlobalVals can never be null.
958 assert(isa(V2) && "Canonicalization guarantee!");
959 if (!C#1 ->hasExternalWeakLinkage())
960 return ICmpInst::ICMP_NE;

(gdb) p C#1 ->hasExternalWeakLinkage()
$17 = false
(gdb) p C#1 ->dump()
@​__gthrw_pthread_cancel = alias weak i32 (i32)* @​pthread_cancel ; <i32 (i32)*> [#uses=1]

Note that external_weak is illegal on an alias. I'm not sure what the resolution ought to be here. I'm also not sure whether the original issue I found in instcombine also needs to be fixed.

[asl]

Note that external_weak is illegal on an alias. I'm not sure what the
resolution ought to be here. I'm also not sure whether the original issue I
found in instcombine also needs to be fixed.
It seems, that we have to check whether GV is an alias and "ultimate dereference" it (in terms of gcc): we have to go through the chain of aliasees and obtain the last GV being aliased. And test for linkage on it. This is because alias itself can be weak (=weakref), but aliasee itself - not.

[llvmbot]

Is it possible for a GV to be a weak reference to itself,
perhaps through a chain of weak references? If not, does
the verifier reject attempts to create such a self-referential
GV?

[llvmbot]

What does it mean to have a "weak alias" rather than just an "alias"?
And is it correct to have a "weak alias" to something which is not
weak (in this case to something which is "extern_weak")? What is the
relationship between the linkage type of the alias and the aliasee?
Is there any point in an alias having a linkage type? From Nick's
reduced testcase:

@​__gthrw_pthread_cancel = alias weak i32 (i32)* @​pthread_cancel
declare extern_weak i32 @​pthread_cancel(i32)

[asl]

Is it possible for a GV to be a weak reference to itself,
perhaps through a chain of weak references?
No.

If not, does the verifier reject attempts to create such a self-referential
GV?
Added to todo list.

[asl]

What does it mean to have a "weak alias" rather than just an "alias"?
Alias defines new symbol, which is "physically" the same as aliasee. Thus it can have its own linkage.

And is it correct to have a "weak alias" to something which is not
weak (in this case to something which is "extern_weak")?
Yes.

What is the relationship between the linkage type of the alias and the aliasee?
No relation at all.

@​__gthrw_pthread_cancel = alias weak i32 (i32)* @​pthread_cancel
declare extern_weak i32 @​pthread_cancel(i32)
This seems to be correct.

[llvmbot]

Alias defines new symbol, which is "physically" the same as aliasee. Thus it
can have its own linkage.

If the alias is the same as the aliasee, and just provides a different
name for it, shouldn't it have the same linkage as the aliasee? In fact,
shouldn't it not have linkage at all, and everything be taken from the
aliasee?

[asl]

Alias defines new symbol, which is "physically" the same as aliasee. Thus it
can have its own linkage.

If the alias is the same as the aliasee, and just provides a different
name for it, shouldn't it have the same linkage as the aliasee?
No, because it's new symbol, which, in fact, references the same symbol.

[asl]

Alias defines new symbol, which is "physically" the same as aliasee. Thus it
can have its own linkage.

If the alias is the same as the aliasee, and just provides a different
name for it, shouldn't it have the same linkage as the aliasee?
No, because it's new symbol, which, in fact, references the same symbol.

[llvmbot]

No, because it's new symbol, which, in fact, references the same symbol.

Do you mean that if A is a weak alias for B in object file O1, but
A is a strong symbol in object file O2, then, when you link, the
connection between A and B is forgotten, and A becomes the strong
symbol in the linked file? Or something like that?

[lattner]

The fix is to tell the constant folding pass to back off from aliases. We expect the linker to resolve them as aggressively as possible, so this shouldn't cause poor codegen. The code you found in instcombine is actually dead, as it is only reached if the operands are not both constants.

Both issues are fixed here:
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20070910/053371.html
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20070910/053372.html

-Chris

[llvmbot]

mentioned in issue llvm/llvm-bugzilla-archive#1678