[PATCH 0/2] Expression parser fixes for address space qualifiers.

[PATCH 0/2] Expression parser fixes for address space qualifiers.

[Felix Willgerodt]

These are some general bug fixes for the address space qualifier parsing in
C/C++ and is pre-work for a target at Intel that will make use of this feature.

I tested this on x86-64 Ubuntu 20.04 and Fedora 31, and the avr parts with
'target sim avr' and avr-gcc 9.2.0 using '-mmcu=atmega328p'.

Any comments very welcome!

Felix

Felix Willgerodt (2):
  gdb: Allow address space qualifier parsing in C++.
  gdb: Fix reduce/reduce conflicts for qualifier_seq_noopt in the C
    parser.

 gdb/c-exp.y | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

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

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

[Felix Willgerodt]

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(+)

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:
-- 
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

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

[Tom Tromey]

>>>>> "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

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

[Willgerodt, Felix]

> 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

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

[Luis Machado]

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
> 

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

[Simon Marchi]

> 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

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

[Simon Marchi]

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

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

[Simon Marchi]

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

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

[Willgerodt, Felix]

>-----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 2/2] gdb: Fix reduce/reduce conflicts for qualifier_seq_noopt in the C parser.

[Felix Willgerodt]

This fixes a problem with GDB's address space qualifier parsing.  GDB uses
'@' as a way to express an address space in expression evaluation.  This can
currently lead to a crash for "Add support for the __flash qualifier on AVR"
(487d975399dfcb2bb2f0998a7d12bd62acdd9fa1), the only user I am aware of.

Program:
~~~
const __flash char data_in_flash = 0xab;

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

Before:
~~~
(gdb) p data_in_flash
$1 = -85 '\253'
(gdb) p *(const char * @flash) pointer_to_flash
$2 = -85 '\253'
(gdb) p *(@flash const char *) pointer_to_flash
type-stack.c:201: internal-error: type* type_stack::follow_types(type*): unrecognized tp_ value in follow_types
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n)
~~~

After:
~~~
(gdb) p data_in_flash
$1 = -85 '\253'
(gdb) p *(const char *) pointer_to_flash
$2 = 0 '\000'
(gdb) p *(const char * @flash) pointer_to_flash
$3 = -85 '\253'
(gdb) p *(@flash const char *) pointer_to_flash
$4 = 0 '\000'
(gdb)
~~~

Note that how the binding of this qualifier is interpreted and resolved for an
address/pointer is target specific.  Hence only the prepended qualifier works
for AVR, even if it seems syntactically incorrect.  I won't change this for
AVR, as I am not familiar with that target.

Bison now also complains about less conflicts:

Before:
  YACC   c-exp.c
gdb/gdb/c-exp.y: warning: 153 shift/reduce conflicts [-Wconflicts-sr]
gdb/gdb/c-exp.y: warning: 70 reduce/reduce conflicts [-Wconflicts-rr]

After:
  YACC   c-exp.c
gdb/gdb/c-exp.y: warning: 60 shift/reduce conflicts [-Wconflicts-sr]
gdb/gdb/c-exp.y: warning: 69 reduce/reduce conflicts [-Wconflicts-rr]

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

	* c-exp.y (qualifier_seq_noopt): Replace qualifier_seq with
	qualifier_seq_noopt.
---
 gdb/c-exp.y | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/c-exp.y b/gdb/c-exp.y
index e2ceb2057a1..256937f2034 100644
--- a/gdb/c-exp.y
+++ b/gdb/c-exp.y
@@ -1275,7 +1275,7 @@ single_qualifier:
 
 qualifier_seq_noopt:
 		single_qualifier
-	| 	qualifier_seq single_qualifier
+	| 	qualifier_seq_noopt single_qualifier
 	;
 
 qualifier_seq:
-- 
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

Re: [PATCH 2/2] gdb: Fix reduce/reduce conflicts for qualifier_seq_noopt in the C parser.

[Tom Tromey]

>>>>> "Felix" == Felix Willgerodt via Gdb-patches <gdb-patches@sourceware.org> writes:

Felix> Note that how the binding of this qualifier is interpreted and resolved for an
Felix> address/pointer is target specific.  Hence only the prepended qualifier works
Felix> for AVR, even if it seems syntactically incorrect.  I won't change this for
Felix> AVR, as I am not familiar with that target.

Probably we should follow whatever GCC does here.

Felix> 	* c-exp.y (qualifier_seq_noopt): Replace qualifier_seq with
Felix> 	qualifier_seq_noopt.

This seems fine to me.  Thanks again.

Tom

RE: [PATCH 2/2] gdb: Fix reduce/reduce conflicts for qualifier_seq_noopt in the C parser.

[Willgerodt, Felix]

>Felix> Note that how the binding of this qualifier is interpreted and 
>Felix> resolved for an address/pointer is target specific.  Hence only 
>Felix> the prepended qualifier works for AVR, even if it seems 
>Felix> syntactically incorrect.  I won't change this for AVR, as I am not familiar with that target.

Tom>Probably we should follow whatever GCC does here.

We should, but we don't for AVR (the binding in GDBs internal type representation is correct). I checked with avr-gcc 9.2.0 and 'target sim avr', hence this paragraph.
But I am not familiar enough with AVR to fix this, as it would require changes to the avr-tdep file and the address tagging/translation logic in there.
	
Thanks, I will push this together with the first patch once the testing question is resolved there.
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