Hi Rafael,
On 17/04/13 01:18, Rafael Espíndola wrote:
When reviewing my previous patch to GlobalOpt Duncan raised some
questions about the semantics of llvm.used. We should probably clarify
the documentation and have the verifier check it before anything else.
The attached patch makes it explicit that aliases can be put in
llvm.used and changes the verifier to check llvm.used only has
functions, variables and aliases.
Duncan, are you OK with this definition? Is the patch OK?
I think the definition is fine. However
+++ b/lib/IR/Verifier.cpp
@@ -446,6 +446,32 @@ void Verifier::visitGlobalVariable(GlobalVariable &GV) {
}
}
+ if (GV.hasName() && (GV.getName() == "llvm.used")) {
+ Assert1(!GV.hasInitializer() || GV.hasAppendingLinkage(),
+ "invalid linkage for intrinsic global variable", &GV);
I don't think the verifier should be testing the linkage. Using a different
kind of linkage does probably indicate that the user made a mistake, but as it
doesn't have any impact on LLVM itself (eg it doesn't make optimizations invalid
or something problematic like that) I don't think it should be illegal. Maybe
this check could be moved into the "lint" pass.
+ Type *GVType = cast<PointerType>(GV.getType())->getElementType();
+ if (ArrayType *ATy = dyn_cast<ArrayType>(GVType)) {
Should it be allowed to not have array type? If we decide to only allow array
type then all the places that work with llvm.used can probably be simplified a
bit, since I think they all bail out if the type is not an array type.
+ PointerType *PTy = dyn_cast<PointerType>(ATy->getElementType());
+ Assert1(PTy, "wrong type for intrinsic global variable", &GV);
+ IntegerType *IntTy = dyn_cast<IntegerType>(PTy->getElementType());
+ Assert1(IntTy && IntTy->getBitWidth() == 8,
+ "wrong type for intrinsic global variable", &GV);
I don't think we should test for a particular pointer type. Traditionally
people use i8*, but as the pointer type doesn't have any semantic impact I
don't think using other types should be illegal. For example, using {}*
would be perfectly reasonable IMO.
+ if (GV.hasInitializer()) {
+ Constant *Init = GV.getInitializer();
+ ConstantArray *InitArray = dyn_cast<ConstantArray>(Init);
+ Assert1(InitArray, "wrong initalizer for intrinsic global variable",
+ Init);
This would reject a zero length array type initialized to null. So maybe only
test this if the array length is non-zero?
+ for (unsigned i = 0, e = InitArray->getNumOperands(); i != e; ++i) {
+ Value *V = Init->getOperand(i)->stripPointerCasts();
+ // stripPointerCasts strips aliases, so we only need to check for
+ // variables and functions.
+ Assert1(isa<GlobalVariable>(V) || isa<Function>(V),
+ "invalid llvm.used member", V);
+ }
+ }
+ }
+ }
+
visitGlobalValue(GV);
}
Ciao, Duncan.
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
