Hi Rafael,
On 17/04/13 22:21, Rafael Espíndola wrote:
+ 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.
It would cause LTO to behave differently from regular linking. Note
that we check for appending linkage in llvm.global_ctors for example.
It is the same situation in here, no?
OK, fair enough.
+ 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.
Yes, basically replace dyn_cast with cast in the users. I was going to
do it in a followup patch, but there are not that many users, so I
included it in the attached one.
You forgot to have the verifier complain 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.
Makes sense. We only care about what it points to.
Thanks!
+ 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?
Not sure. That would require every user to check for ConstantAggregateZero.
Don't they just loop over the array index range and do stuff per index, in which
case this wouldn't be a problem (empty loop)?
Finally...
--- a/lib/IR/Verifier.cpp
+++ b/lib/IR/Verifier.cpp
@@ -446,6 +446,29 @@ void Verifier::visitGlobalVariable(GlobalVariable &GV) {
}
}
+ if (GV.hasName() && (GV.getName() == "llvm.used")) {
+ Assert1(!GV.hasInitializer() || GV.hasAppendingLinkage(),
+ "invalid linkage for intrinsic global variable", &GV);
+ Type *GVType = cast<PointerType>(GV.getType())->getElementType();
The cast shouldn't be needed as if GV is a GlobalValue then getType is tweaked
to return PointerType.
+ if (ArrayType *ATy = dyn_cast<ArrayType>(GVType)) {
+ PointerType *PTy = dyn_cast<PointerType>(ATy->getElementType());
+ Assert1(PTy, "wrong type for intrinsic global variable", &GV);
+ if (GV.hasInitializer()) {
+ Constant *Init = GV.getInitializer();
+ ConstantArray *InitArray = dyn_cast<ConstantArray>(Init);
+ Assert1(InitArray, "wrong initalizer for intrinsic global variable",
+ Init);
+ 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
