The test/Feature/constexpr.ll test case is incorrect. It is specifically allowing this: %t4 = global int** cast (uint** %t3 to int**) %t4 = global int** cast (uint** %t3 to int**) This should produce an error as it is incorrect to allow redefinition of global vars, even if they have exactly the same type and initializer. Such redefinition needs to be made illegal in llvm-as. The test case needs to be modified to not test for allowing redefinition. A new test case should check that llvm-as generates an error on redefinition.
This test case: test/Regression/Assembler/2003-02-02-ConstGlobal.ll also tests for redefinition of global/constant. Chris, are you SURE you want to make this illegal. I would have to remove this regression test if I fixed this test. It was obviously a regression on something. Are we certain the llvm-gcc4 front end cannot produce such things?
Yes, that testcase is also broken. These were due to llvm-gcc3, which would output stuff then sometimes have to change its mind. This was a broken design due to llvm-gcc3 having it's own implementation of the llvm ir that wasn't quite right.
Okay, there's a dozen or so more of these. I'll either fix the test case to not redefine a name or delete it entirely (if redefinition is all its testing).
Sounds good
Why stop at just redefinition in a type plane? Any redefinition of an external global symbol should be illegal, even across types.
i agree. Historically we allowed this *because* of type planes. However, I'd like to drop them entirely. I'm not sure I will get to it in the 2.0 timeframe though.
Hmm .. what about: test/Regression/Transforms/FunctionResolve/2003-10-21-GlobalResolveHack.ll which says: ; This tests a hack put into place specifically for the C++ libstdc++ library. ; It uses an ugly hack which is cleaned up by the funcresolve pass. ; RUN: llvm-upgrade < %s | llvm-as | opt -funcresolve | llvm-dis | \ ; grep %X | grep '{' %X = external global { int } %X = global [ 4 x sbyte ] zeroinitializer implementation int* %test() { %P = getelementptr {int}* %X, long 0, uint 0 ret int* %P } Has the ugly hack now been removed?
good point. To handle that, we need to verify that llvm-link will correctly unify the globals in these two .ll files: a.ll: %X = external global { int } b.ll: %X = global [ 4 x sbyte ] zeroinitializer The linker should merge those together: we should not have 'type-plane aware linking'. -Chris
What are the unification rules? Same size?
same name
The situation wouldn't arise if they weren't the same name. That's not what I'm asking. Assuming the names are the same: 1. What rules are there for disallowing the merged types? I suggested that they have to be the same size. 2. Which type would one pick for the merged gvar? The initialized one? The first one? Reid.
I guess it doesn't matter. llvm-link produces: ; ModuleID = 'c.bc' %X = global [4 x i8] zeroinitializer ; <[4 x i8]*> [#uses=0] implementation ; Functions: which I guess is correct. So, can I just go ahead and make same named gvars in llvm-as and verifier illegal?
sounds good. The buggy behavior in the linker has probably since been fixed.
Fixed with this patch: http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20070101/042099.html