[cfe-dev] Bug in clang parsing pointer to an array in function parameter list

[Steve Naroff]

On Sep 9, 2007, at 3:32 PM, Justin Handville wrote:

> With the following patch, I can now compile and execute this source  
> file:
>
> int puts(char*);
>
> int main(int argc, char* argv[])
> {
>   char* foo = argv[0];
>
>   puts(foo);
>
>   return 0;
> }
>

Justin,

I'm a bit confused. The front-end (Sema::ParseParamDeclarator)  
already converts argv from "char *[]" to "char **".

See the "DeclRefExpr" node below (on line 6).

I don't know why the code generator would need to fiddle with this.

Chris will need to advise...

snaroff

[dylan:~/llvm/tools/clang] admin% ../../Debug/bin/clang justin.c - 
parse-ast-dump

int puts(char *);

int main(int argc, char *argv[])
(CompoundStmt 0x3105ed0 <justin.c:5:1, line:11:1>
   (DeclStmt 0x3105bf0 <:0:0>
     0x3105dd0 "char *foo =
       (ArraySubscriptExpr 0x3105db0 <justin.c:6:15, col:21> 'char *'
         (DeclRefExpr 0x3105d70 <col:15> 'char **' Decl='argv'  
0x3105d40)
         (IntegerLiteral 0x3105d90 <col:20> 'int' 0))")
   (CallExpr 0x3105e70 <line:8:3, col:11> 'int'
     (ImplicitCastExpr 0x3105e60 <col:3> 'int (*)(char *)'
       (DeclRefExpr 0x3105e00 <col:3> 'int (char *)' Decl='puts'  
0x3105c20))
     (DeclRefExpr 0x3105e20 <col:8> 'char *' Decl='foo' 0x3105dd0))
   (ReturnStmt 0x3105ec0 <line:10:3, col:10>
     (IntegerLiteral 0x3105ea0 <col:10> 'int' 0)))


> Here's the patch.  I'm curious to see if this will work in all cases.
> I am still adding test cases to the build, so it is a bit
> experimental:
>
> ndex: CodeGen/CodeGenFunction.h
> ===================================================================
> --- CodeGen/CodeGenFunction.h   (revision 41790)
> +++ CodeGen/CodeGenFunction.h   (working copy)
> @@ -237,6 +237,11 @@
>    };
>    llvm::SmallVector<BreakContinue, 8> BreakContinueStack;
>
> +  static bool
> +  isCompatibleDeclPointerType(
> +    const llvm::Type* Arg,
> +    const llvm::Type* DeclPtr);
> +
>  public:
>    CodeGenFunction(CodeGenModule &cgm);
>
> Index: CodeGen/CGDecl.cpp
> ===================================================================
> --- CodeGen/CGDecl.cpp  (revision 41790)
> +++ CodeGen/CGDecl.cpp  (working copy)
> @@ -96,11 +96,69 @@
>    }
>  }
>
> +bool
> +CodeGenFunction::isCompatibleDeclPointerType(
> +  const llvm::Type* ArgType,
> +  const llvm::Type* DeclType)
> +{
> +  if(
> +      ArgType->getTypeID() != llvm::Type::PointerTyID
> +   || DeclType->getTypeID() != llvm::Type::PointerTyID)
> +  {
> +    return false;
> +  }
> +
> +  while(
> +      ArgType->getNumContainedTypes() == 1
> +   && DeclType->getNumContainedTypes() == 1)
> +  {
> +    const llvm::Type* ArgContainedType = ArgType->getContainedType 
> (0);
> +    const llvm::Type* DeclContainedType = DeclType- 
> >getContainedType(0);
> +
> +    //if the two contained types point to the same
> +    //location, then these pointer types are compatible
> +    if(
> +      ArgContainedType == DeclContainedType)
> +    {
> +      return true;
> +    }
> +    //is the pointer indirection level different?
> +    else if(
> +      ArgContainedType->getNumContainedTypes() !=
> +        DeclContainedType->getNumContainedTypes())
> +    {
> +      break;
> +    }
> +    //are these compatible pointer / array types?
> +    else if(
> +        llvm::SequentialType::classof(ArgContainedType)
> +     && llvm::SequentialType::classof(DeclContainedType))
> +    {
> +      if( ArgContainedType->getNumContainedTypes() != 1 )
> +      {
> +        //FIXME will this ever happen with arrays / pointers?
> +        break;
> +      }
> +
> +      ArgType = ArgContainedType;
> +      DeclType = DeclContainedType;
> +    }
> +    else
> +    {
> +      //one or more of these is something other than an array /  
> pointer
> +      break;
> +    }
> +  }
> +
> +  return false;
> +}
> +
>  /// Emit an alloca for the specified parameter and set up  
> LocalDeclMap.
>  void CodeGenFunction::EmitParmDecl(const ParmVarDecl &D,  
> llvm::Value *Arg) {
>    QualType Ty = D.getCanonicalType();
>
>    llvm::Value *DeclPtr;
> +
>    if (!Ty->isConstantSizeType(getContext())) {
>      // Variable sized values always are passed by-reference.
>      DeclPtr = Arg;
> @@ -111,7 +169,19 @@
>        // TODO: Alignment
>        DeclPtr = new llvm::AllocaInst(LTy, 0, std::string(D.getName 
> ())+".addr",
>                                       AllocaInsertPt);
> -
> +
> +      //do we need to perform an implicit conversion?
> +      if( isCompatibleDeclPointerType(Arg->getType(), DeclPtr- 
> >getType()) )
> +      {
> +        llvm::Value* CastPtr =
> +          Builder.CreateBitCast(
> +            Arg,
> +            LTy,
> +            (std::string(D.getName()) + ".implconv").c_str());
> +
> +        Arg = CastPtr;
> +      }
> +
>        // Store the initial value into the alloca.
>        Builder.CreateStore(Arg, DeclPtr);
>      } else {
>
> On 9/9/07, Justin Handville <uniheliodem at gmail.com> wrote:
>> Perhaps the better question to ask is where should this transform  
>> occur?
>>
>> On 9/9/07, Justin Handville <uniheliodem at gmail.com> wrote:
>>> The following code snippet generates an assert when compiled with
>>> clang -emit-llvm
>>>
>>> int main(int argc, char* argv[])
>>> {
>>>   return 0;
>>> }
>>>
>>> Assert:
>>>
>>> Assertion failed: (getOperand(0)->getType() ==
>>> cast<PointerType>(getOperand(1)->getType())->getElementType() &&  
>>> "Ptr
>>> must be a pointer to Val type!"), function AssertOK, file
>>> Instructions.cpp, line 796.
>>>
>>> The problem here is at line 116 of CGDecl.cpp:
>>>
>>>     if (LTy->isFirstClassType()) {
>>>       // TODO: Alignment
>>>       DeclPtr = new llvm::AllocaInst(LTy, 0, std::string(D.getName 
>>> ())+".addr",
>>>                                      AllocaInsertPt);
>>>
>>>       // Store the initial value into the alloca.
>>>       Builder.CreateStore(Arg, DeclPtr);  //attempting to store
>>> char*[] as char**
>>>
>>> The problem is that llvm treats char*[] and char** as two distinct
>>> types as far as I can tell.  EmitParmDecl should convert the type  
>>> to a
>>> char** before calling CreateStore, since this is how the argument
>>> would be passed in C ABI.
>>>
>>> I assume that a transform step should be added before the  
>>> CreateStore
>>> that could normalize the argument to match the declptr.  I assume a
>>> good approach would be to descend through both Arg and DeclPtr,
>>> transforming Arg to match DeclPtr where compatible, or raising an
>>> appropriate diagnostic error if such a transformation is not  
>>> possible.
>>>
>>> If I can get some approval that this route would work in this case,
>>> then I will go ahead and submit the patch.
>>>
>>> Thanks,
>>> Justin
>>>
>>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev