[PATCH] [ms-cxxabi] Mangle function pointer arguments correctly

[Richard Smith]

================
Comment at: lib/AST/MicrosoftMangle.cpp:1217-1225
@@ -1210,11 +1216,11 @@
     if (D) {
-      // If we got a decl, use the type-as-written to make sure arrays
-      // get mangled right.  Note that we can't rely on the TSI
-      // existing if (for example) the parameter was synthesized.
+      // If we got a decl, use it to get the source range.  This used to be
+      // important for getting the type-as-written.  We've since changed to
+      // traversing the type-as-written, so this shouldn't be necessary anymore.
       for (FunctionDecl::param_const_iterator Parm = D->param_begin(),
              ParmEnd = D->param_end(); Parm != ParmEnd; ++Parm) {
         TypeSourceInfo *TSI = (*Parm)->getTypeSourceInfo();
         QualType Type = TSI ? TSI->getType() : (*Parm)->getType();
         mangleArgumentType(Type, (*Parm)->getSourceRange());
       }
     } else {
----------------
Reid Kleckner wrote:
> Richard Smith wrote:
> > Can you just use the parameter types from the FunctionProtoType here?
> Yes, but I either need to do a parallel iteration or lose the source range.  What's preferable?
It seems fine to just pass in the source range for the function type; these source ranges are just used for emitting "we can't mangle this yet" diagnostics, so accuracy doesn't seem like a massive priority. We need this to work for the case of functions with no corresponding FunctionDecl anyway, and that case would be improved by passing mangleFunctionType a SourceRange instead of a FunctionDecl.

Incidentally, the MS mangler's use of SourceRange seems like overkill -- all it uses is the begin location from the range, so it may as well use a SourceLocation.

================
Comment at: lib/AST/MicrosoftMangle.cpp:263-264
@@ -262,2 +262,4 @@
   // We don't even know how to mangle their types anyway :).
-  const FunctionProtoType *FT = FD->getType()->castAs<FunctionProtoType>();
+  TypeSourceInfo *TSI = FD->getTypeSourceInfo();
+  QualType T = TSI ? TSI->getType() : FD->getType();
+  const FunctionProtoType *FT = T->castAs<FunctionProtoType>();
----------------
Reid Kleckner wrote:
> Peter Collingbourne wrote:
> > Richard Smith wrote:
> > > Which declaration is 'FD' here? If I have this in a header:
> > > 
> > >   void f(int p[123]);
> > > 
> > > ... and this in my source file:
> > > 
> > >   #include "header"
> > >   void f(int *const p) {}
> > > 
> > > ... then do we use the mangling for the header's declaration or for the source file's declaration? I would guess (or rather, hope) that we use the mangling for the first declaration of the function.
> > ITYM
> > 
> >   void f(int *p) {}
> > 
> > i.e. a redeclaration of f with a different mangling (the const qualified version would have the same mangling).
> Congratulations, you just identified a mangling bug that results in real link errors.
> 
> If the forward decl for f is present, the parameter is mangled as const.  If the forward is missing, it is non-const, resulting in link errors.  Awesome.
> 
> I'm adding FD = FD->getFirstDeclaration(), which I hope is the right fix.
SGTM


http://llvm-reviews.chandlerc.com/D844