[patch] Clarify and verify what llvm.used is

[Duncan Sands]

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.