Bug in clang parsing pointer to an array in function parameter list

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

Perhaps the better question to ask is where should this transform occur?

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;
}

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

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)))

Ah. Then I'll check in the front end. I thought that the char*[] in
the parameter list looked a little funny, but I thought maybe that
might be intended.

Without the patch, the code asserts, since it is trying to store a
char*[] into a char** location. It wouldn't be difficult to add the
check to the appropriate place in the front end to change the argument
declaration to a char**, as llvm-gcc does.

Also, note that the issue is with the initial store of the argv
argument generated by the code generator, which isn't shown in the
parse-ast-dump call.

I'm still not familiar with the code base, so I'll need a little
guidance with where this change belongs, but the bug definitely
exists.

Okay. I found the section in Sema::ParseParamDeclarator where the bug
is occurring. Apparently, this is where the fix needs to go. I'll
work on analyzing why this code is failing, put together some test
cases, and submit the patch at the appropriate place. I apologize,
I'm still learning the code.

Okay. I found the section in Sema::ParseParamDeclarator where the bug
is occurring. Apparently, this is where the fix needs to go. I'll
work on analyzing why this code is failing, put together some test
cases, and submit the patch at the appropriate place. I apologize,
I'm still learning the code.

No need to apologize. Here is some more data that might help...

Sema::ParseParamDeclarator().

First, let's be clear on the bug. The bug is the functions type is not in sync with it's ParmVarDecl AST node.

The function type is computed *before* calling Sema::ParseParamDeclarator(), which does the ParmVarDecl conversion. As a result, we could either...

(a) Move the argument conversion "up" to Sema::GetTypeForDeclarator(). If we did this, the conversion code could be removed from Sema::ParseParamDeclarator().
(b) Have Sema::ParseParamDeclarator() some how change the functions type when it does the conversion for ParmVarDecl (I don't like this, put want to mention it for completeness).
(c) See if it is possible for the code generator to rely on the ParmVarDecls, *not* the function type. Chris and I thought long and hard about this particular ParmVarDecl conversion (hence the large comment in the code). That said, this may well be a code gen bug. If not, then we will need to consider a front-end fix.

snaroff

Breakpoint 1, clang::Sema::GetTypeForDeclarator (this=0x2605900, D=@0xbffff2a0, S=0x2605a50) at SemaType.cpp:248
248 if (!FTI.hasPrototype) {
(gdb) c
Continuing.
int (foo)(int, char *[])

Breakpoint 2, clang::Sema::ParseParamDeclarator (this=0x2605900, FTI=@0xbffff2c0, ArgNo=0, FnScope=0x2605b20) at SemaDecl.cpp:688
688 QualType parmDeclType = QualType::getFromOpaquePtr(PI.TypeInfo);

Well, I'll defer to you and Chris on this. Let me know where to make
the change, and I will be more than happy to make it. Would this also
be a good time to address the FIXME at the bottom of the comment in
Sema::ParseParamDeclarator corresponding to this?