| Message ID | 20120508001002.B3183160A7C@sterling.mtv.corp.google.com |
|---|---|
| State | New |
| Headers | show |
On Mon, May 7, 2012 at 7:10 PM, Sterling Augustine <saugustine@google.com> wrote: > This is the second in the series of patches to make c decl pretty printing > more closely match the demangler. A full explanation is here: > > http://gcc.gnu.org/ml/gcc-patches/2012-05/msg00512.html > > OK for mainline? Now I realize something that is wrong with the previous patch. Writing 'const T*' in C++ is very much wide spread style is also the style used in the C++ standard, TC++PL (the de facto popular reference TC++PL), our own C++ standard library implementation, and many popular modern C++ textbooks. It is a strongly well established style. Changing the pretty printer to satisfy the demangler as opposed to users look wrong headed. -- Gaby > > Sterling > > 2012-05-07 Sterling Augustine <saugustine@google.com> > > * c-family/c-pretty-print.c (pp_c_specifier_qualifier_list): Move > call to pp_c_type_qualifier_list from start to end of function. > * cp/error.c (dump_decl): Change "{anonymous}" to "(anonymous > namespace)" > (lang_decl_name): Handle "(anonymous namespace)" correctly. > > Index: c-family/c-pretty-print.c > =================================================================== > --- c-family/c-pretty-print.c (revision 187271) > +++ c-family/c-pretty-print.c (working copy) > @@ -446,8 +446,6 @@ pp_c_specifier_qualifier_list (c_pretty_printer *p > { > const enum tree_code code = TREE_CODE (t); > > - if (TREE_CODE (t) != POINTER_TYPE) > - pp_c_type_qualifier_list (pp, t); > switch (code) > { > case REFERENCE_TYPE: > @@ -494,6 +492,8 @@ pp_c_specifier_qualifier_list (c_pretty_printer *p > pp_simple_type_specifier (pp, t); > break; > } > + if (TREE_CODE (t) != POINTER_TYPE) > + pp_c_type_qualifier_list (pp, t); > } > > /* parameter-type-list: > Index: cp/error.c > =================================================================== > --- cp/error.c (revision 187271) > +++ cp/error.c (working copy) > @@ -1028,7 +1028,7 @@ dump_decl (tree t, int flags) > dump_scope (CP_DECL_CONTEXT (t), flags); > flags &= ~TFF_UNQUALIFIED_NAME; > if (DECL_NAME (t) == NULL_TREE) > - pp_cxx_ws_string (cxx_pp, M_("{anonymous}")); > + pp_cxx_ws_string (cxx_pp, M_("(anonymous namespace)")); > else > pp_cxx_tree_identifier (cxx_pp, DECL_NAME (t)); > } > @@ -2596,6 +2596,8 @@ lang_decl_name (tree decl, int v, bool translate) > > if (TREE_CODE (decl) == FUNCTION_DECL) > dump_function_name (decl, TFF_PLAIN_IDENTIFIER); > + else if (DECL_NAME (decl) == NULL && TREE_CODE (decl) == NAMESPACE_DECL) > + pp_string (cxx_pp, M_("(anonymous namespace)")); > else > dump_decl (DECL_NAME (decl), TFF_PLAIN_IDENTIFIER); > > > -- > This patch is available for review at http://codereview.appspot.com/6195056
On Mon, May 7, 2012 at 6:44 PM, Gabriel Dos Reis <gdr@integrable-solutions.net> wrote: > On Mon, May 7, 2012 at 7:10 PM, Sterling Augustine > <saugustine@google.com> wrote: >> This is the second in the series of patches to make c decl pretty printing >> more closely match the demangler. A full explanation is here: >> >> http://gcc.gnu.org/ml/gcc-patches/2012-05/msg00512.html >> >> OK for mainline? > > Now I realize something that is wrong with the previous patch. > Writing 'const T*' in C++ is very much wide spread style is also the > style used in the C++ standard, TC++PL (the de facto popular reference TC++PL), > our own C++ standard library implementation, and many popular modern > C++ textbooks. > It is a strongly well established style. > Changing the pretty printer to satisfy the demangler as opposed to users > look wrong headed. I'm most definitely not trying to satisfy the demangler--I'm trying to make GCC's naming consistent with the rest of the tool chain, which will be good for users. Consider the C++ function: int foo(const char *bar); The problem is that the toolchain disagrees on the canonical pretty-name of this function. If you use nm, objdump, or readelf, or anything that must recover the name from the binary, you will see: int foo(char const*) The demangler follows the documented gnu_v3 demangling convention. As far as I can tell, the C++ front end is ad-hoc. This disagreement creates inconsistencies in the debugging information that is confusing to users. Do you have a suggestion for fixing the disagreement? I would love to add this as a parameter somewhere, but the decision is very deep in the internals of the pretty printer. Sterling
On Tue, May 8, 2012 at 11:20 AM, Sterling Augustine <saugustine@google.com> wrote: > Do you have a suggestion for fixing the disagreement? I would love to > add this as a parameter somewhere, but the decision is very deep in > the internals of the pretty printer. I disagree with your characterization that the pretty-printer is ad-hoc. Since its inception, I chose to follow closely the established C++ style as previously explained. The pretty printer was designed as a tool for diagnostics to users, not for the demangler. If the demangler happens to find some of its functionalities useful, the proper action would be to customize it as opposed to a whole change to satisfy its internals. A way of doing this is to have the pretty-printer object initialized with a style (e.g. standard C++ style, gdb style, etc.). The places you want to change should be predicated on the current style matching gdb style, etc. -- Gaby
On 8 May 2012 18:20, Sterling Augustine <saugustine@google.com> wrote: > On Mon, May 7, 2012 at 6:44 PM, Gabriel Dos Reis > <gdr@integrable-solutions.net> wrote: >> On Mon, May 7, 2012 at 7:10 PM, Sterling Augustine >> <saugustine@google.com> wrote: >>> This is the second in the series of patches to make c decl pretty printing >>> more closely match the demangler. A full explanation is here: >>> >>> http://gcc.gnu.org/ml/gcc-patches/2012-05/msg00512.html >>> >>> OK for mainline? >> >> Now I realize something that is wrong with the previous patch. >> Writing 'const T*' in C++ is very much wide spread style is also the >> style used in the C++ standard, TC++PL (the de facto popular reference TC++PL), >> our own C++ standard library implementation, and many popular modern >> C++ textbooks. >> It is a strongly well established style. >> Changing the pretty printer to satisfy the demangler as opposed to users >> look wrong headed. > > I'm most definitely not trying to satisfy the demangler--I'm trying to > make GCC's naming consistent with the rest of the tool chain, which > will be good for users. Consider the C++ function: > > int foo(const char *bar); > > The problem is that the toolchain disagrees on the canonical > pretty-name of this function. If you use nm, objdump, or readelf, or > anything that must recover the name from the binary, you will see: > > int foo(char const*) > > The demangler follows the documented gnu_v3 demangling convention. As > far as I can tell, the C++ front end is ad-hoc. This disagreement > creates inconsistencies in the debugging information that is confusing > to users. > > Do you have a suggestion for fixing the disagreement? I would love to > add this as a parameter somewhere, but the decision is very deep in > the internals of the pretty printer. A suggestion: Make dwarf_name call the demangler, and then a (new?) a function that converts a mangled decl to a human-readable string. In any case, the pretty-printer does a lot of stuff that is mostly useless for just printing a declaration (translation, wrapping, etc.). Bonus point if all GNU toolchain program use the same functions for demangling and undemagling (because I guess they actually don't, no?) I guess it cannot be so easy, so I apologize in advance for saying nonsense. Cheers, Manuel.
On Tue, May 8, 2012 at 4:38 PM, Manuel López-Ibáñez <lopezibanez@gmail.com> wrote: > A suggestion: Make dwarf_name call the demangler, and then a (new?) a > function that converts a mangled decl to a human-readable string. In > any case, the pretty-printer does a lot of stuff that is mostly > useless for just printing a declaration (translation, wrapping, etc.). > > Bonus point if all GNU toolchain program use the same functions for > demangling and undemagling (because I guess they actually don't, no?) > > I guess it cannot be so easy, so I apologize in advance for saying nonsense. It makes sense; and I don't think it is as complicated as it might sound. -- Gaby
>> A suggestion: Make dwarf_name call the demangler, and then a (new?) a >> function that converts a mangled decl to a human-readable string. In >> any case, the pretty-printer does a lot of stuff that is mostly >> useless for just printing a declaration (translation, wrapping, etc.). >> >> Bonus point if all GNU toolchain program use the same functions for >> demangling and undemagling (because I guess they actually don't, no?) >> >> I guess it cannot be so easy, so I apologize in advance for saying nonsense. > > It makes sense; and I don't think it is as complicated as it might sound. dwarf_name takes a tree; the demangler takes a mangled name. We don't have mangled names for many of the names we want to enter into the pubnames table. -cary
On 9 May 2012 01:34, Cary Coutant <ccoutant@google.com> wrote: >>> A suggestion: Make dwarf_name call the demangler, and then a (new?) a >>> function that converts a mangled decl to a human-readable string. In >>> any case, the pretty-printer does a lot of stuff that is mostly >>> useless for just printing a declaration (translation, wrapping, etc.). >>> >>> Bonus point if all GNU toolchain program use the same functions for >>> demangling and undemagling (because I guess they actually don't, no?) >>> >>> I guess it cannot be so easy, so I apologize in advance for saying nonsense. >> >> It makes sense; and I don't think it is as complicated as it might sound. > > dwarf_name takes a tree; the demangler takes a mangled name. We don't > have mangled names for many of the names we want to enter into the > pubnames table. Sorry that was a typo. My proposal is to mangle the tree (GCC already does that) and then demangle it to a human-readable string (GCC also does that already, no? Or at least GDB and other programs have to do that anyway, no? In any case, the demangling code should be shared and, hence, consistent.) For the names that the mangler/demangler do not support, I guess it doesn't make sense to extend the demangler, so you will need to handle them explicitly. You may call then the pretty-printer, since anyway GDB and the other programs cannot demangle those names, so whatever is used by GCC should be fine. Or implement a custom pretty-printer for them. A custom pretty-printer will be for sure much simpler than the diagnostics pretty-printer, since you don't have to worry about all kinds of trees, wrapping, inheritance, i18n, etc. Probably I am missing some details that may make the plan above infeasible. In any case, I think your intention to make the output of the different toolchain programs more consistent is worthwhile. If dwarf_name has to use the pretty-printer, then I'd prefer that you changed the demangler to match the diagnostics output, rather than the other way around. Also, I am no maintainer, so these are merely suggestions that you may freely ignore. :-) Cheers, Manuel..
Index: c-family/c-pretty-print.c =================================================================== --- c-family/c-pretty-print.c (revision 187271) +++ c-family/c-pretty-print.c (working copy) @@ -446,8 +446,6 @@ pp_c_specifier_qualifier_list (c_pretty_printer *p { const enum tree_code code = TREE_CODE (t); - if (TREE_CODE (t) != POINTER_TYPE) - pp_c_type_qualifier_list (pp, t); switch (code) { case REFERENCE_TYPE: @@ -494,6 +492,8 @@ pp_c_specifier_qualifier_list (c_pretty_printer *p pp_simple_type_specifier (pp, t); break; } + if (TREE_CODE (t) != POINTER_TYPE) + pp_c_type_qualifier_list (pp, t); } /* parameter-type-list: Index: cp/error.c =================================================================== --- cp/error.c (revision 187271) +++ cp/error.c (working copy) @@ -1028,7 +1028,7 @@ dump_decl (tree t, int flags) dump_scope (CP_DECL_CONTEXT (t), flags); flags &= ~TFF_UNQUALIFIED_NAME; if (DECL_NAME (t) == NULL_TREE) - pp_cxx_ws_string (cxx_pp, M_("{anonymous}")); + pp_cxx_ws_string (cxx_pp, M_("(anonymous namespace)")); else pp_cxx_tree_identifier (cxx_pp, DECL_NAME (t)); } @@ -2596,6 +2596,8 @@ lang_decl_name (tree decl, int v, bool translate) if (TREE_CODE (decl) == FUNCTION_DECL) dump_function_name (decl, TFF_PLAIN_IDENTIFIER); + else if (DECL_NAME (decl) == NULL && TREE_CODE (decl) == NAMESPACE_DECL) + pp_string (cxx_pp, M_("(anonymous namespace)")); else dump_decl (DECL_NAME (decl), TFF_PLAIN_IDENTIFIER);
This is the second in the series of patches to make c decl pretty printing more closely match the demangler. A full explanation is here: http://gcc.gnu.org/ml/gcc-patches/2012-05/msg00512.html OK for mainline? Sterling 2012-05-07 Sterling Augustine <saugustine@google.com> * c-family/c-pretty-print.c (pp_c_specifier_qualifier_list): Move call to pp_c_type_qualifier_list from start to end of function. * cp/error.c (dump_decl): Change "{anonymous}" to "(anonymous namespace)" (lang_decl_name): Handle "(anonymous namespace)" correctly. -- This patch is available for review at http://codereview.appspot.com/6195056