[1/2] gdb: Allow address space qualifier parsing in C++.

Message ID 20210326142609.245016-2-felix.willgerodt@intel.com
State New
Headers show
Series
  • Expression parser fixes for address space qualifiers.
Related show

Commit Message

Mike Frysinger via Gdb-patches March 26, 2021, 2:26 p.m.
The goal of this patch is to allow target dependent address space qualifiers
in the C++ expression parser.  This can be useful for memory examination on
targets that actually use different address spaces in hardware without
having to deep-dive into implementation details of the whole solution.

GDB uses the @ symbol to parse address space qualifiers.  The only current
user that I am aware of is the __flash support for avr, which was added in
"Add support for the __flash qualifier on AVR"
(487d975399dfcb2bb2f0998a7d12bd62acdd9fa1) and only works for C.

One use-case of the AVR patch is:

~~~
const __flash char data_in_flash = 0xab;

int
main (void)
{
  const __flash char *pointer_to_flash = &data_in_flash;
}
~~~

~~~
(gdb) print pointer_to_flash
$1 = 0x1e8 <data_in_flash> "\253"
(gdb) print/x *pointer_to_flash
$2 = 0xab
(gdb) x/x pointer_to_flash
0x1e8 <data_in_flash>: 0xXXXXXXab
(gdb)
(gdb) p/x *(char* @flash) pointer_to_flash
$3 = 0xab
~~~

I want to enable a similar usage of e.g. @local in C++.

Before this patch (using "set debug parser on"):

