[patch] Clarify and verify what llvm.used is

[Rafael Espíndola]

>> +  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?

>> +    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.

>> +      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.

>> +      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.

>
> Ciao, Duncan.


New patch attached. Thanks.

Cheers,
Rafael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: t.patch
Type: application/octet-stream
Size: 10672 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130417/12236e94/attachment.obj>