* [PATCH] Fix printing destructors with void in parameters for clang programs @ 2022-09-23 15:50 Bruno Larsen 2022-09-23 23:00 ` Tom de Vries 0 siblings, 1 reply; 4+ messages in thread From: Bruno Larsen @ 2022-09-23 15:50 UTC (permalink / raw) To: gdb-patches When GDB prints the methods of a C++ class, it will always skip the first parameter, assuming it to be a 'this' pointer. However, when deciding if it should print "void" for the parameters, it disregards the fact that the first parameter was skipped, so if the method only had that parameter, for instance how clang compiles destructors, we'd see ~foo(void) instead of ~foo(). Fix this behavior by explicitly testing if there were 0 arguments. --- There is another possibility for a fix, which is to stop ignoring the first parameter, but there is a comment at that part of the code that says "some compilers may not support artificial parameters". I'm not sure how true that still is, and I would certainly like a solution like that more, since (as keiths points out in here: https://sourceware.org/bugzilla/show_bug.cgi?id=8218) there is no actual rule saying that compilers must use an artificial 'this' as the first parameter. If anyone knows, please share with the class :) --- gdb/c-typeprint.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gdb/c-typeprint.c b/gdb/c-typeprint.c index 3a611cdac5d..8e05bdc81fe 100644 --- a/gdb/c-typeprint.c +++ b/gdb/c-typeprint.c @@ -300,7 +300,7 @@ cp_type_print_method_args (struct type *mtype, const char *prefix, } else if (varargs) gdb_printf (stream, "..."); - else if (language == language_cplus) + else if (language == language_cplus && nargs == 0) gdb_printf (stream, "void"); gdb_printf (stream, ")"); -- 2.37.3 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Fix printing destructors with void in parameters for clang programs 2022-09-23 15:50 [PATCH] Fix printing destructors with void in parameters for clang programs Bruno Larsen @ 2022-09-23 23:00 ` Tom de Vries 2022-09-26 8:29 ` Bruno Larsen 0 siblings, 1 reply; 4+ messages in thread From: Tom de Vries @ 2022-09-23 23:00 UTC (permalink / raw) To: Bruno Larsen, gdb-patches [-- Attachment #1: Type: text/plain, Size: 3033 bytes --] On 9/23/22 17:50, Bruno Larsen via Gdb-patches wrote: > When GDB prints the methods of a C++ class, it will always skip the first > parameter, assuming it to be a 'this' pointer. However, when deciding if > it should print "void" for the parameters, it disregards the fact that > the first parameter was skipped, so if the method only had that > parameter, for instance how clang compiles destructors, we'd see > ~foo(void) instead of ~foo(). > Hi, I wrote the following test-case to investigate the problem: ... $ cat test.cc class A { public: int a; A() { this->a = 3; } ~A() { a = 0; } }; int main (void) { A a; return a.a; } $ g++ test.cc -g $ ./a.out; echo $? 3 ... With g++ (7.5.0) I see: ... $ gdb -q -batch a.out -ex "ptype A" type = class A { public: int a; A(void); ~A(); } ... and with clang++ (13.0.1): ... $ gdb -q -batch a.out -ex "ptype A" type = class A { public: int a; A(void); ~A(void); } ... So, ok, I see how the patch makes the output for clang++ and g++ the same, getting us "A()" and "~A()". I don't have a preference of say ~A::A() vs A::~A(void), but I suppose the former is the newer, more c++-like style, and the latter leaves less room for confusion. Do you have a preference, or were you merely trying to make the output for the destructor the same for both cases? [ But now look at the expression parsing side. This does not get us anything, with both g++ and clang++: ... $ gdb -q -batch a.out -ex "p A::A()" -ex "p A::~A()" Cannot resolve method A::A to any overloaded instance Cannot resolve method A::~A to any overloaded instance ... but again with both g++ and clang++ (and with and without your patch): ... $ gdb -q -batch a.out -ex "p A::A(void)" -ex "p A::~A(void)" $1 = {void (A * const)} 0x4004f4 <A::A()> $2 = {void (A * const)} 0x40050a <A::~A()> ... So, I wonder if the expression parsing could be improved to handle the "A (void) == A ()" equivalence. But that aside. ] Anyway, an interesting detail is that the g++-generated destructor was already printed without void. Investigation shows that it is due to having two artificial parameters. I think that shows that we're making decisions about how to handle similar things in different places in the code, which means it needs a bit of restructuring. > - else if (language == language_cplus) > + else if (language == language_cplus && nargs == 0) > gdb_printf (stream, "void"); I remain confused about why we're doing things differently based on language. Anyway, the nargs == 0 implies staticp, so I wonder if we should not handle a static function with implicit first argument the same (not sure if that can happen, but even so, maybe we don't want to use that assumption into the code), in which case the nargs == 0 doesn't address that scenario. I tried rewriting the code in a more regular for loop structure, in attached patch, not fully tested yet but passes at least gdb.cp/*.exp for me. WDYT ? Thanks, - Tom [-- Attachment #2: tmp.patch --] [-- Type: text/x-patch, Size: 3282 bytes --] diff --git a/gdb/c-typeprint.c b/gdb/c-typeprint.c index 3a611cdac5d..e17a4ccfa1e 100644 --- a/gdb/c-typeprint.c +++ b/gdb/c-typeprint.c @@ -273,35 +273,46 @@ cp_type_print_method_args (struct type *mtype, const char *prefix, language_cplus, DMGL_ANSI); gdb_puts ("(", stream); - /* Skip the class variable. We keep this here to accommodate older - compilers and debug formats which may not support artificial - parameters. */ - i = staticp ? 0 : 1; - if (nargs > i) + int printed_args = 0; + for (i = 0; i < nargs; ++i) { - while (i < nargs) + if (i == 0 && !staticp) { - struct field arg = args[i++]; - - /* Skip any artificial arguments. */ - if (FIELD_ARTIFICIAL (arg)) - continue; + /* Skip the class variable. We keep this here to accommodate older + compilers and debug formats which may not support artificial + parameters. */ + continue; + } - c_print_type (arg.type (), "", stream, 0, 0, language, flags); + struct field arg = args[i]; + /* Skip any artificial arguments. */ + if (FIELD_ARTIFICIAL (arg)) + continue; - if (i == nargs && varargs) - gdb_printf (stream, ", ..."); - else if (i < nargs) - { - gdb_printf (stream, ", "); - stream->wrap_here (8); - } + if (printed_args > 0) + { + gdb_printf (stream, ", "); + stream->wrap_here (8); } + + c_print_type (arg.type (), "", stream, 0, 0, language, flags); + printed_args++; + } + + if (varargs) + { + if (printed_args == 0) + gdb_printf (stream, "..."); + else + /* Note: no wrapping done here. Is that intended? */ + gdb_printf (stream, ", ..."); + } + else if (printed_args == 0) + { + /* Why do we do this differently for non-c++? */ + if (language == language_cplus && staticp) + gdb_printf (stream, "void"); } - else if (varargs) - gdb_printf (stream, "..."); - else if (language == language_cplus) - gdb_printf (stream, "void"); gdb_printf (stream, ")"); diff --git a/gdb/testsuite/gdb.cp/classes.exp b/gdb/testsuite/gdb.cp/classes.exp index 7b8b315ac1f..7c214a57100 100644 --- a/gdb/testsuite/gdb.cp/classes.exp +++ b/gdb/testsuite/gdb.cp/classes.exp @@ -328,7 +328,7 @@ proc test_ptype_class_objects {} { "class_with_typedefs::protected_int protected_int_;" } { field protected \ "class_with_typedefs::private_int private_int_;" } - { method public "class_with_typedefs(void);" } + { method public "class_with_typedefs();" } { method public "class_with_typedefs::public_int add_public(class_with_typedefs::public_int);" } { method public \ "class_with_typedefs::public_int add_all(int);" } diff --git a/gdb/testsuite/gdb.cp/rvalue-ref-overload.exp b/gdb/testsuite/gdb.cp/rvalue-ref-overload.exp index 36342673aad..b0c555a8fa9 100644 --- a/gdb/testsuite/gdb.cp/rvalue-ref-overload.exp +++ b/gdb/testsuite/gdb.cp/rvalue-ref-overload.exp @@ -43,7 +43,7 @@ gdb_test "up" ".*main.*" "up from marker1" # Print the monster class type. cp_test_ptype_class "foo_rr_instance1" "" "class" "foo" \ { - { method public "foo(void);" } + { method public "foo();" } { method public "foo(foo_lval_ref);" } { method public "foo(foo_rval_ref);" } { method public "~foo();" } ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Fix printing destructors with void in parameters for clang programs 2022-09-23 23:00 ` Tom de Vries @ 2022-09-26 8:29 ` Bruno Larsen 2022-09-27 10:51 ` Tom de Vries 0 siblings, 1 reply; 4+ messages in thread From: Bruno Larsen @ 2022-09-26 8:29 UTC (permalink / raw) To: Tom de Vries, gdb-patches On 24/09/2022 01:00, Tom de Vries wrote: > On 9/23/22 17:50, Bruno Larsen via Gdb-patches wrote: >> When GDB prints the methods of a C++ class, it will always skip the >> first >> parameter, assuming it to be a 'this' pointer. However, when deciding if >> it should print "void" for the parameters, it disregards the fact that >> the first parameter was skipped, so if the method only had that >> parameter, for instance how clang compiles destructors, we'd see >> ~foo(void) instead of ~foo(). >> > > Hi, > > I wrote the following test-case to investigate the problem: > ... > $ cat test.cc > class A { > public: > int a; > > A() { > this->a = 3; > } > ~A() { > a = 0; > } > }; > > int > main (void) > { > A a; > > return a.a; > } > $ g++ test.cc -g > $ ./a.out; echo $? > 3 > ... > > With g++ (7.5.0) I see: > ... > $ gdb -q -batch a.out -ex "ptype A" > type = class A { > public: > int a; > > A(void); > ~A(); > } > ... > and with clang++ (13.0.1): > ... > $ gdb -q -batch a.out -ex "ptype A" > type = class A { > public: > int a; > > A(void); > ~A(void); > } > ... > > So, ok, I see how the patch makes the output for clang++ and g++ the > same, getting us "A()" and "~A()". I don't have a preference of say > ~A::A() vs A::~A(void), but I suppose the former is the newer, more > c++-like style, and the latter leaves less room for confusion. Do you > have a preference, or were you merely trying to make the output for > the destructor the same for both cases? Hi! Thanks for the testcase. Much more concise than the one I was using and forgot to mention (gdb.cp/templates.exp). As for a reasoning for the change, my personal preference is consistency, but while looking over this bug, I found this PR c++/8218 (https://sourceware.org/bugzilla/show_bug.cgi?id=8218), and seeing Keith's history with this area, I asked him, and he stated that he believes ~A(void) is definitely a bug. > > [ But now look at the expression parsing side. This does not get us > anything, with both g++ and clang++: > ... > $ gdb -q -batch a.out -ex "p A::A()" -ex "p A::~A()" > Cannot resolve method A::A to any overloaded instance > Cannot resolve method A::~A to any overloaded instance > ... > but again with both g++ and clang++ (and with and without your patch): > ... > $ gdb -q -batch a.out -ex "p A::A(void)" -ex "p A::~A(void)" > $1 = {void (A * const)} 0x4004f4 <A::A()> > $2 = {void (A * const)} 0x40050a <A::~A()> > ... > > So, I wonder if the expression parsing could be improved to handle the > "A (void) == A ()" equivalence. But that aside. ] [ That's a good point. I think this is worth fixing, especially if we start printing constructors and destructors without (void) consistently. ] > > Anyway, an interesting detail is that the g++-generated destructor was > already printed without void. Investigation shows that it is due to > having two artificial parameters. I think that shows that we're making > decisions about how to handle similar things in different places in > the code, which means it needs a bit of restructuring. Yeah, the commit that creates this behavior is mentioned in the bug I linked above. It was specifically fixing GDB printing A::A(int) when the int parameter was just artificial. > >> - else if (language == language_cplus) >> + else if (language == language_cplus && nargs == 0) >> gdb_printf (stream, "void"); > > I remain confused about why we're doing things differently based on > language. There is a difference in semantics between C and C++ in function declarations with an empty parameter list, as I found in this stack overflow post: https://softwareengineering.stackexchange.com/questions/286490/what-is-the-difference-between-function-and-functionvoid However, I don't think this distinction is all that important for the debugger, so I'm fine with removing the language check. > > Anyway, the nargs == 0 implies staticp, so I wonder if we should not > handle a static function with implicit first argument the same (not > sure if that can happen, but even so, maybe we don't want to use that > assumption into the code), in which case the nargs == 0 doesn't > address that scenario. > > I tried rewriting the code in a more regular for loop structure, in > attached patch, not fully tested yet but passes at least gdb.cp/*.exp > for me. WDYT ? I also haven't tested this, but it looks like a better solution. Feel free to make this into a proper patch, or let me know and I'll add the finishing touches. Cheers, Bruno > > Thanks, > - Tom > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Fix printing destructors with void in parameters for clang programs 2022-09-26 8:29 ` Bruno Larsen @ 2022-09-27 10:51 ` Tom de Vries 0 siblings, 0 replies; 4+ messages in thread From: Tom de Vries @ 2022-09-27 10:51 UTC (permalink / raw) To: Bruno Larsen, gdb-patches On 9/26/22 10:29, Bruno Larsen wrote: > On 24/09/2022 01:00, Tom de Vries wrote: >> On 9/23/22 17:50, Bruno Larsen via Gdb-patches wrote: >>> When GDB prints the methods of a C++ class, it will always skip the >>> first >>> parameter, assuming it to be a 'this' pointer. However, when deciding if >>> it should print "void" for the parameters, it disregards the fact that >>> the first parameter was skipped, so if the method only had that >>> parameter, for instance how clang compiles destructors, we'd see >>> ~foo(void) instead of ~foo(). >>> >> >> Hi, >> >> I wrote the following test-case to investigate the problem: >> ... >> $ cat test.cc >> class A { >> public: >> int a; >> >> A() { >> this->a = 3; >> } >> ~A() { >> a = 0; >> } >> }; >> >> int >> main (void) >> { >> A a; >> >> return a.a; >> } >> $ g++ test.cc -g >> $ ./a.out; echo $? >> 3 >> ... >> >> With g++ (7.5.0) I see: >> ... >> $ gdb -q -batch a.out -ex "ptype A" >> type = class A { >> public: >> int a; >> >> A(void); >> ~A(); >> } >> ... >> and with clang++ (13.0.1): >> ... >> $ gdb -q -batch a.out -ex "ptype A" >> type = class A { >> public: >> int a; >> >> A(void); >> ~A(void); >> } >> ... >> >> So, ok, I see how the patch makes the output for clang++ and g++ the >> same, getting us "A()" and "~A()". I don't have a preference of say >> ~A::A() vs A::~A(void), but I suppose the former is the newer, more >> c++-like style, and the latter leaves less room for confusion. Do you >> have a preference, or were you merely trying to make the output for >> the destructor the same for both cases? > > Hi! > > Thanks for the testcase. Much more concise than the one I was using and > forgot to mention (gdb.cp/templates.exp). > > As for a reasoning for the change, my personal preference is > consistency, but while looking over this bug, I found this PR c++/8218 > (https://sourceware.org/bugzilla/show_bug.cgi?id=8218), and seeing > Keith's history with this area, I asked him, and he stated that he > believes ~A(void) is definitely a bug. > Since you're after consistency, I've submitted a patch ( https://sourceware.org/pipermail/gdb-patches/2022-September/192155.html ) that achieves that, by printing ~A(void). I'm not sure in what sense that's a bug, but we'll have that discussion there. >> >> [ But now look at the expression parsing side. This does not get us >> anything, with both g++ and clang++: >> ... >> $ gdb -q -batch a.out -ex "p A::A()" -ex "p A::~A()" >> Cannot resolve method A::A to any overloaded instance >> Cannot resolve method A::~A to any overloaded instance >> ... >> but again with both g++ and clang++ (and with and without your patch): >> ... >> $ gdb -q -batch a.out -ex "p A::A(void)" -ex "p A::~A(void)" >> $1 = {void (A * const)} 0x4004f4 <A::A()> >> $2 = {void (A * const)} 0x40050a <A::~A()> >> ... >> >> So, I wonder if the expression parsing could be improved to handle the >> "A (void) == A ()" equivalence. But that aside. ] > [ That's a good point. I think this is worth fixing, especially if we > start printing constructors and destructors without (void) consistently. ] >> >> Anyway, an interesting detail is that the g++-generated destructor was >> already printed without void. Investigation shows that it is due to >> having two artificial parameters. I think that shows that we're making >> decisions about how to handle similar things in different places in >> the code, which means it needs a bit of restructuring. > Yeah, the commit that creates this behavior is mentioned in the bug I > linked above. It was specifically fixing GDB printing A::A(int) when the > int parameter was just artificial. >> >>> - else if (language == language_cplus) >>> + else if (language == language_cplus && nargs == 0) >>> gdb_printf (stream, "void"); >> >> I remain confused about why we're doing things differently based on >> language. > > There is a difference in semantics between C and C++ in function > declarations with an empty parameter list, as I found in this stack > overflow post: > https://softwareengineering.stackexchange.com/questions/286490/what-is-the-difference-between-function-and-functionvoid > There is indeed. All the more reason to print void when language is C. > > However, I don't think this distinction is all that important for the > debugger, so I'm fine with removing the language check. > After pondering it a bit more, I realized it could be to avoid void for languages that do not contain the keyword. >> >> Anyway, the nargs == 0 implies staticp, so I wonder if we should not >> handle a static function with implicit first argument the same (not >> sure if that can happen, but even so, maybe we don't want to use that >> assumption into the code), in which case the nargs == 0 doesn't >> address that scenario. >> >> I tried rewriting the code in a more regular for loop structure, in >> attached patch, not fully tested yet but passes at least gdb.cp/*.exp >> for me. WDYT ? > I also haven't tested this, but it looks like a better solution. Feel > free to make this into a proper patch, or let me know and I'll add the > finishing touches. So, I'm hoping that I get aforementioned patch committed, which should hopefully address your consistency concern. Thanks, - Tom ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-09-27 10:51 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-09-23 15:50 [PATCH] Fix printing destructors with void in parameters for clang programs Bruno Larsen 2022-09-23 23:00 ` Tom de Vries 2022-09-26 8:29 ` Bruno Larsen 2022-09-27 10:51 ` Tom de Vries
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).