[v2] xtensa: initialize call_abi in xtensa_tdep

  Use XSHAL_ABI value provided by xtensa-config.h to correctly initialize
xtensa_tdep.call_abi
This fixes calls to functions from GDB that otherwise fail with the
following assertion in call0 configuration:

  gdb/regcache.c:602: internal-error: regcache_raw_read: Assertion
  `regnum >= 0 && regnum < regcache->descr->nr_raw_registers' failed.

gdb/
	* xtensa-tdep.h (XTENSA_GDBARCH_TDEP_INSTANTIATE): Initialize
	call_abi using XSHAL_ABI macro.
---
Changes v1 -> v2:
- fix call_abi code formatting.

 gdb/xtensa-tdep.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
  

On Thu, Aug 20, 2015 at 03:52:40PM +0300, Max Filippov wrote:
> Use XSHAL_ABI value provided by xtensa-config.h to correctly initialize
> xtensa_tdep.call_abi
> This fixes calls to functions from GDB that otherwise fail with the
> following assertion in call0 configuration:
> 
>   gdb/regcache.c:602: internal-error: regcache_raw_read: Assertion
>   `regnum >= 0 && regnum < regcache->descr->nr_raw_registers' failed.
> 
> gdb/
> 	* xtensa-tdep.h (XTENSA_GDBARCH_TDEP_INSTANTIATE): Initialize
> 	call_abi using XSHAL_ABI macro.

I think you missed or ignored one comment about the fact that
the code I am seeing in current xtensa-tdep.h is not what your patch
says. So it seems to me you are sending a patch that doesn't seem
to be applying to master.

I would also be beneficial to explore what I was trying to explain
regarding the fact that determining the proper ABI should be done
on the fly, rather than hardcoded. This is particularly true with
the fact that changing the hardcoded values involves adapting
the contents of a file, which is not user-friendly, and nearly
impossible for anyone but a knowledgeable GDB contributor.

> ---
> Changes v1 -> v2:
> - fix call_abi code formatting.
> 
>  gdb/xtensa-tdep.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/xtensa-tdep.h b/gdb/xtensa-tdep.h
> index adacaf8..ff090f3 100644
> --- a/gdb/xtensa-tdep.h
> +++ b/gdb/xtensa-tdep.h
> @@ -246,7 +246,9 @@ struct gdbarch_tdep
>  	  .spill_location = -1,					\
>  	  .spill_size = (spillsz),				\
>  	  .unused = 0,						\
> -	  .call_abi = 0,					\
> +	  .call_abi = (XSHAL_ABI == XTHAL_ABI_CALL0		\
> +		       ? CallAbiCall0Only			\
> +		       : CallAbiDefault),			\
>  	  .debug_interrupt_level = XCHAL_DEBUGLEVEL,		\
>  	  .icache_line_bytes = XCHAL_ICACHE_LINESIZE,		\
>  	  .dcache_line_bytes = XCHAL_DCACHE_LINESIZE,		\
> -- 
> 1.8.1.4
  

> I think you missed or ignored one comment about the fact that
> the code I am seeing in current xtensa-tdep.h is not what your patch
> says. So it seems to me you are sending a patch that doesn't seem
> to be applying to master.
> 
> I would also be beneficial to explore what I was trying to explain
> regarding the fact that determining the proper ABI should be done
> on the fly, rather than hardcoded. This is particularly true with
> the fact that changing the hardcoded values involves adapting
> the contents of a file, which is not user-friendly, and nearly
> impossible for anyone but a knowledgeable GDB contributor.

You answered in the other email:
| I agree with that, but currently we can't distinguish executables with
| different call ABI.

Odd; that means that the linker would not reject the link of
objects that use different calling conventions? Wow, better be
careful!

In any case, I think what should really be done, if it has to be
hard-set in the debugger, is use a configure command-line option.
Or use a GDB setting "set xtensa call-abi [...]".

In the meantime, I have no strong objection to this code going in
on the grounds that it doesn't really make things all that worse.
But given that this is going against what I would recommend, give it
another week so that other Maintainers have a chance to comment
as well if they disagree with letting this in.

