| Message ID | 1401191856-27585-3-git-send-email-palves@redhat.com |
|---|---|
| State | New |
| Headers | show |
> Fix this by adding a new DEMANGLE_COMPONENT_CONVERSION component type, > which does what DEMANGLE_COMPONENT_CAST does today, and making > DEMANGLE_COMPONENT_CAST just simply print its component subtree. > > I think we could instead reuse DEMANGLE_COMPONENT_CAST and in > d_print_comp_inner still do: > > @@ -5001,9 +5013,9 @@ d_print_comp_inner (struct d_print_info *dpi, int options, > d_print_comp (dpi, options, dc->u.s_extended_operator.name); > return; > > case DEMANGLE_COMPONENT_CAST: > d_append_string (dpi, "operator "); > - d_print_cast (dpi, options, dc); > + d_print_conversion (dpi, options, dc); > return; > > leaving the unary cast case below calling d_print_cast, but seems to > me that spliting the component types makes it easier to reason about > the code. I agree. > libiberty/ > 2014-05-27 Pedro Alves <palves@redhat.com> > > PR other/61321 > PR other/61233 > * demangle.h (enum demangle_component_type) > <DEMANGLE_COMPONENT_CONVERSION>: New value. > * cp-demangle.c (d_demangle_callback, d_make_comp): Handle > DEMANGLE_COMPONENT_CONVERSION. > (is_ctor_dtor_or_conversion): Handle DEMANGLE_COMPONENT_CONVERSION > instead of DEMANGLE_COMPONENT_CAST. > (d_operator_name): Return a DEMANGLE_COMPONENT_CONVERSION > component if handling a conversion. > (d_count_templates_scopes, d_print_comp_inner): Handle > DEMANGLE_COMPONENT_CONVERSION. > (d_print_comp_inner): Handle DEMANGLE_COMPONENT_CONVERSION instead > of DEMANGLE_COMPONENT_CAST. > (d_print_cast): Rename as ... > (d_print_conversion): ... this. Adjust comments. > (d_print_cast): Rewrite - simply print the left subcomponent. > * cp-demint.c (cplus_demangle_fill_component): Handle > DEMANGLE_COMPONENT_CONVERSION. > > * testsuite/demangle-expected: Add tests. Looks good to me. Thanks! Ian, does this look good to you? -cary
On Fri, May 30, 2014 at 10:37 AM, Cary Coutant <ccoutant@google.com> wrote: >> Fix this by adding a new DEMANGLE_COMPONENT_CONVERSION component type, >> which does what DEMANGLE_COMPONENT_CAST does today, and making >> DEMANGLE_COMPONENT_CAST just simply print its component subtree. >> >> I think we could instead reuse DEMANGLE_COMPONENT_CAST and in >> d_print_comp_inner still do: >> >> @@ -5001,9 +5013,9 @@ d_print_comp_inner (struct d_print_info *dpi, int options, >> d_print_comp (dpi, options, dc->u.s_extended_operator.name); >> return; >> >> case DEMANGLE_COMPONENT_CAST: >> d_append_string (dpi, "operator "); >> - d_print_cast (dpi, options, dc); >> + d_print_conversion (dpi, options, dc); >> return; >> >> leaving the unary cast case below calling d_print_cast, but seems to >> me that spliting the component types makes it easier to reason about >> the code. > > I agree. > >> libiberty/ >> 2014-05-27 Pedro Alves <palves@redhat.com> >> >> PR other/61321 >> PR other/61233 >> * demangle.h (enum demangle_component_type) >> <DEMANGLE_COMPONENT_CONVERSION>: New value. >> * cp-demangle.c (d_demangle_callback, d_make_comp): Handle >> DEMANGLE_COMPONENT_CONVERSION. >> (is_ctor_dtor_or_conversion): Handle DEMANGLE_COMPONENT_CONVERSION >> instead of DEMANGLE_COMPONENT_CAST. >> (d_operator_name): Return a DEMANGLE_COMPONENT_CONVERSION >> component if handling a conversion. >> (d_count_templates_scopes, d_print_comp_inner): Handle >> DEMANGLE_COMPONENT_CONVERSION. >> (d_print_comp_inner): Handle DEMANGLE_COMPONENT_CONVERSION instead >> of DEMANGLE_COMPONENT_CAST. >> (d_print_cast): Rename as ... >> (d_print_conversion): ... this. Adjust comments. >> (d_print_cast): Rewrite - simply print the left subcomponent. >> * cp-demint.c (cplus_demangle_fill_component): Handle >> DEMANGLE_COMPONENT_CONVERSION. >> >> * testsuite/demangle-expected: Add tests. > > Looks good to me. Thanks! > > Ian, does this look good to you? I tend to defer to Jason on this sort of newfangled mangling nuttiness, but I can take a look if he doesn't have time. Ian
On 05/30/2014 07:05 PM, Ian Lance Taylor wrote: >> > Looks good to me. Thanks! Thanks Cary! >> > Ian, does this look good to you? > I tend to defer to Jason on this sort of newfangled mangling > nuttiness, but I can take a look if he doesn't have time. Thanks! FWIW, it'd be great to have this approved/rejected, as I'd like to get this fixed in gdb 7.8, and we're just about to branch. Thanks,
On 05/27/2014 07:57 AM, Pedro Alves wrote: > And after the fix for 59195, due to: > > static void > d_print_cast (struct d_print_info *dpi, int options, > const struct demangle_component *dc) > { > ... > /* For a cast operator, we need the template parameters from > the enclosing template in scope for processing the type. */ > if (dpi->current_template != NULL) > { > dpt.next = dpi->templates; > dpi->templates = &dpt; > dpt.template_decl = dpi->current_template; > } > > when printing the template argument list of A (what should be "<sizeof > (int)>"), the template parameter 0 (that is, "T_", the '**' above) now > refers to the first parameter of the the template argument list of the > 'A' template (the '*' above), exactly what we were already trying to > print. This leads to infinite recursion, and stack exaustion. The > template parameter 0 should actually refer to the first parameter of > the 'function_temp' template. It seems that the problem here is more general; a template argument list is not in scope within that same template argument list. Can't we fix that without special-casing conversion ops? Jason
> It seems that the problem here is more general; a template argument list is > not in scope within that same template argument list. Can't we fix that > without special-casing conversion ops? I think conversion ops really are a special case. It's the only case where the template parameters refer to the template argument list from the cast operator's enclosing template. In a cast expression, like anywhere else you might have a template parameter, the template parameter refers to the template argument list of the immediately enclosing template. I think this note from Section 5.1.3 (Operator Encodings) of the ABI is what makes this a special case (it's an informative comment in the document, but seems to me to be normative): "For a user-defined conversion operator the result type (i.e., the type to which the operator converts) is part of the mangled name of the function. If the conversion operator is a member template, the result type will appear before the template parameters. There may be forward references in the result type to the template parameters." -cary
On 07/24/2014 11:35 PM, Cary Coutant wrote: >> It seems that the problem here is more general; a template argument list is >> not in scope within that same template argument list. Can't we fix that >> without special-casing conversion ops? > > I think conversion ops really are a special case. Thanks Cary. FWIW, I agree. (GDB 7.8 hasn't been released yet, though it's close. If this patch is approved as is, we'll be able to have the crash fixed there. If this requires a significant rewrite though, I'm afraid I might not be able to do it myself anytime soon.) > It's the only case > where the template parameters refer to the template argument list from > the cast operator's enclosing template. In a cast expression, like > anywhere else you might have a template parameter, the template > parameter refers to the template argument list of the immediately > enclosing template. > > I think this note from Section 5.1.3 (Operator Encodings) of the ABI > is what makes this a special case (it's an informative comment in the > document, but seems to me to be normative): > > "For a user-defined conversion operator the result type (i.e., the > type to which the operator converts) is part of the mangled name of > the function. If the conversion operator is a member template, the > result type will appear before the template parameters. There may be > forward references in the result type to the template parameters." >
Ping. Jason, do you still think the special-case for conversion ops is inappropriate? -cary On Fri, Jul 25, 2014 at 2:16 AM, Pedro Alves <palves@redhat.com> wrote: > On 07/24/2014 11:35 PM, Cary Coutant wrote: >>> It seems that the problem here is more general; a template argument list is >>> not in scope within that same template argument list. Can't we fix that >>> without special-casing conversion ops? >> >> I think conversion ops really are a special case. > > Thanks Cary. FWIW, I agree. > > (GDB 7.8 hasn't been released yet, though it's close. If this > patch is approved as is, we'll be able to have the crash > fixed there. If this requires a significant rewrite though, > I'm afraid I might not be able to do it myself anytime soon.) > >> It's the only case >> where the template parameters refer to the template argument list from >> the cast operator's enclosing template. In a cast expression, like >> anywhere else you might have a template parameter, the template >> parameter refers to the template argument list of the immediately >> enclosing template. >> >> I think this note from Section 5.1.3 (Operator Encodings) of the ABI >> is what makes this a special case (it's an informative comment in the >> document, but seems to me to be normative): >> >> "For a user-defined conversion operator the result type (i.e., the >> type to which the operator converts) is part of the mangled name of >> the function. If the conversion operator is a member template, the >> result type will appear before the template parameters. There may be >> forward references in the result type to the template parameters." >> > > -- > Thanks, > Pedro Alves >
Ping. I'm getting more reports of this bug internally, and it would be nice to have the fix upstream. -cary On Mon, Oct 13, 2014 at 11:43 AM, Cary Coutant <ccoutant@google.com> wrote: > Ping. Jason, do you still think the special-case for conversion ops is > inappropriate? > > -cary > > > On Fri, Jul 25, 2014 at 2:16 AM, Pedro Alves <palves@redhat.com> wrote: >> On 07/24/2014 11:35 PM, Cary Coutant wrote: >>>> It seems that the problem here is more general; a template argument list is >>>> not in scope within that same template argument list. Can't we fix that >>>> without special-casing conversion ops? >>> >>> I think conversion ops really are a special case. >> >> Thanks Cary. FWIW, I agree. >> >> (GDB 7.8 hasn't been released yet, though it's close. If this >> patch is approved as is, we'll be able to have the crash >> fixed there. If this requires a significant rewrite though, >> I'm afraid I might not be able to do it myself anytime soon.) >> >>> It's the only case >>> where the template parameters refer to the template argument list from >>> the cast operator's enclosing template. In a cast expression, like >>> anywhere else you might have a template parameter, the template >>> parameter refers to the template argument list of the immediately >>> enclosing template. >>> >>> I think this note from Section 5.1.3 (Operator Encodings) of the ABI >>> is what makes this a special case (it's an informative comment in the >>> document, but seems to me to be normative): >>> >>> "For a user-defined conversion operator the result type (i.e., the >>> type to which the operator converts) is part of the mangled name of >>> the function. If the conversion operator is a member template, the >>> result type will appear before the template parameters. There may be >>> forward references in the result type to the template parameters." >>> >> >> -- >> Thanks, >> Pedro Alves >>
Sorry I forgot about this for so long. The patch is OK. Jason
diff --git a/include/demangle.h b/include/demangle.h index bbad71b..7dc2648 100644 --- a/include/demangle.h +++ b/include/demangle.h @@ -373,6 +373,10 @@ enum demangle_component_type /* A typecast, represented as a unary operator. The one subtree is the type to which the argument should be cast. */ DEMANGLE_COMPONENT_CAST, + /* A conversion operator, represented as a unary operator. The one + subtree is the type to which the argument should be converted + to. */ + DEMANGLE_COMPONENT_CONVERSION, /* A nullary expression. The left subtree is the operator. */ DEMANGLE_COMPONENT_NULLARY, /* A unary expression. The left subtree is the operator, and the diff --git a/libiberty/cp-demangle.c b/libiberty/cp-demangle.c index c0d2ffe..a86e8aa 100644 --- a/libiberty/cp-demangle.c +++ b/libiberty/cp-demangle.c @@ -538,8 +538,10 @@ d_print_array_type (struct d_print_info *, int, static void d_print_expr_op (struct d_print_info *, int, const struct demangle_component *); -static void -d_print_cast (struct d_print_info *, int, const struct demangle_component *); +static void d_print_cast (struct d_print_info *, int, + const struct demangle_component *); +static void d_print_conversion (struct d_print_info *, int, + const struct demangle_component *); static int d_demangle_callback (const char *, int, demangle_callbackref, void *); @@ -727,6 +729,9 @@ d_dump (struct demangle_component *dc, int indent) case DEMANGLE_COMPONENT_CAST: printf ("cast\n"); break; + case DEMANGLE_COMPONENT_CONVERSION: + printf ("conversion operator\n"); + break; case DEMANGLE_COMPONENT_NULLARY: printf ("nullary operator\n"); break; @@ -936,6 +941,7 @@ d_make_comp (struct d_info *di, enum demangle_component_type type, case DEMANGLE_COMPONENT_IMAGINARY: case DEMANGLE_COMPONENT_VENDOR_TYPE: case DEMANGLE_COMPONENT_CAST: + case DEMANGLE_COMPONENT_CONVERSION: case DEMANGLE_COMPONENT_JAVA_RESOURCE: case DEMANGLE_COMPONENT_DECLTYPE: case DEMANGLE_COMPONENT_PACK_EXPANSION: @@ -1227,7 +1233,7 @@ is_ctor_dtor_or_conversion (struct demangle_component *dc) return is_ctor_dtor_or_conversion (d_right (dc)); case DEMANGLE_COMPONENT_CTOR: case DEMANGLE_COMPONENT_DTOR: - case DEMANGLE_COMPONENT_CAST: + case DEMANGLE_COMPONENT_CONVERSION: return 1; } } @@ -1783,11 +1789,16 @@ d_operator_name (struct d_info *di) { struct demangle_component *type; int was_conversion = di->is_conversion; + struct demangle_component *res; di->is_conversion = ! di->is_expression; type = cplus_demangle_type (di); + if (di->is_conversion) + res = d_make_comp (di, DEMANGLE_COMPONENT_CONVERSION, type, NULL); + else + res = d_make_comp (di, DEMANGLE_COMPONENT_CAST, type, NULL); di->is_conversion = was_conversion; - return d_make_comp (di, DEMANGLE_COMPONENT_CAST, type, NULL); + return res; } else { @@ -3881,6 +3892,7 @@ d_count_templates_scopes (int *num_templates, int *num_scopes, case DEMANGLE_COMPONENT_TEMPLATE_ARGLIST: case DEMANGLE_COMPONENT_INITIALIZER_LIST: case DEMANGLE_COMPONENT_CAST: + case DEMANGLE_COMPONENT_CONVERSION: case DEMANGLE_COMPONENT_NULLARY: case DEMANGLE_COMPONENT_UNARY: case DEMANGLE_COMPONENT_BINARY: @@ -5001,9 +5013,9 @@ d_print_comp_inner (struct d_print_info *dpi, int options, d_print_comp (dpi, options, dc->u.s_extended_operator.name); return; - case DEMANGLE_COMPONENT_CAST: + case DEMANGLE_COMPONENT_CONVERSION: d_append_string (dpi, "operator "); - d_print_cast (dpi, options, dc); + d_print_conversion (dpi, options, dc); return; case DEMANGLE_COMPONENT_NULLARY: @@ -5736,11 +5748,20 @@ d_print_expr_op (struct d_print_info *dpi, int options, static void d_print_cast (struct d_print_info *dpi, int options, - const struct demangle_component *dc) + const struct demangle_component *dc) +{ + d_print_comp (dpi, options, d_left (dc)); +} + +/* Print a conversion operator. */ + +static void +d_print_conversion (struct d_print_info *dpi, int options, + const struct demangle_component *dc) { struct d_print_template dpt; - /* For a cast operator, we need the template parameters from + /* For a conversion operator, we need the template parameters from the enclosing template in scope for processing the type. */ if (dpi->current_template != NULL) { diff --git a/libiberty/cp-demint.c b/libiberty/cp-demint.c index 1d1a77a..efcc5b7 100644 --- a/libiberty/cp-demint.c +++ b/libiberty/cp-demint.c @@ -110,6 +110,7 @@ cplus_demangle_fill_component (struct demangle_component *p, case DEMANGLE_COMPONENT_IMAGINARY: case DEMANGLE_COMPONENT_VENDOR_TYPE: case DEMANGLE_COMPONENT_CAST: + case DEMANGLE_COMPONENT_CONVERSION: if (right != NULL) return 0; break; diff --git a/libiberty/testsuite/demangle-expected b/libiberty/testsuite/demangle-expected index 864ee7e..723c632 100644 --- a/libiberty/testsuite/demangle-expected +++ b/libiberty/testsuite/demangle-expected @@ -4348,3 +4348,26 @@ void post<std::function<void ()> >(std::function<void ()>&&)::{lambda()#1}*& std _QueueNotification_QueueController__$4PPPPPPPM_A_INotice___Z _QueueNotification_QueueController__$4PPPPPPPM_A_INotice___Z _QueueNotification_QueueController__$4PPPPPPPM_A_INotice___Z +# +# These two are from gcc PR61321, and gcc PR61233 / gdb PR16957 +# +--format=gnu-v3 +_Z13function_tempIiEv1AIXszcvT_Li999EEE +void function_temp<int>(A<sizeof ((int)(999))>) +# +--format=gnu-v3 +_Z7ZipWithI7QStringS0_5QListZN4oral6detail16AdaptCreateTableI7AccountEES0_RKNS3_16CachedFieldsDataEEUlRKS0_SA_E_ET1_IDTclfp1_cvT__EcvT0__EEEERKT1_ISC_ERKT1_ISD_ET2_ +QList<decltype ({parm#3}((QString)(), (QString)()))> ZipWith<QString, QString, QList, QString oral::detail::AdaptCreateTable<Account>(oral::detail::CachedFieldsData const&)::{lambda(QString const&, QString const&)#1}>(QList<QString oral::detail::AdaptCreateTable<Account>(oral::detail::CachedFieldsData const&)::{lambda(QString const&, QString const&)#1}> const&, QList<QList> const&, QString oral::detail::AdaptCreateTable<Account>(oral::detail::CachedFieldsData const&)::{lambda(QString const&, QString const&)#1}) +# +# These three are symbols generated by g++'s testsuite, which triggered the same bug as above. +--format=gnu-v3 +_Z14int_if_addableI1YERiP1AIXszpldecvPT_Li0EdecvS4_Li0EEE +int& int_if_addable<Y>(A<sizeof ((*((Y*)(0)))+(*((Y*)(0))))>*) +# +--format=gnu-v3 +_Z3bazIiEvP1AIXszcl3foocvT__ELCf00000000_00000000EEEE +void baz<int>(A<sizeof (foo((int)(), (floatcomplex )00000000_00000000))>*) +# +--format=gnu-v3 +_Z3fooI1FEN1XIXszdtcl1PclcvT__EEE5arrayEE4TypeEv +X<sizeof ((P(((F)())())).array)>::Type foo<F>()
The fix for bug 59195: [C++ demangler handles conversion operator incorrectly] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59195 unfortunately makes the demangler crash due to infinite recursion, in case of casts in template parameters. For example, with: template<int> struct A {}; template <typename Y> void function_temp(A<sizeof ((Y)(999))>) {} template void function_temp<int>(A<sizeof (int)>); The 'function_temp<int>' instantiation above mangles to: _Z13function_tempIiEv1AIXszcvT_Li999EEE The demangler parses this as: typed name template name 'function_temp' template argument list builtin type int function type builtin type void argument list template (*) name 'A' template argument list unary operator operator sizeof unary operator cast template parameter 0 (**) literal builtin type int name '999' And after the fix for 59195, due to: static void d_print_cast (struct d_print_info *dpi, int options, const struct demangle_component *dc) { ... /* For a cast operator, we need the template parameters from the enclosing template in scope for processing the type. */ if (dpi->current_template != NULL) { dpt.next = dpi->templates; dpi->templates = &dpt; dpt.template_decl = dpi->current_template; } when printing the template argument list of A (what should be "<sizeof (int)>"), the template parameter 0 (that is, "T_", the '**' above) now refers to the first parameter of the the template argument list of the 'A' template (the '*' above), exactly what we were already trying to print. This leads to infinite recursion, and stack exaustion. The template parameter 0 should actually refer to the first parameter of the 'function_temp' template. Where it reads "for the cast operator" in the comment in d_print_cast (above), it's really talking about a conversion operator, like: struct A { template <typename U> explicit operator U(); }; We don't want to inject the template parameters from the enclosing template in scope when processing a cast _expression_, only when handling a conversion operator. The problem is that DEMANGLE_COMPONENT_CAST is currently ambiguous, and means _both_ 'conversion operator' and 'cast expression'. Fix this by adding a new DEMANGLE_COMPONENT_CONVERSION component type, which does what DEMANGLE_COMPONENT_CAST does today, and making DEMANGLE_COMPONENT_CAST just simply print its component subtree. I think we could instead reuse DEMANGLE_COMPONENT_CAST and in d_print_comp_inner still do: @@ -5001,9 +5013,9 @@ d_print_comp_inner (struct d_print_info *dpi, int options, d_print_comp (dpi, options, dc->u.s_extended_operator.name); return; case DEMANGLE_COMPONENT_CAST: d_append_string (dpi, "operator "); - d_print_cast (dpi, options, dc); + d_print_conversion (dpi, options, dc); return; leaving the unary cast case below calling d_print_cast, but seems to me that spliting the component types makes it easier to reason about the code. g++'s testsuite actually generates three symbols that crash the demangler in the same way. I've added those as tests in the demangler testsuite as well. And then this fixes PR other/61233 too, which happens to be a demangler crash originally reported to GDB, at: https://sourceware.org/bugzilla/show_bug.cgi?id=16957 Bootstrapped and regtested on x86_64 Fedora 20. Also ran this through GDB's testsuite. GDB will require a small update to use DEMANGLE_COMPONENT_CONVERSION in one place it's using DEMANGLE_COMPONENT_CAST in its sources. libiberty/ 2014-05-27 Pedro Alves <palves@redhat.com> PR other/61321 PR other/61233 * demangle.h (enum demangle_component_type) <DEMANGLE_COMPONENT_CONVERSION>: New value. * cp-demangle.c (d_demangle_callback, d_make_comp): Handle DEMANGLE_COMPONENT_CONVERSION. (is_ctor_dtor_or_conversion): Handle DEMANGLE_COMPONENT_CONVERSION instead of DEMANGLE_COMPONENT_CAST. (d_operator_name): Return a DEMANGLE_COMPONENT_CONVERSION component if handling a conversion. (d_count_templates_scopes, d_print_comp_inner): Handle DEMANGLE_COMPONENT_CONVERSION. (d_print_comp_inner): Handle DEMANGLE_COMPONENT_CONVERSION instead of DEMANGLE_COMPONENT_CAST. (d_print_cast): Rename as ... (d_print_conversion): ... this. Adjust comments. (d_print_cast): Rewrite - simply print the left subcomponent. * cp-demint.c (cplus_demangle_fill_component): Handle DEMANGLE_COMPONENT_CONVERSION. * testsuite/demangle-expected: Add tests. --- include/demangle.h | 4 ++++ libiberty/cp-demangle.c | 37 +++++++++++++++++++++++++++-------- libiberty/cp-demint.c | 1 + libiberty/testsuite/demangle-expected | 23 ++++++++++++++++++++++ 4 files changed, 57 insertions(+), 8 deletions(-)