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

[Richard Smith]

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

 > -// 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/f2183bf0/attachment.html>