Patchwork [C++,and,pubnames,2/2] Adjust c decl pretty printer to match demangler (issue6195056)

login
register
mail settings
Submitter Sterling Augustine
Date May 8, 2012, 12:10 a.m.
Message ID <20120508001002.B3183160A7C@sterling.mtv.corp.google.com>
Download mbox | patch
Permalink /patch/157492/
State New
Headers show

Comments

Sterling Augustine - May 8, 2012, 12:10 a.m.
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
Gabriel Dos Reis - May 8, 2012, 1:44 a.m.
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
Sterling Augustine - May 8, 2012, 4:20 p.m.
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
Gabriel Dos Reis - May 8, 2012, 4:28 p.m.
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
Manuel López-Ibáñez - May 8, 2012, 9:38 p.m.
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.
Gabriel Dos Reis - May 8, 2012, 11:18 p.m.
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
Cary Coutant - May 8, 2012, 11:34 p.m.
>> 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
Manuel López-Ibáñez - May 9, 2012, 7:08 a.m.
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..

Patch

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