[patch] Clarify and verify what llvm.used is

[Duncan Sands]

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.