Thank you Richard for reviewing the change, I have attached a new version of the patch with your suggestions. If all look good I would be grateful if it could be committed for me as I do not have a svn account. Regards,
Pierre Gousseau SN Systems - Sony Computer Entertainment Group On 14 November 2014 02:57, Richard Smith <[email protected]> wrote: > On Thu, Nov 13, 2014 at 6:14 AM, pierre gousseau < > [email protected]> wrote: > >> Dear All, >> >> I would like to propose a fix to an ABI issue found with the new ABI test >> suite. >> This is my first patch for clang so please bear with me :-) >> > > Thanks for the investigation and the patch! > > >> The test case is: >> >> clang version 3.6.0 (221593) >> clang -cc1 -triple i386-pc-linux -S -emit-llvm >> ---- >> #ifdef BAD >> template <class T> void g3(char (&buffer)[sizeof(T() + 5)]) {} >> void call_g3() { >> char buffer[sizeof(int)]; >> g3<int>(buffer); >> } >> #endif >> template <class T> void g4(char (&buffer)[sizeof(T() + 5L)]) {} >> void call_g4() { >> char buffer[sizeof(long int)]; >> g4<long int>(buffer); >> } >> ---- >> >> In the test case above the mangled name for g4 is different depending on >> the presence of g3. >> >> When g3 is not defined, g4 mangled name is : _Z2g4IlEvRAszplcvT__ELl5E_c >> When g3 is defined, g4 mangled name is : _Z2g4IlEvRAszplcvT__ELi5E_c >> >> The issue seem to be caused by the AST mechanism that decides when >> existing nodes can be reused to deduce the type of X in the expression >> "char (&)[X]". >> Where "X" is a type dependent expression (eg T() + 5L). >> The mechanism seems to only take the size of the integer literal "5" to >> discriminate between "5" and "5L". >> So the patch adds the "builtin kind" to the list of attributes to >> consider, when discriminating between AST nodes. >> >> While the anomaly was initially spotted between "int" and "long int" on >> X86, the investigation led to see that the problem also existed for targets >> where "double" and "long double" are the same size so the triple >> "mips-none-none" was chosen for the regression test. >> >> Please let me know if this is an acceptable change. >> > > Yes, this is the right approach. > > >> Regards, >> >> Pierre Gousseau >> SN Systems - Sony Computer Entertainment Group >> >> --- >> Index: svn/upstream/tools/clang/lib/AST/StmtProfile.cpp >> =================================================================== >> --- svn/upstream/tools/clang/lib/AST/StmtProfile.cpp (revision 221593) >> +++ svn/upstream/tools/clang/lib/AST/StmtProfile.cpp (working copy) >> @@ -501,6 +501,10 @@ >> void StmtProfiler::VisitIntegerLiteral(const IntegerLiteral *S) { >> VisitExpr(S); >> S->getValue().Profile(ID); >> + SplitQualType splitType = S->getType().split(); >> + const BuiltinType * builtinType = splitType.Ty->getAs<BuiltinType>(); >> + if (builtinType) >> + ID.AddInteger(builtinType->getKind()); >> > > You can use > > ID.AddInteger(S->getType()->castAs<BuiltinType>()->getKind()); > > here, because the integer literal is guaranteed to be of a builtin type. > (And no need to split the type manually; QualType's operator-> does the > right thing here.) > > >> } >> >> void StmtProfiler::VisitCharacterLiteral(const CharacterLiteral *S) { >> @@ -513,6 +517,10 @@ >> VisitExpr(S); >> S->getValue().Profile(ID); >> ID.AddBoolean(S->isExact()); >> + SplitQualType splitType = S->getType().split(); >> + const BuiltinType * builtinType = splitType.Ty->getAs<BuiltinType>(); >> + if (builtinType) >> + ID.AddInteger(builtinType->getKind()); >> > > Likewise here. > > Do you have an SVN account, or do you need someone to commit this for you? > > >> } >> >> void StmtProfiler::VisitImaginaryLiteral(const ImaginaryLiteral *S) { >> Index: >> svn/upstream/tools/clang/test/CodeGen/mips-wrong-template-mangling.cpp >> =================================================================== >> --- svn/upstream/tools/clang/test/CodeGen/mips-wrong-template-mangling.cpp >> (revision >> 0) >> +++ svn/upstream/tools/clang/test/CodeGen/mips-wrong-template-mangling.cpp >> (working >> copy) >> @@ -0,0 +1,38 @@ >> +// This checks that the mangling of g4 and g6 is not dependent on the >> presence >> +// of g3 and g5, the bug was due to the AST nodes for the array size >> expressions >> +// being wrongly shared between g3/g4 and g5/g6 on targets where the >> single >> +// "long" keyword has no effects >> +// RUN: %clang_cc1 -triple mips-none-none -emit-llvm -o - %s | FileCheck >> %s >> +// RUN: %clang_cc1 -triple mips-none-none -DBAD -emit-llvm -o - %s | >> FileCheck -check-prefix=CHECK-BAD %s >> + >> +#ifdef BAD >> +template <class T> void g3(char (&buffer)[sizeof(T() + 5.0)]) {} >> +void call_g3() { >> + char buffer[sizeof(double)]; >> + // CHECK-BAD: _Z2g3IdEvRAszplcvT__ELd4014000000000000E_c >> + g3<double>(buffer); >> +} >> +#endif >> +template <class T> void g4(char (&buffer)[sizeof(T() + 5.0L)]) {} >> +void call_g4() { >> + char buffer[sizeof(long double)]; >> + // CHECK: _Z2g4IeEvRAszplcvT__ELe4014000000000000E_c >> + // CHECK-BAD: _Z2g4IeEvRAszplcvT__ELe4014000000000000E_c >> + g4<long double>(buffer); >> +} >> + >> +#ifdef BAD >> +template <class T> void g5(char (&buffer)[sizeof(T() + 5)]) {} >> +void call_g5() { >> + char buffer[sizeof(int)]; >> + // CHECK-BAD: _Z2g5IiEvRAszplcvT__ELi5E_c >> + g5<int>(buffer); >> +} >> +#endif >> +template <class T> void g6(char (&buffer)[sizeof(T() + 5L)]) {} >> +void call_g6() { >> + char buffer[sizeof(long int)]; >> + // CHECK: _Z2g6IlEvRAszplcvT__ELl5E_c >> + // CHECK-BAD: _Z2g6IlEvRAszplcvT__ELl5E_c >> + g6<long int>(buffer); >> +} >> --- >> >> _______________________________________________ >> cfe-commits mailing list >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> >> >
wrong-template-mangling.v2.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
