| Message ID | CABOHX+c0+hrO=4ENJAgU7thqvB1_qk=dBNYe5aDcSVsQ2makbg@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On 01/03/2017 06:02 PM, Iain Buclaw wrote: > This patch fixes a crash found on "p *(type *)0x1234" when using "set > debug expression 1". > > While casting works as expected with expression debugging turned off, > this seems to be an indication that I'm infact doing something wrong > in the building of the expression. > > (gdb) print *(int*)(0x0) > Dump of expression @ 0x12db320, before conversion to prefix form: > Language d, 11 elements, 16 bytes each. > Index Opcode Hex Value String Value > 0 OP_TYPE 87 W............... > 1 <unknown 20114800> 20114800 p.2............. > 2 OP_TYPE 87 W............... > 3 OP_LONG 38 &............... > 4 <unknown 19696640> 19696640 ..,............. > 5 OP_NULL 0 ................ > 6 OP_LONG 38 &............... > 7 UNOP_CAST 51 3............... > 8 <unknown 20114800> 20114800 p.2............. > 9 UNOP_CAST 51 3............... > 10 UNOP_IND 61 =............... > Dump of expression @ 0x12db320, after conversion to prefix form: > Expression: `*(int *) 0' > Language d, 11 elements, 16 bytes each. > > > 0 UNOP_IND > 1 UNOP_CAST Type @0x132ed70 (int *) > 4 OP_LONG Type @0x12c8c00 (int), value 0 (0x0) > 8 <unknown 20114800> Unknown format > 9 UNOP_CAST Type @0x3d (Segmentation fault > > > Looks like using UNOP_CAST_TYPE is the right thing to do here, as the > TypeExp handler has wrapped the type around a pair of OP_TYPE opcodes. > > > dlang-debug-expr.patch > > > 2017-01-04 Iain Buclaw <ibuclaw@gdcproject.org> > > gdb/ChangeLog: > > * d-exp.y (CastExpression): Emit UNOP_CAST_TYPE. > > gdb/testsuite/ChangeLog: > > * gdb.dlang/debug-expr.exp: New file. > > --- > diff --git a/gdb/d-exp.y b/gdb/d-exp.y > index 077e645..91d15f2 100644 > --- a/gdb/d-exp.y > +++ b/gdb/d-exp.y > @@ -321,15 +321,12 @@ UnaryExpression: > > CastExpression: > CAST_KEYWORD '(' TypeExp ')' UnaryExpression > - { write_exp_elt_opcode (pstate, UNOP_CAST); > - write_exp_elt_type (pstate, $3); > - write_exp_elt_opcode (pstate, UNOP_CAST); } > + { write_exp_elt_opcode (pstate, UNOP_CAST_TYPE); } > /* C style cast is illegal D, but is still recognised in > the grammar, so we keep this around for convenience. */ > | '(' TypeExp ')' UnaryExpression > - { write_exp_elt_opcode (pstate, UNOP_CAST); > - write_exp_elt_type (pstate, $2); > - write_exp_elt_opcode (pstate, UNOP_CAST); } > + { write_exp_elt_opcode (pstate, UNOP_CAST_TYPE); } > + > ; > > PowExpression: > diff --git a/gdb/testsuite/gdb.dlang/debug-expr.exp b/gdb/testsuite/gdb.dlang/debug-expr.exp > new file mode 100644 > index 0000000..3bb2c09 > --- /dev/null > +++ b/gdb/testsuite/gdb.dlang/debug-expr.exp > @@ -0,0 +1,40 @@ > +# Copyright 2017 Free Software Foundation, Inc. > + > +# This program is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation; either version 3 of the License, or > +# (at your option) any later version. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program. If not, see <http://www.gnu.org/licenses/>. > + > +# Test "set debug expr 1" on d expressions. > + > +if { [skip_d_tests] } { continue } We should output a message: untested "skipping d language tests" It may be more reasonable to just return instead of continuing? The effect will probably be the same, but it is a bit confusing to read "continue" without a visible loop. > + > +gdb_start > +gdb_test_no_output "set language d" > +gdb_test_no_output "set debug expression 1" > + > +# Test whether the expression debug machinery accepts the expression. > + > +proc test_debug_expr { cmd output } { > + global gdb_prompt > + > + gdb_test_multiple $cmd "" { > + -re ".*Invalid expression.*\r\n$gdb_prompt $" { > + fail $cmd > + } > + -re ".*\[\r\n\]$output\r\n$gdb_prompt $" { > + pass $cmd > + } > + } > +} > + > +# This caused gdb to segfault. > +test_debug_expr "print *(int*)(0)" "Cannot access memory at address 0x0" > Otherwise the test looks good. I don't have much state on the fix itself, but it seems reasonable if it fixes a crash. I'm assuming no testsuite regressions?
On 4 January 2017 at 17:08, Luis Machado <lgustavo@codesourcery.com> wrote: > On 01/03/2017 06:02 PM, Iain Buclaw wrote: >> >> This patch fixes a crash found on "p *(type *)0x1234" when using "set >> debug expression 1". >> >> While casting works as expected with expression debugging turned off, >> this seems to be an indication that I'm infact doing something wrong >> in the building of the expression. >> >> (gdb) print *(int*)(0x0) >> Dump of expression @ 0x12db320, before conversion to prefix form: >> Language d, 11 elements, 16 bytes each. >> Index Opcode Hex Value String Value >> 0 OP_TYPE 87 W............... >> 1 <unknown 20114800> 20114800 p.2............. >> 2 OP_TYPE 87 W............... >> 3 OP_LONG 38 &............... >> 4 <unknown 19696640> 19696640 ..,............. >> 5 OP_NULL 0 ................ >> 6 OP_LONG 38 &............... >> 7 UNOP_CAST 51 3............... >> 8 <unknown 20114800> 20114800 p.2............. >> 9 UNOP_CAST 51 3............... >> 10 UNOP_IND 61 =............... >> Dump of expression @ 0x12db320, after conversion to prefix form: >> Expression: `*(int *) 0' >> Language d, 11 elements, 16 bytes each. >> >> >> 0 UNOP_IND >> 1 UNOP_CAST Type @0x132ed70 (int *) >> 4 OP_LONG Type @0x12c8c00 (int), value 0 (0x0) >> 8 <unknown 20114800> Unknown format >> 9 UNOP_CAST Type @0x3d (Segmentation fault >> >> >> Looks like using UNOP_CAST_TYPE is the right thing to do here, as the >> TypeExp handler has wrapped the type around a pair of OP_TYPE opcodes. >> >> >> dlang-debug-expr.patch >> >> >> 2017-01-04 Iain Buclaw <ibuclaw@gdcproject.org> >> >> gdb/ChangeLog: >> >> * d-exp.y (CastExpression): Emit UNOP_CAST_TYPE. >> >> gdb/testsuite/ChangeLog: >> >> * gdb.dlang/debug-expr.exp: New file. >> >> --- >> diff --git a/gdb/d-exp.y b/gdb/d-exp.y >> index 077e645..91d15f2 100644 >> --- a/gdb/d-exp.y >> +++ b/gdb/d-exp.y >> @@ -321,15 +321,12 @@ UnaryExpression: >> >> CastExpression: >> CAST_KEYWORD '(' TypeExp ')' UnaryExpression >> - { write_exp_elt_opcode (pstate, UNOP_CAST); >> - write_exp_elt_type (pstate, $3); >> - write_exp_elt_opcode (pstate, UNOP_CAST); } >> + { write_exp_elt_opcode (pstate, UNOP_CAST_TYPE); } >> /* C style cast is illegal D, but is still recognised in >> the grammar, so we keep this around for convenience. */ >> | '(' TypeExp ')' UnaryExpression >> - { write_exp_elt_opcode (pstate, UNOP_CAST); >> - write_exp_elt_type (pstate, $2); >> - write_exp_elt_opcode (pstate, UNOP_CAST); } >> + { write_exp_elt_opcode (pstate, UNOP_CAST_TYPE); } >> + >> ; >> >> PowExpression: >> diff --git a/gdb/testsuite/gdb.dlang/debug-expr.exp >> b/gdb/testsuite/gdb.dlang/debug-expr.exp >> new file mode 100644 >> index 0000000..3bb2c09 >> --- /dev/null >> +++ b/gdb/testsuite/gdb.dlang/debug-expr.exp >> @@ -0,0 +1,40 @@ >> +# Copyright 2017 Free Software Foundation, Inc. >> + >> +# This program is free software; you can redistribute it and/or modify >> +# it under the terms of the GNU General Public License as published by >> +# the Free Software Foundation; either version 3 of the License, or >> +# (at your option) any later version. >> +# >> +# This program is distributed in the hope that it will be useful, >> +# but WITHOUT ANY WARRANTY; without even the implied warranty of >> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> +# GNU General Public License for more details. >> +# >> +# You should have received a copy of the GNU General Public License >> +# along with this program. If not, see <http://www.gnu.org/licenses/>. >> + >> +# Test "set debug expr 1" on d expressions. >> + >> +if { [skip_d_tests] } { continue } > > > We should output a message: > > untested "skipping d language tests" > > It may be more reasonable to just return instead of continuing? The effect > will probably be the same, but it is a bit confusing to read "continue" > without a visible loop. > I did a quick grep, and it seems like everyone is skippingtests in this way except for gdb.ada and gdb.btrace which are doing { return -1 } >> + >> +gdb_start >> +gdb_test_no_output "set language d" >> +gdb_test_no_output "set debug expression 1" >> + >> +# Test whether the expression debug machinery accepts the expression. >> + >> +proc test_debug_expr { cmd output } { >> + global gdb_prompt >> + >> + gdb_test_multiple $cmd "" { >> + -re ".*Invalid expression.*\r\n$gdb_prompt $" { >> + fail $cmd >> + } >> + -re ".*\[\r\n\]$output\r\n$gdb_prompt $" { >> + pass $cmd >> + } >> + } >> +} >> + >> +# This caused gdb to segfault. >> +test_debug_expr "print *(int*)(0)" "Cannot access memory at address 0x0" >> > > Otherwise the test looks good. I don't have much state on the fix itself, > but it seems reasonable if it fixes a crash. > I had a look at other language frontends for inspiration. At least C/C++ language does it this way. Others that use UNOP_CAST don't write out OP_TYPE. The other way to do it would be to change the grammar to '(' TYPE ')' UnaryExpression. But I don't want to do that. > I'm assuming no testsuite regressions? None at all.
On 01/04/2017 01:38 PM, Iain Buclaw wrote: > On 4 January 2017 at 17:08, Luis Machado <lgustavo@codesourcery.com> wrote: >> On 01/03/2017 06:02 PM, Iain Buclaw wrote: >>> >>> This patch fixes a crash found on "p *(type *)0x1234" when using "set >>> debug expression 1". >>> >>> While casting works as expected with expression debugging turned off, >>> this seems to be an indication that I'm infact doing something wrong >>> in the building of the expression. >>> >>> (gdb) print *(int*)(0x0) >>> Dump of expression @ 0x12db320, before conversion to prefix form: >>> Language d, 11 elements, 16 bytes each. >>> Index Opcode Hex Value String Value >>> 0 OP_TYPE 87 W............... >>> 1 <unknown 20114800> 20114800 p.2............. >>> 2 OP_TYPE 87 W............... >>> 3 OP_LONG 38 &............... >>> 4 <unknown 19696640> 19696640 ..,............. >>> 5 OP_NULL 0 ................ >>> 6 OP_LONG 38 &............... >>> 7 UNOP_CAST 51 3............... >>> 8 <unknown 20114800> 20114800 p.2............. >>> 9 UNOP_CAST 51 3............... >>> 10 UNOP_IND 61 =............... >>> Dump of expression @ 0x12db320, after conversion to prefix form: >>> Expression: `*(int *) 0' >>> Language d, 11 elements, 16 bytes each. >>> >>> >>> 0 UNOP_IND >>> 1 UNOP_CAST Type @0x132ed70 (int *) >>> 4 OP_LONG Type @0x12c8c00 (int), value 0 (0x0) >>> 8 <unknown 20114800> Unknown format >>> 9 UNOP_CAST Type @0x3d (Segmentation fault >>> >>> >>> Looks like using UNOP_CAST_TYPE is the right thing to do here, as the >>> TypeExp handler has wrapped the type around a pair of OP_TYPE opcodes. >>> >>> >>> dlang-debug-expr.patch >>> >>> >>> 2017-01-04 Iain Buclaw <ibuclaw@gdcproject.org> >>> >>> gdb/ChangeLog: >>> >>> * d-exp.y (CastExpression): Emit UNOP_CAST_TYPE. >>> >>> gdb/testsuite/ChangeLog: >>> >>> * gdb.dlang/debug-expr.exp: New file. >>> >>> --- >>> diff --git a/gdb/d-exp.y b/gdb/d-exp.y >>> index 077e645..91d15f2 100644 >>> --- a/gdb/d-exp.y >>> +++ b/gdb/d-exp.y >>> @@ -321,15 +321,12 @@ UnaryExpression: >>> >>> CastExpression: >>> CAST_KEYWORD '(' TypeExp ')' UnaryExpression >>> - { write_exp_elt_opcode (pstate, UNOP_CAST); >>> - write_exp_elt_type (pstate, $3); >>> - write_exp_elt_opcode (pstate, UNOP_CAST); } >>> + { write_exp_elt_opcode (pstate, UNOP_CAST_TYPE); } >>> /* C style cast is illegal D, but is still recognised in >>> the grammar, so we keep this around for convenience. */ >>> | '(' TypeExp ')' UnaryExpression >>> - { write_exp_elt_opcode (pstate, UNOP_CAST); >>> - write_exp_elt_type (pstate, $2); >>> - write_exp_elt_opcode (pstate, UNOP_CAST); } >>> + { write_exp_elt_opcode (pstate, UNOP_CAST_TYPE); } >>> + >>> ; >>> >>> PowExpression: >>> diff --git a/gdb/testsuite/gdb.dlang/debug-expr.exp >>> b/gdb/testsuite/gdb.dlang/debug-expr.exp >>> new file mode 100644 >>> index 0000000..3bb2c09 >>> --- /dev/null >>> +++ b/gdb/testsuite/gdb.dlang/debug-expr.exp >>> @@ -0,0 +1,40 @@ >>> +# Copyright 2017 Free Software Foundation, Inc. >>> + >>> +# This program is free software; you can redistribute it and/or modify >>> +# it under the terms of the GNU General Public License as published by >>> +# the Free Software Foundation; either version 3 of the License, or >>> +# (at your option) any later version. >>> +# >>> +# This program is distributed in the hope that it will be useful, >>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of >>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >>> +# GNU General Public License for more details. >>> +# >>> +# You should have received a copy of the GNU General Public License >>> +# along with this program. If not, see <http://www.gnu.org/licenses/>. >>> + >>> +# Test "set debug expr 1" on d expressions. >>> + >>> +if { [skip_d_tests] } { continue } >> >> >> We should output a message: >> >> untested "skipping d language tests" >> >> It may be more reasonable to just return instead of continuing? The effect >> will probably be the same, but it is a bit confusing to read "continue" >> without a visible loop. >> > > I did a quick grep, and it seems like everyone is skippingtests in > this way except for gdb.ada and gdb.btrace which are doing { return -1 > } > > That's a bit of a stretch. Take, for example, a few of the examples in gdb.base. You will see a number of them returning. The problem here is inheriting past confusing practices when we use some existing files to create new ones, which is not your fault really. I'm guilty myself. :-) Just recently i fixed a bunch of bad practices in terms of test naming. I think using { continue } here to mean "just end the test" is one of those bad practices that we still carry around in our codebase. Until someone decides to clean those up, we still still be carrying those around. I agree with you we have examples in the codebase using exactly this confusing construct, but we want to make sure new code does things in more reasonable ways so our codebase stays clean and readable. >>> + >>> +gdb_start >>> +gdb_test_no_output "set language d" >>> +gdb_test_no_output "set debug expression 1" >>> + >>> +# Test whether the expression debug machinery accepts the expression. >>> + >>> +proc test_debug_expr { cmd output } { >>> + global gdb_prompt >>> + >>> + gdb_test_multiple $cmd "" { >>> + -re ".*Invalid expression.*\r\n$gdb_prompt $" { >>> + fail $cmd >>> + } >>> + -re ".*\[\r\n\]$output\r\n$gdb_prompt $" { >>> + pass $cmd >>> + } >>> + } >>> +} >>> + >>> +# This caused gdb to segfault. >>> +test_debug_expr "print *(int*)(0)" "Cannot access memory at address 0x0" >>> >> >> Otherwise the test looks good. I don't have much state on the fix itself, >> but it seems reasonable if it fixes a crash. >> > > I had a look at other language frontends for inspiration. At least > C/C++ language does it this way. Others that use UNOP_CAST don't > write out OP_TYPE. The other way to do it would be to change the > grammar to '(' TYPE ')' UnaryExpression. But I don't want to do > that. > >> I'm assuming no testsuite regressions? > > None at all. >
On 4 January 2017 at 20:48, Luis Machado <lgustavo@codesourcery.com> wrote: > On 01/04/2017 01:38 PM, Iain Buclaw wrote: >> >> On 4 January 2017 at 17:08, Luis Machado <lgustavo@codesourcery.com> >> wrote: >>> >>> On 01/03/2017 06:02 PM, Iain Buclaw wrote: >>>> >>>> >>>> This patch fixes a crash found on "p *(type *)0x1234" when using "set >>>> debug expression 1". >>>> >>>> While casting works as expected with expression debugging turned off, >>>> this seems to be an indication that I'm infact doing something wrong >>>> in the building of the expression. >>>> >>>> (gdb) print *(int*)(0x0) >>>> Dump of expression @ 0x12db320, before conversion to prefix form: >>>> Language d, 11 elements, 16 bytes each. >>>> Index Opcode Hex Value String Value >>>> 0 OP_TYPE 87 W............... >>>> 1 <unknown 20114800> 20114800 p.2............. >>>> 2 OP_TYPE 87 W............... >>>> 3 OP_LONG 38 &............... >>>> 4 <unknown 19696640> 19696640 ..,............. >>>> 5 OP_NULL 0 ................ >>>> 6 OP_LONG 38 &............... >>>> 7 UNOP_CAST 51 3............... >>>> 8 <unknown 20114800> 20114800 p.2............. >>>> 9 UNOP_CAST 51 3............... >>>> 10 UNOP_IND 61 =............... >>>> Dump of expression @ 0x12db320, after conversion to prefix form: >>>> Expression: `*(int *) 0' >>>> Language d, 11 elements, 16 bytes each. >>>> >>>> >>>> 0 UNOP_IND >>>> 1 UNOP_CAST Type @0x132ed70 (int *) >>>> 4 OP_LONG Type @0x12c8c00 (int), value 0 >>>> (0x0) >>>> 8 <unknown 20114800> Unknown format >>>> 9 UNOP_CAST Type @0x3d (Segmentation fault >>>> >>>> >>>> Looks like using UNOP_CAST_TYPE is the right thing to do here, as the >>>> TypeExp handler has wrapped the type around a pair of OP_TYPE opcodes. >>>> >>>> >>>> dlang-debug-expr.patch >>>> >>>> >>>> 2017-01-04 Iain Buclaw <ibuclaw@gdcproject.org> >>>> >>>> gdb/ChangeLog: >>>> >>>> * d-exp.y (CastExpression): Emit UNOP_CAST_TYPE. >>>> >>>> gdb/testsuite/ChangeLog: >>>> >>>> * gdb.dlang/debug-expr.exp: New file. >>>> >>>> --- >>>> diff --git a/gdb/d-exp.y b/gdb/d-exp.y >>>> index 077e645..91d15f2 100644 >>>> --- a/gdb/d-exp.y >>>> +++ b/gdb/d-exp.y >>>> @@ -321,15 +321,12 @@ UnaryExpression: >>>> >>>> CastExpression: >>>> CAST_KEYWORD '(' TypeExp ')' UnaryExpression >>>> - { write_exp_elt_opcode (pstate, UNOP_CAST); >>>> - write_exp_elt_type (pstate, $3); >>>> - write_exp_elt_opcode (pstate, UNOP_CAST); } >>>> + { write_exp_elt_opcode (pstate, UNOP_CAST_TYPE); } >>>> /* C style cast is illegal D, but is still recognised in >>>> the grammar, so we keep this around for convenience. */ >>>> | '(' TypeExp ')' UnaryExpression >>>> - { write_exp_elt_opcode (pstate, UNOP_CAST); >>>> - write_exp_elt_type (pstate, $2); >>>> - write_exp_elt_opcode (pstate, UNOP_CAST); } >>>> + { write_exp_elt_opcode (pstate, UNOP_CAST_TYPE); } >>>> + >>>> ; >>>> >>>> PowExpression: >>>> diff --git a/gdb/testsuite/gdb.dlang/debug-expr.exp >>>> b/gdb/testsuite/gdb.dlang/debug-expr.exp >>>> new file mode 100644 >>>> index 0000000..3bb2c09 >>>> --- /dev/null >>>> +++ b/gdb/testsuite/gdb.dlang/debug-expr.exp >>>> @@ -0,0 +1,40 @@ >>>> +# Copyright 2017 Free Software Foundation, Inc. >>>> + >>>> +# This program is free software; you can redistribute it and/or modify >>>> +# it under the terms of the GNU General Public License as published by >>>> +# the Free Software Foundation; either version 3 of the License, or >>>> +# (at your option) any later version. >>>> +# >>>> +# This program is distributed in the hope that it will be useful, >>>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of >>>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >>>> +# GNU General Public License for more details. >>>> +# >>>> +# You should have received a copy of the GNU General Public License >>>> +# along with this program. If not, see <http://www.gnu.org/licenses/>. >>>> + >>>> +# Test "set debug expr 1" on d expressions. >>>> + >>>> +if { [skip_d_tests] } { continue } >>> >>> >>> >>> We should output a message: >>> >>> untested "skipping d language tests" >>> >>> It may be more reasonable to just return instead of continuing? The >>> effect >>> will probably be the same, but it is a bit confusing to read "continue" >>> without a visible loop. >>> >> >> I did a quick grep, and it seems like everyone is skippingtests in >> this way except for gdb.ada and gdb.btrace which are doing { return -1 >> } >> >> > > That's a bit of a stretch. Take, for example, a few of the examples in > gdb.base. You will see a number of them returning. > > The problem here is inheriting past confusing practices when we use some > existing files to create new ones, which is not your fault really. I'm > guilty myself. :-) > Yes indeed. I wasn't disagreeing, just questioning the two competing ways of returning. I will update to use return and push this in then if there's no disagreement. :-) > Just recently i fixed a bunch of bad practices in terms of test naming. > > I think using { continue } here to mean "just end the test" is one of those > bad practices that we still carry around in our codebase. Until someone > decides to clean those up, we still still be carrying those around. > > I agree with you we have examples in the codebase using exactly this > confusing construct, but we want to make sure new code does things in more > reasonable ways so our codebase stays clean and readable. > > I'll make a note and if I remember, I'll send something through later. >>>> + >>>> +gdb_start >>>> +gdb_test_no_output "set language d" >>>> +gdb_test_no_output "set debug expression 1" >>>> + >>>> +# Test whether the expression debug machinery accepts the expression. >>>> + >>>> +proc test_debug_expr { cmd output } { >>>> + global gdb_prompt >>>> + >>>> + gdb_test_multiple $cmd "" { >>>> + -re ".*Invalid expression.*\r\n$gdb_prompt $" { >>>> + fail $cmd >>>> + } >>>> + -re ".*\[\r\n\]$output\r\n$gdb_prompt $" { >>>> + pass $cmd >>>> + } >>>> + } >>>> +} >>>> + >>>> +# This caused gdb to segfault. >>>> +test_debug_expr "print *(int*)(0)" "Cannot access memory at address >>>> 0x0" >>>> >>> >>> Otherwise the test looks good. I don't have much state on the fix itself, >>> but it seems reasonable if it fixes a crash. >>> >> >> I had a look at other language frontends for inspiration. At least >> C/C++ language does it this way. Others that use UNOP_CAST don't >> write out OP_TYPE. The other way to do it would be to change the >> grammar to '(' TYPE ')' UnaryExpression. But I don't want to do >> that. >> >>> I'm assuming no testsuite regressions? >> >> >> None at all. >> >
On 01/04/2017 02:38 PM, Iain Buclaw wrote: > On 4 January 2017 at 20:48, Luis Machado <lgustavo@codesourcery.com> wrote: >> On 01/04/2017 01:38 PM, Iain Buclaw wrote: >>> >>> On 4 January 2017 at 17:08, Luis Machado <lgustavo@codesourcery.com> >>> wrote: >>>> >>>> On 01/03/2017 06:02 PM, Iain Buclaw wrote: >>>>> >>>>> >>>>> This patch fixes a crash found on "p *(type *)0x1234" when using "set >>>>> debug expression 1". >>>>> >>>>> While casting works as expected with expression debugging turned off, >>>>> this seems to be an indication that I'm infact doing something wrong >>>>> in the building of the expression. >>>>> >>>>> (gdb) print *(int*)(0x0) >>>>> Dump of expression @ 0x12db320, before conversion to prefix form: >>>>> Language d, 11 elements, 16 bytes each. >>>>> Index Opcode Hex Value String Value >>>>> 0 OP_TYPE 87 W............... >>>>> 1 <unknown 20114800> 20114800 p.2............. >>>>> 2 OP_TYPE 87 W............... >>>>> 3 OP_LONG 38 &............... >>>>> 4 <unknown 19696640> 19696640 ..,............. >>>>> 5 OP_NULL 0 ................ >>>>> 6 OP_LONG 38 &............... >>>>> 7 UNOP_CAST 51 3............... >>>>> 8 <unknown 20114800> 20114800 p.2............. >>>>> 9 UNOP_CAST 51 3............... >>>>> 10 UNOP_IND 61 =............... >>>>> Dump of expression @ 0x12db320, after conversion to prefix form: >>>>> Expression: `*(int *) 0' >>>>> Language d, 11 elements, 16 bytes each. >>>>> >>>>> >>>>> 0 UNOP_IND >>>>> 1 UNOP_CAST Type @0x132ed70 (int *) >>>>> 4 OP_LONG Type @0x12c8c00 (int), value 0 >>>>> (0x0) >>>>> 8 <unknown 20114800> Unknown format >>>>> 9 UNOP_CAST Type @0x3d (Segmentation fault >>>>> >>>>> >>>>> Looks like using UNOP_CAST_TYPE is the right thing to do here, as the >>>>> TypeExp handler has wrapped the type around a pair of OP_TYPE opcodes. >>>>> >>>>> >>>>> dlang-debug-expr.patch >>>>> >>>>> >>>>> 2017-01-04 Iain Buclaw <ibuclaw@gdcproject.org> >>>>> >>>>> gdb/ChangeLog: >>>>> >>>>> * d-exp.y (CastExpression): Emit UNOP_CAST_TYPE. >>>>> >>>>> gdb/testsuite/ChangeLog: >>>>> >>>>> * gdb.dlang/debug-expr.exp: New file. >>>>> >>>>> --- >>>>> diff --git a/gdb/d-exp.y b/gdb/d-exp.y >>>>> index 077e645..91d15f2 100644 >>>>> --- a/gdb/d-exp.y >>>>> +++ b/gdb/d-exp.y >>>>> @@ -321,15 +321,12 @@ UnaryExpression: >>>>> >>>>> CastExpression: >>>>> CAST_KEYWORD '(' TypeExp ')' UnaryExpression >>>>> - { write_exp_elt_opcode (pstate, UNOP_CAST); >>>>> - write_exp_elt_type (pstate, $3); >>>>> - write_exp_elt_opcode (pstate, UNOP_CAST); } >>>>> + { write_exp_elt_opcode (pstate, UNOP_CAST_TYPE); } >>>>> /* C style cast is illegal D, but is still recognised in >>>>> the grammar, so we keep this around for convenience. */ >>>>> | '(' TypeExp ')' UnaryExpression >>>>> - { write_exp_elt_opcode (pstate, UNOP_CAST); >>>>> - write_exp_elt_type (pstate, $2); >>>>> - write_exp_elt_opcode (pstate, UNOP_CAST); } >>>>> + { write_exp_elt_opcode (pstate, UNOP_CAST_TYPE); } >>>>> + >>>>> ; >>>>> >>>>> PowExpression: >>>>> diff --git a/gdb/testsuite/gdb.dlang/debug-expr.exp >>>>> b/gdb/testsuite/gdb.dlang/debug-expr.exp >>>>> new file mode 100644 >>>>> index 0000000..3bb2c09 >>>>> --- /dev/null >>>>> +++ b/gdb/testsuite/gdb.dlang/debug-expr.exp >>>>> @@ -0,0 +1,40 @@ >>>>> +# Copyright 2017 Free Software Foundation, Inc. >>>>> + >>>>> +# This program is free software; you can redistribute it and/or modify >>>>> +# it under the terms of the GNU General Public License as published by >>>>> +# the Free Software Foundation; either version 3 of the License, or >>>>> +# (at your option) any later version. >>>>> +# >>>>> +# This program is distributed in the hope that it will be useful, >>>>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of >>>>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >>>>> +# GNU General Public License for more details. >>>>> +# >>>>> +# You should have received a copy of the GNU General Public License >>>>> +# along with this program. If not, see <http://www.gnu.org/licenses/>. >>>>> + >>>>> +# Test "set debug expr 1" on d expressions. >>>>> + >>>>> +if { [skip_d_tests] } { continue } >>>> >>>> >>>> >>>> We should output a message: >>>> >>>> untested "skipping d language tests" >>>> >>>> It may be more reasonable to just return instead of continuing? The >>>> effect >>>> will probably be the same, but it is a bit confusing to read "continue" >>>> without a visible loop. >>>> >>> >>> I did a quick grep, and it seems like everyone is skippingtests in >>> this way except for gdb.ada and gdb.btrace which are doing { return -1 >>> } >>> >>> >> >> That's a bit of a stretch. Take, for example, a few of the examples in >> gdb.base. You will see a number of them returning. >> >> The problem here is inheriting past confusing practices when we use some >> existing files to create new ones, which is not your fault really. I'm >> guilty myself. :-) >> > > Yes indeed. I wasn't disagreeing, just questioning the two competing > ways of returning. > > I will update to use return and push this in then if there's no > disagreement. :-) > Hopefully someone will chime in for a second opinion. :-)
>>>>>> +if { [skip_d_tests] } { continue } >>>>> >>>>> >>>>> >>>>> We should output a message: >>>>> >>>>> untested "skipping d language tests" >>>>> >>>>> It may be more reasonable to just return instead of continuing? The >>>>> effect >>>>> will probably be the same, but it is a bit confusing to read >>>>> "continue" >>>>> without a visible loop. >>>>> >>>> >>>> I did a quick grep, and it seems like everyone is skippingtests in >>>> this way except for gdb.ada and gdb.btrace which are doing { return >>>> -1 >>>> } >>>> >>>> >>> >>> That's a bit of a stretch. Take, for example, a few of the examples >>> in >>> gdb.base. You will see a number of them returning. >>> >>> The problem here is inheriting past confusing practices when we use >>> some >>> existing files to create new ones, which is not your fault really. >>> I'm >>> guilty myself. :-) >>> >> >> Yes indeed. I wasn't disagreeing, just questioning the two competing >> ways of returning. >> >> I will update to use return and push this in then if there's no >> disagreement. :-) >> > > Hopefully someone will chime in for a second opinion. :-) +1 :)
On 17-01-04 21:38:07, Iain Buclaw wrote: > >>>> +if { [skip_d_tests] } { continue } > >>> > >>> We should output a message: > >>> > >>> untested "skipping d language tests" > >>> > >>> It may be more reasonable to just return instead of continuing? The > >>> effect > >>> will probably be the same, but it is a bit confusing to read "continue" > >>> without a visible loop. > >>> > >> > >> I did a quick grep, and it seems like everyone is skippingtests in > >> this way except for gdb.ada and gdb.btrace which are doing { return -1 > >> } > >> > >> > > > > That's a bit of a stretch. Take, for example, a few of the examples in > > gdb.base. You will see a number of them returning. > > > > The problem here is inheriting past confusing practices when we use some > > existing files to create new ones, which is not your fault really. I'm > > guilty myself. :-) > > > > Yes indeed. I wasn't disagreeing, just questioning the two competing > ways of returning. > > I will update to use return and push this in then if there's no > disagreement. :-) > "return" is clearer than "continue", and "return" is preferred in https://www.sourceware.org/gdb/wiki/GDBTestcaseCookbook -- Yao (齐尧)
On 5 January 2017 at 17:13, Simon Marchi <simon.marchi@polymtl.ca> wrote: >>>>>>> +if { [skip_d_tests] } { continue } >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> We should output a message: >>>>>> >>>>>> untested "skipping d language tests" >>>>>> >>>>>> It may be more reasonable to just return instead of continuing? The >>>>>> effect >>>>>> will probably be the same, but it is a bit confusing to read >>>>>> "continue" >>>>>> without a visible loop. >>>>>> >>>>> >>>>> I did a quick grep, and it seems like everyone is skippingtests in >>>>> this way except for gdb.ada and gdb.btrace which are doing { return -1 >>>>> } >>>>> >>>>> >>>> >>>> That's a bit of a stretch. Take, for example, a few of the examples in >>>> gdb.base. You will see a number of them returning. >>>> >>>> The problem here is inheriting past confusing practices when we use some >>>> existing files to create new ones, which is not your fault really. I'm >>>> guilty myself. :-) >>>> >>> >>> Yes indeed. I wasn't disagreeing, just questioning the two competing >>> ways of returning. >>> >>> I will update to use return and push this in then if there's no >>> disagreement. :-) >>> >> >> Hopefully someone will chime in for a second opinion. :-) > > > +1 :) FYI, committed patched as-is except for the following adjustment as discussed: -if { [skip_d_tests] } { continue } +if { [skip_d_tests] } { return -1 } Thanks for the review.
2017-01-04 Iain Buclaw <ibuclaw@gdcproject.org> gdb/ChangeLog: * d-exp.y (CastExpression): Emit UNOP_CAST_TYPE. gdb/testsuite/ChangeLog: * gdb.dlang/debug-expr.exp: New file. --- diff --git a/gdb/d-exp.y b/gdb/d-exp.y index 077e645..91d15f2 100644 --- a/gdb/d-exp.y +++ b/gdb/d-exp.y @@ -321,15 +321,12 @@ UnaryExpression: CastExpression: CAST_KEYWORD '(' TypeExp ')' UnaryExpression - { write_exp_elt_opcode (pstate, UNOP_CAST); - write_exp_elt_type (pstate, $3); - write_exp_elt_opcode (pstate, UNOP_CAST); } + { write_exp_elt_opcode (pstate, UNOP_CAST_TYPE); } /* C style cast is illegal D, but is still recognised in the grammar, so we keep this around for convenience. */ | '(' TypeExp ')' UnaryExpression - { write_exp_elt_opcode (pstate, UNOP_CAST); - write_exp_elt_type (pstate, $2); - write_exp_elt_opcode (pstate, UNOP_CAST); } + { write_exp_elt_opcode (pstate, UNOP_CAST_TYPE); } + ; PowExpression: diff --git a/gdb/testsuite/gdb.dlang/debug-expr.exp b/gdb/testsuite/gdb.dlang/debug-expr.exp new file mode 100644 index 0000000..3bb2c09 --- /dev/null +++ b/gdb/testsuite/gdb.dlang/debug-expr.exp @@ -0,0 +1,40 @@ +# Copyright 2017 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +# Test "set debug expr 1" on d expressions. + +if { [skip_d_tests] } { continue } + +gdb_start +gdb_test_no_output "set language d" +gdb_test_no_output "set debug expression 1" + +# Test whether the expression debug machinery accepts the expression. + +proc test_debug_expr { cmd output } { + global gdb_prompt + + gdb_test_multiple $cmd "" { + -re ".*Invalid expression.*\r\n$gdb_prompt $" { + fail $cmd + } + -re ".*\[\r\n\]$output\r\n$gdb_prompt $" { + pass $cmd + } + } +} + +# This caused gdb to segfault. +test_debug_expr "print *(int*)(0)" "Cannot access memory at address 0x0"
This patch fixes a crash found on "p *(type *)0x1234" when using "set debug expression 1". While casting works as expected with expression debugging turned off, this seems to be an indication that I'm infact doing something wrong in the building of the expression. (gdb) print *(int*)(0x0) Dump of expression @ 0x12db320, before conversion to prefix form: Language d, 11 elements, 16 bytes each. Index Opcode Hex Value String Value 0 OP_TYPE 87 W............... 1 <unknown 20114800> 20114800 p.2............. 2 OP_TYPE 87 W............... 3 OP_LONG 38 &............... 4 <unknown 19696640> 19696640 ..,............. 5 OP_NULL 0 ................ 6 OP_LONG 38 &............... 7 UNOP_CAST 51 3............... 8 <unknown 20114800> 20114800 p.2............. 9 UNOP_CAST 51 3............... 10 UNOP_IND 61 =............... Dump of expression @ 0x12db320, after conversion to prefix form: Expression: `*(int *) 0' Language d, 11 elements, 16 bytes each. 0 UNOP_IND 1 UNOP_CAST Type @0x132ed70 (int *) 4 OP_LONG Type @0x12c8c00 (int), value 0 (0x0) 8 <unknown 20114800> Unknown format 9 UNOP_CAST Type @0x3d (Segmentation fault Looks like using UNOP_CAST_TYPE is the right thing to do here, as the TypeExp handler has wrapped the type around a pair of OP_TYPE opcodes.