> > ---
> > Changes v1 -> v2:
> > - fix call_abi code formatting.
> > 
> >  gdb/xtensa-tdep.h | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/gdb/xtensa-tdep.h b/gdb/xtensa-tdep.h
> > index adacaf8..ff090f3 100644
> > --- a/gdb/xtensa-tdep.h
> > +++ b/gdb/xtensa-tdep.h
> > @@ -246,7 +246,9 @@ struct gdbarch_tdep
> >  	  .spill_location = -1,					\
> >  	  .spill_size = (spillsz),				\
> >  	  .unused = 0,						\
> > -	  .call_abi = 0,					\
> > +	  .call_abi = (XSHAL_ABI == XTHAL_ABI_CALL0		\
> > +		       ? CallAbiCall0Only			\
> > +		       : CallAbiDefault),			\
> >  	  .debug_interrupt_level = XCHAL_DEBUGLEVEL,		\
> >  	  .icache_line_bytes = XCHAL_ICACHE_LINESIZE,		\
> >  	  .dcache_line_bytes = XCHAL_DCACHE_LINESIZE,		\
> > -- 
> > 1.8.1.4
> 
> -- 
> Joel
  

On Thu, Aug 20, 2015 at 4:07 PM, Joel Brobecker <brobecker@adacore.com> wrote:
> On Thu, Aug 20, 2015 at 03:52:40PM +0300, Max Filippov wrote:
>> Use XSHAL_ABI value provided by xtensa-config.h to correctly initialize
>> xtensa_tdep.call_abi
>> This fixes calls to functions from GDB that otherwise fail with the
>> following assertion in call0 configuration:
>>
>>   gdb/regcache.c:602: internal-error: regcache_raw_read: Assertion
>>   `regnum >= 0 && regnum < regcache->descr->nr_raw_registers' failed.
>>
>> gdb/
>>       * xtensa-tdep.h (XTENSA_GDBARCH_TDEP_INSTANTIATE): Initialize
>>       call_abi using XSHAL_ABI macro.
>
> I think you missed or ignored one comment about the fact that
> the code I am seeing in current xtensa-tdep.h is not what your patch
> says.

Sorry, I'm not sure what you mean. I've changed call_abi initialization
in the macro XTENSA_GDBARCH_TDEP_INSTANTIATE in file
gdb/xtensa-tdep.h from 'call_abi = 0' to 'call_abi = (XSHAL_ABI == ...',
which I wrote to the changelog.

> So it seems to me you are sending a patch that doesn't seem
> to be applying to master.

You're right, I haven't rebased v2 to the current master and now it indeed
does not apply. I'll send fixed v3.

> I would also be beneficial to explore what I was trying to explain
> regarding the fact that determining the proper ABI should be done
> on the fly, rather than hardcoded. This is particularly true with
> the fact that changing the hardcoded values involves adapting
> the contents of a file, which is not user-friendly, and nearly
> impossible for anyone but a knowledgeable GDB contributor.

Actually it's as simple as unpacking a tarball to the source directory,
and we have it automated in the environments that build toolchains,
like the Buildroot or the crosstool-NG.

The idea behind this is the following: Xtensa core is configurable, a lot
of its properties may be changed. Nobody even try to test all possible
combinations of configuration options and nobody really cares how many
Xtensa core configurations exist, people that generate Xtensa core
only care about their particular core. When they generate it they get
all the files that need to be changed in the toolchain, they apply them
and they get the toolchain for their particular core.
  

On Thu, Aug 20, 2015 at 4:14 PM, Joel Brobecker <brobecker@adacore.com> wrote:
>> I think you missed or ignored one comment about the fact that
>> the code I am seeing in current xtensa-tdep.h is not what your patch
>> says. So it seems to me you are sending a patch that doesn't seem
>> to be applying to master.
>>
>> I would also be beneficial to explore what I was trying to explain
>> regarding the fact that determining the proper ABI should be done
>> on the fly, rather than hardcoded. This is particularly true with
>> the fact that changing the hardcoded values involves adapting
>> the contents of a file, which is not user-friendly, and nearly
>> impossible for anyone but a knowledgeable GDB contributor.
>
> You answered in the other email:
> | I agree with that, but currently we can't distinguish executables with
> | different call ABI.
>
> Odd; that means that the linker would not reject the link of
> objects that use different calling conventions? Wow, better be
> careful!

Yes, but as long as the calling convention is fixed for the toolchain
this is not a problem.

> In any case, I think what should really be done, if it has to be
> hard-set in the debugger, is use a configure command-line option.
> Or use a GDB setting "set xtensa call-abi [...]".

This only increases the number of moving parts: where previously
only a set of files had to be replaced we'd now have to change a
set of files plus give some configuration options. It isn't needed for
our usecase: the particular build of gdb is meant to be used with
matching build of the compiler, linker and the hardware.

> In the meantime, I have no strong objection to this code going in
> on the grounds that it doesn't really make things all that worse.
> But given that this is going against what I would recommend, give it
> another week so that other Maintainers have a chance to comment
> as well if they disagree with letting this in.
  

On 08/20/2015 02:37 PM, Max Filippov wrote:

> Actually it's as simple as unpacking a tarball to the source directory,
> and we have it automated in the environments that build toolchains,
> like the Buildroot or the crosstool-NG.
> 
> The idea behind this is the following: Xtensa core is configurable, a lot
> of its properties may be changed. Nobody even try to test all possible
> combinations of configuration options and nobody really cares how many
> Xtensa core configurations exist, people that generate Xtensa core
> only care about their particular core. When they generate it they get
> all the files that need to be changed in the toolchain, they apply them
> and they get the toolchain for their particular core.

How about making the configuration generator tool output some data
file that gdb would source?

Thanks,
Pedro Alves
  

Pedro Alves wrote:
> On 08/20/2015 02:37 PM, Max Filippov wrote:

> 

> > Actually it's as simple as unpacking a tarball to the source directory,

> > and we have it automated in the environments that build toolchains,

> > like the Buildroot or the crosstool-NG.

> >

> > The idea behind this is the following: Xtensa core is configurable, a

> lot

> > of its properties may be changed. Nobody even try to test all possible

> > combinations of configuration options and nobody really cares how many

> > Xtensa core configurations exist, people that generate Xtensa core

> > only care about their particular core. When they generate it they get

> > all the files that need to be changed in the toolchain, they apply them

> > and they get the toolchain for their particular core.

> 

> How about making the configuration generator tool output some data

> file that gdb would source?


As I understand it, a simple data file is problematic.  For example,
customers adding custom extensions to an Xtensa processor can describe
instruction operand encoding and decoding using arbitrary verilog
expressions.  Although typically simple, there is currently no constraint
on these expressions, so to fully support this, they get translated into
C code which GDB and other tools can use to assemble and disassemble
Xtensa instructions.

So the more natural "data file" to describe a custom processor is a shared
library or DLL.  This is what Cadence's Tensilica tools use.  However, in
the far past, upstreaming this approach was problematic given the various
projects' aversion to dynamic shared libraries, which aren't supported
on every host architecture (not all support a dlopen equivalent).

Might it be possible now to introduce use of a dynamic shared library?
If that works for GDB, my next question will be about binutils and gcc :-)

Thanks!
-Marc
  

On 08/20/2015 09:30 PM, Marc Gauthier wrote:
> Pedro Alves wrote:
>> On 08/20/2015 02:37 PM, Max Filippov wrote:
>>
>>> Actually it's as simple as unpacking a tarball to the source directory,
>>> and we have it automated in the environments that build toolchains,
>>> like the Buildroot or the crosstool-NG.
>>>
>>> The idea behind this is the following: Xtensa core is configurable, a
>> lot
>>> of its properties may be changed. Nobody even try to test all possible
>>> combinations of configuration options and nobody really cares how many
>>> Xtensa core configurations exist, people that generate Xtensa core
>>> only care about their particular core. When they generate it they get
>>> all the files that need to be changed in the toolchain, they apply them
>>> and they get the toolchain for their particular core.
>>
>> How about making the configuration generator tool output some data
>> file that gdb would source?
> 
> As I understand it, a simple data file is problematic.  For example,
> customers adding custom extensions to an Xtensa processor can describe
> instruction operand encoding and decoding using arbitrary verilog
> expressions.  Although typically simple, there is currently no constraint
> on these expressions, so to fully support this, they get translated into
> C code which GDB and other tools can use to assemble and disassemble
> Xtensa instructions.

Does that mean you're replacing some src/opcodes/ files too, for disassembly?

> So the more natural "data file" to describe a custom processor is a shared
> library or DLL. 

Another option would be some Python API.

> This is what Cadence's Tensilica tools use.  However, in
> the far past, upstreaming this approach was problematic given the various
> projects' aversion to dynamic shared libraries, which aren't supported
> on every host architecture (not all support a dlopen equivalent).
> 

That's not really a problem.  Hosts that can't dlopen just don't
support all features.  From https://sourceware.org/gdb/wiki/Systems,
probably the only one would be MSDOS/djgpp.

E.g., GDB already supports a JIT debug info reader API, which loads a
shared library plugin into GDB.

 https://sourceware.org/gdb/onlinedocs/gdb/Writing-JIT-Debug-Info-Readers.html

Another example, GDB loads a libcc1.so library in order to invoke gcc, in
order to provide the new "compile" set of subcommands.

GCC is also extensible with plugins nowadays.  That's what the libcc1.so
library talks to -- a gcc plugin.

The binutils ld linker has a plugin interface, for loading the LTO plugin.

> Might it be possible now to introduce use of a dynamic shared library?

I think it'd be useful to see a couple representative diffs of pristine
FSF GDB vs a configured GDB, to get a better feel for what needs exposing.

> If that works for GDB, my next question will be about binutils and gcc :-)

Thanks,
Pedro Alves