[PATCH] AST: Mangle nullptr properly in template args for the Itanium ABI

[David Majnemer]

On Sat, Sep 7, 2013 at 1:40 PM, Richard Smith <richard at metafoo.co.uk> wrote:

> On Sat, Sep 7, 2013 at 12:33 PM, David Majnemer <david.majnemer at gmail.com>wrote:
>
>> On Sat, Sep 7, 2013 at 10:35 AM, Richard Smith <richard at metafoo.co.uk>wrote:
>>
>>>
>>> On 7 Sep 2013 02:39, "David Majnemer" <david.majnemer at gmail.com> wrote:
>>> >
>>> > Hi rsmith, rjmccall, eli.friedman, doug.gregor,
>>> >
>>> > We would previously treat nullptr literals as we were mangling the for
>>> > the null pointer value of the template parameter's type instead.
>>> >
>>> > This fixes PR17141.
>>> >
>>> > http://llvm-reviews.chandlerc.com/D1625
>>> >
>>> > Files:
>>> >   lib/AST/ItaniumMangle.cpp
>>> >   test/CodeGenCXX/mangle-nullptr-arg.cpp
>>> >   test/CodeGenCXX/visibility.cpp
>>> >
>>> > Index: lib/AST/ItaniumMangle.cpp
>>> > ===================================================================
>>> > --- lib/AST/ItaniumMangle.cpp
>>> > +++ lib/AST/ItaniumMangle.cpp
>>> > @@ -3337,10 +3337,8 @@
>>> >      break;
>>> >    }
>>> >    case TemplateArgument::NullPtr: {
>>> > -    //  <expr-primary> ::= L <type> 0 E
>>> > -    Out << 'L';
>>> > -    mangleType(A.getNullPtrType());
>>> > -    Out << "0E";
>>> > +    //  <expr-primary> ::= L <nullptr type> E
>>> > +    Out << "LDnE";
>>> >      break;
>>> >    }
>>> >    case TemplateArgument::Pack: {
>>> > Index: test/CodeGenCXX/mangle-nullptr-arg.cpp
>>> > ===================================================================
>>> > --- test/CodeGenCXX/mangle-nullptr-arg.cpp
>>> > +++ test/CodeGenCXX/mangle-nullptr-arg.cpp
>>> > @@ -2,15 +2,15 @@
>>> >
>>> >  template<int *ip> struct IP {};
>>> >
>>> > -// CHECK-LABEL: define void @_Z5test12IPILPi0EE
>>> > +// CHECK-LABEL: define void @_Z5test12IPILDnEE
>>> >  void test1(IP<nullptr>) {}
>>>
>>> The old mangling was correct here.
>>>
>>> >  struct X{ };
>>> >  template<int X::*pm> struct PM {};
>>> >
>>> > -// CHECK-LABEL: define void @_Z5test22PMILM1Xi0EE
>>> > +// CHECK-LABEL: define void @_Z5test22PMILDnEE
>>> >  void test2(PM<nullptr>) { }
>>>
>>> And here. You should only change the mangling of a null pointer of type
>>> std::nullptr_t.
>>>
>> I considered only changing the mangling for std::nullptr_t type arguments
>> but the following wording in the standard discouraged me:
>>
>> "The pointer literal nullptr is encoded as "LDnE"."
>>
>> I don't see where in the ABI we get the latitude to mangle this as-if it
>> were (param-type)0. That said there are obvious problems with giving all of
>> them the "LDnE" mangling. I will only use the different mangling for
>> nullptr_t template parameters.
>>
>
> Frequently the document lags behind discussion on cxx-abi-dev. The most
> recent discussion on this is here:
>
> http://sourcerytools.com/pipermail/cxx-abi-dev/2011-August/002422.html
>
> The LDnE mangling was agreed over a year beforehand, here:
>
> http://sourcerytools.com/pipermail/cxx-abi-dev/2010-July/002353.html
>
> ... however, looking at that in more detail, it looks like all that was
> agreed was that "nullptr" is mangled as LDnE as an expression, not
> necessarily as a template argument. And it certainly makes sense to me to
> use the L <type> 0 E mangling for all null pointer template arguments.
>
>
>> Also, maybe this ABI break should be listed in the release notes.
>> Will do.
>>
>>> Does GCC implement this rule yet?
>>>
>> They do not but icc 13.0.1 does.
>>
>
> I'm now wondering if we and gcc are correct here, and EDG and icc are
> wrong. Hopefully John can provide his interpretation :)
>

I think that you are right and that this is a defect in the specification.


>
>  > -// CHECK-LABEL: define void @_Z5test316DependentTypePtrIPiLS0_0EE
>>> > +// CHECK-LABEL: define void @_Z5test316DependentTypePtrIPiLDnEE
>>> >  template<typename T, T x> struct DependentTypePtr {};
>>> >  void test3(DependentTypePtr<int*,nullptr>) { }
>>> > Index: test/CodeGenCXX/visibility.cpp
>>> > ===================================================================
>>> > --- test/CodeGenCXX/visibility.cpp
>>> > +++ test/CodeGenCXX/visibility.cpp
>>> > @@ -971,8 +971,8 @@
>>> >    void f() {
>>> >      zed<nullptr>();
>>> >    }
>>> > -  // CHECK-LABEL: define internal void
>>> @_ZN6test523zedILPNS_12_GLOBAL__N_13fooE0EEEvv
>>> > -  // CHECK-HIDDEN-LABEL: define internal void
>>> @_ZN6test523zedILPNS_12_GLOBAL__N_13fooE0EEEvv
>>> > +  // CHECK-LABEL: define internal void @_ZN6test523zedILDnEEEvv
>>> > +  // CHECK-HIDDEN-LABEL: define internal void @_ZN6test523zedILDnEEEvv
>>> >  }
>>> >
>>> >  namespace test53 {
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130907/47c842b5/attachment.html>