[PATCH] Wrong mangling of template argument in presence of another template function.

[Pierre Gousseau]

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 <richard at metafoo.co.uk> wrote:

> On Thu, Nov 13, 2014 at 6:14 AM, pierre gousseau <
> pierregousseau14 at gmail.com> 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
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141114/1148cce5/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: wrong-template-mangling.v2.patch
Type: application/octet-stream
Size: 2567 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141114/1148cce5/attachment.obj>