~~~
(gdb) p *(int* @local) 0x1234
(...)
Reading a token: Next token is token '@' ()
Shifting token '@' ()
Entering state 46
Reading a token: Next token is token UNKNOWN_CPP_NAME (ssym<name=local, sym=(null), field_of_this=0>)
A syntax error in expression, near `local) &x'.
~~~

After:
~~~
(gdb) p *(int* @local) 0x1234
(...)
Reading a token: Next token is token '@' ()
Shifting token '@' ()
Entering state 46
Reading a token: Next token is token UNKNOWN_CPP_NAME (ssym<name=local, sym=(null), field_of_this=0>)
Shifting token UNKNOWN_CPP_NAME (ssym<name=local, sym=(null), field_of_this=0>)
Entering state 121
Reducing stack by rule 278 (line 1773):
   $1 = token UNKNOWN_CPP_NAME (ssym<name=local, sym=(null), field_of_this=0>)
-> $$ = nterm name ()
Stack now 0 49 52 76 222 337 46
Entering state 167
Reducing stack by rule 131 (line 1225):
   $1 = token '@' ()
   $2 = nterm name ()
Unknown address space specifier: "local"
~~~

The "Unknown address space qualifier" is the right behaviour, as I ran this
on a target that doesn't have multiple address spaces and therefore obviously
no support for such qualifiers.

gdb/ChangeLog:
2021-03-19  Felix Willgerodt  <felix.willgerodt@intel.com>

	* c-exp.y (single_qualifier): Handle UNKNOWN_CPP_NAME.
---
 gdb/c-exp.y | 5 +++++
 1 file changed, 5 insertions(+)

-- 
2.25.4

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Comments

Tom Tromey April 1, 2021, 6:16 p.m. | #1
>>>>> "Felix" == Felix Willgerodt via Gdb-patches <gdb-patches@sourceware.org> writes:


Felix> gdb/ChangeLog:
Felix> 2021-03-19  Felix Willgerodt  <felix.willgerodt@intel.com>

Felix> 	* c-exp.y (single_qualifier): Handle UNKNOWN_CPP_NAME.

Thank you.  This is ok.

Do we have or want a test for this?

Tom
Mike Frysinger via Gdb-patches April 2, 2021, 12:33 p.m. | #2
> Do we have or want a test for this?

>

> Tom


This '@address_space_qualifier' is a bit of an undocumented and untested feature AFAIK. Even the avr tests for __flash don't test it.
I did search the git history a bit, but couldn't really determine why it was added. Only that it was added years before the __flash patch was.
But since it is there and since I need a language agnostic way to specify this, I plan to use it for a future target.

The only test I could currently write for this patch is something like:
gdb_test "*(@somerandomqualifiername int *) 0x12345678" "Unknown address space specifier: \"somerandomqualifiername\""

for a C++ program on any target. If you think that is valuable, I can easily add that.
The target I want to use this for in the end won't be ready for upstream for a while unfortunately.

Thanks,
Felix
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
Mike Frysinger via Gdb-patches April 2, 2021, 12:47 p.m. | #3
Hi,

On 4/2/21 9:33 AM, Willgerodt, Felix via Gdb-patches wrote:
>> Do we have or want a test for this?

>>

>> Tom

> 

> This '@address_space_qualifier' is a bit of an undocumented and untested feature AFAIK. Even the avr tests for __flash don't test it.

> I did search the git history a bit, but couldn't really determine why it was added. Only that it was added years before the __flash patch was.

> But since it is there and since I need a language agnostic way to specify this, I plan to use it for a future target.


Just to give some context, I have used this feature multiple times for 
architectures that expose separate address classes via DWARF, none of 
which have made its way upstream (the most recent port to use this is 
the ARM Morello one, for capability types).

I think IBM's Cell BE has used it as well, but the port was removed from 
the tree a few years ago.

But yes, it is a bit undocumented and obscure. It is hard to see tests 
for this because they tend to be arch-specific. Cell BE had tests for it 
(gdb/testsuite/gdb.cell/ea-test.exp), now removed.

> 

> The only test I could currently write for this patch is something like:

> gdb_test "*(@somerandomqualifiername int *) 0x12345678" "Unknown address space specifier: \"somerandomqualifiername\""

> 

> for a C++ program on any target. If you think that is valuable, I can easily add that.

> The target I want to use this for in the end won't be ready for upstream for a while unfortunately.

> 

> Thanks,

> Felix

> Intel Deutschland GmbH

> Registered Address: Am Campeon 10, 85579 Neubiberg, Germany

> Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>

> Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva

> Chairperson of the Supervisory Board: Nicole Lau

> Registered Office: Munich

> Commercial Register: Amtsgericht Muenchen HRB 186928

>
Mike Frysinger via Gdb-patches April 2, 2021, 4:48 p.m. | #4
> This '@address_space_qualifier' is a bit of an undocumented and untested feature AFAIK. Even the avr tests for __flash don't test it.

> I did search the git history a bit, but couldn't really determine why it was added. Only that it was added years before the __flash patch was.

> But since it is there and since I need a language agnostic way to specify this, I plan to use it for a future target.

> 

> The only test I could currently write for this patch is something like:

> gdb_test "*(@somerandomqualifiername int *) 0x12345678" "Unknown address space specifier: \"somerandomqualifiername\""> 

> for a C++ program on any target. If you think that is valuable, I can easily add that.

> The target I want to use this for in the end won't be ready for upstream for a while unfortunately.


Hi Felix,

I think it would be valuable to have a test like this.  It's better
than nothing, and it's always good to check error cases to make sure GDB
doesn't crash on them.  I can imagine that this test could test with
both a C and C++ program to cover everything correctly (and maybe other
languages, but I don't know much about them, if that even applies to
them).

A while ago I added a simavr board file, to be able to run tests against
an AVR target.  simavr is easy to build and use (it may even be packaged
in distros, but I'd be tempted to use the latest available version).
All of this to say you could try to run and improve the flash qualifier
test for AVR (or write a new test).

There's just one thing, avr-gcc/avr-g++ seem to produce stabs by
default, it's not really useful to test with stabs nowadays.  I had a
patch to make that board use dwarf by default (see below), but I never
got around to try it properly and post it.  I'll try to do it soon (but
you can apply it locally in the mean time).

Simon


From d1215e56aad6f9fc02f2cb2318ea522b32fd4f79 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@efficios.com>

Date: Sun, 21 Jun 2020 11:25:12 -0400
Subject: [PATCH] gdb/testsuite: use -gdwarf-4 in simavr board

Change-Id: I70e471fad3a79ab1d79d13dda8436bb9eb666e0a
---
 gdb/testsuite/boards/simavr.exp | 1 +
 1 file changed, 1 insertion(+)

diff --git a/gdb/testsuite/boards/simavr.exp b/gdb/testsuite/boards/simavr.exp
index c4f278ccb12e..35885ef78c27 100644
--- a/gdb/testsuite/boards/simavr.exp
+++ b/gdb/testsuite/boards/simavr.exp
@@ -43,6 +43,7 @@ set_board_info c++compiler avr-g++
 
 set_board_info cflags "-mmcu=${simavr_mcu}"
 set_board_info ldflags "-mmcu=${simavr_mcu}"
+set_board_info debug_flags "-gdwarf-4"
 
 set_board_info use_gdb_stub 1
 set_board_info gdb_protocol "remote"
-- 
2.30.1
Mike Frysinger via Gdb-patches April 2, 2021, 4:51 p.m. | #5
On 2021-04-02 12:48 p.m., Simon Marchi via Gdb-patches wrote:>> This '@address_space_qualifier' is a bit of an undocumented and untested feature AFAIK. Even the avr tests for __flash don't test it.
>> I did search the git history a bit, but couldn't really determine why it was added. Only that it was added years before the __flash patch was.

>> But since it is there and since I need a language agnostic way to specify this, I plan to use it for a future target.

>>

>> The only test I could currently write for this patch is something like:

>> gdb_test "*(@somerandomqualifiername int *) 0x12345678" "Unknown address space specifier: \"somerandomqualifiername\""> 

>> for a C++ program on any target. If you think that is valuable, I can easily add that.

>> The target I want to use this for in the end won't be ready for upstream for a while unfortunately.

> 

> Hi Felix,

> 

> I think it would be valuable to have a test like this.  It's better

> than nothing, and it's always good to check error cases to make sure GDB

> doesn't crash on them.  I can imagine that this test could test with

> both a C and C++ program to cover everything correctly (and maybe other

> languages, but I don't know much about them, if that even applies to

> them).

> 

> A while ago I added a simavr board file, to be able to run tests against

> an AVR target.  simavr is easy to build and use (it may even be packaged

> in distros, but I'd be tempted to use the latest available version).

> All of this to say you could try to run and improve the flash qualifier

> test for AVR (or write a new test).

> 

> There's just one thing, avr-gcc/avr-g++ seem to produce stabs by

> default, it's not really useful to test with stabs nowadays.  I had a

> patch to make that board use dwarf by default (see below), but I never

> got around to try it properly and post it.  I'll try to do it soon (but

> you can apply it locally in the mean time).

> 

> Simon


Oh, and when running the test (with the dwarf patch applied), I hit an
internal error:


 92 (gdb) PASS: gdb.arch/avr-flash-qualifier.exp: print p
 93 backtrace 1^M
 94 #0  pass_to_function (p=0xe4 <data_in_flash> "\253") at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.arch/avr-flash-qualifier.c:23^M
 95 /home/simark/src/binutils-gdb/gdb/trad-frame.h:143: internal-error: LONGEST trad_frame_saved_reg::addr() const: Assertion `m_kind == trad_frame_saved_reg_    kind::ADDR' failed.^M

I'll try to bisect that, it might be a problem introduced by the
recent trad_frame changes (or the changes merely uncovered an existing
bug).


Simon
Mike Frysinger via Gdb-patches April 5, 2021, 2:32 a.m. | #6
On 2021-04-02 12:51 p.m., Simon Marchi via Gdb-patches wrote:
> Oh, and when running the test (with the dwarf patch applied), I hit an

> internal error:

> 

> 

>  92 (gdb) PASS: gdb.arch/avr-flash-qualifier.exp: print p

>  93 backtrace 1^M

>  94 #0  pass_to_function (p=0xe4 <data_in_flash> "\253") at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.arch/avr-flash-qualifier.c:23^M

>  95 /home/simark/src/binutils-gdb/gdb/trad-frame.h:143: internal-error: LONGEST trad_frame_saved_reg::addr() const: Assertion `m_kind == trad_frame_saved_reg_    kind::ADDR' failed.^M


This is now fixed in master.

Simon
Mike Frysinger via Gdb-patches April 6, 2021, 9:30 a.m. | #7
>-----Original Message-----

>From: Simon Marchi <simon.marchi@polymtl.ca> 

>Sent: Freitag, 2. April 2021 18:49

>

>Hi Felix,

>

>I think it would be valuable to have a test like this.  It's better than nothing, and it's always good to check error cases to make sure

>GDB doesn't crash on them.  I can imagine that this test could test with both a C and C++ program to cover everything correctly (and

>maybe other languages, but I don't know much about them, if that even applies to them).

>

>A while ago I added a simavr board file, to be able to run tests against an AVR target.  simavr is easy to build and use (it may even be

>packaged in distros, but I'd be tempted to use the latest available version).

>All of this to say you could try to run and improve the flash qualifier test for AVR (or write a new test).

>

>There's just one thing, avr-gcc/avr-g++ seem to produce stabs by default, it's not really useful to test with stabs nowadays.  I had a

>patch to make that board use dwarf by default (see below), but I never got around to try it properly and post it.  I'll try to do it soon

> (but you can apply it locally in the mean time).

>

>Simon


I think we are mixing my two patches a bit here, which is of course understandable as they are somewhat related.
However, avr-g++ didn't compile a program using __flash when I tried:
"error: '__flash' does not name a type"
I assume that support is only implemented in avr-gcc and I therefore can't even write an avr test for patch 1.

I opted for writing a general C and C++ test for patch 1, that has the advantage that every normal testsuite run has the chance to catch regressions.

Thanks for all the comments, I will push v2 in a couple of minutes.

Felix

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Patch

diff --git a/gdb/c-exp.y b/gdb/c-exp.y
index c0e4b494f3d..e2ceb2057a1 100644
--- a/gdb/c-exp.y
+++ b/gdb/c-exp.y
@@ -1266,6 +1266,11 @@  single_qualifier:
 		  cpstate->type_stack.insert (pstate,
 					      copy_name ($2.stoken).c_str ());
 		}
+	|	'@' UNKNOWN_CPP_NAME
+		{
+		  cpstate->type_stack.insert (pstate,
+					      copy_name ($2.stoken).c_str ());
+		}
 	;
 
 qualifier_seq_noopt: