[0/5] Add Xtensa ESP chips support

  Since ESP chips are getting more and more popular for developers I
would like to suggest these patches to consider to include Xtensa ESP
chips support for binutils and gdb.

The chips support was done in a not common binutils way. There are a
few reasons for that:
As I know, the Xtensa specific code for binutils and gdb is generated
with tools from Tensilica Inc.
To build binutils and gdb with other chip presets we need to override
some related files (https://github.com/espressif/xtensa-overlays).
To make it easy to integrate new chips I did refactor the code a bit to
make a possibility to just copy and paste Tensilica-generated files and
modify a few lines in a code to make it supported in binutils and gdb.

Please consider these changes to merge, I believe this could be a
pretty improvement to extend supported CPUs without using third-party
sources patches.

Alexey Lapshin (5):
  bfd: xtensa: move common code from ld and gas
  gas: xtensa: add endianness, loops, booleans options
  ld: xtensa: use default LD command line options to specify endianness
  gas: xtensa: add esp32, esp32s2, esp32s3 isa-modules options
  gdb: xtensa: add support for esp32, esp32s2, esp32s3 isa-modules

 bfd/Makefile.am                               |    10 +-
 bfd/Makefile.in                               |    15 +-
 bfd/configure                                 |     4 +-
 bfd/configure.ac                              |     4 +-
 bfd/elf32-xtensa.c                            |   126 +
 ...nsa-modules.c => xtensa-default-modules.c} |     2 +-
 bfd/xtensa-esp32-modules.c                    | 19191 +++++++
 bfd/xtensa-esp32s2-modules.c                  | 11671 +++++
 bfd/xtensa-esp32s3-modules.c                  | 43674 ++++++++++++++++
 bfd/xtensa-isa.c                              |    25 +-
 gas/config/tc-xtensa.c                        |    79 +-
 gas/config/tc-xtensa.h                        |     2 +-
 gas/config/xtensa-relax.c                     |     8 +-
 gdb/Makefile.in                               |    10 +-
 gdb/configure.tgt                             |     2 +-
 ...tensa-config.c => xtensa-config-default.c} |    66 +-
 gdb/xtensa-config-esp32.c                     |   389 +
 gdb/xtensa-config-esp32s2.c                   |   271 +
 gdb/xtensa-config-esp32s3.c                   |   496 +
 gdb/xtensa-tdep.c                             |    70 +-
 gdb/xtensa-tdep.h                             |     2 +-
 include/elf/xtensa.h                          |    22 +
 include/xtensa-isa.h                          |     8 +
 ld/emultempl/xtensaelf.em                     |   169 +-
 24 files changed, 76128 insertions(+), 188 deletions(-)
 rename bfd/{xtensa-modules.c => xtensa-default-modules.c} (99%)
 create mode 100644 bfd/xtensa-esp32-modules.c
 create mode 100644 bfd/xtensa-esp32s2-modules.c
 create mode 100644 bfd/xtensa-esp32s3-modules.c
 rename gdb/{xtensa-config.c => xtensa-config-default.c} (84%)
 create mode 100644 gdb/xtensa-config-esp32.c
 create mode 100644 gdb/xtensa-config-esp32s2.c
 create mode 100644 gdb/xtensa-config-esp32s3.c

-- 
2.34.1
  

Hi Alexey,

On Sat, Oct 22, 2022 at 5:51 AM Alexey Lapshin via Binutils
<binutils@sourceware.org> wrote:
>
> Since ESP chips are getting more and more popular for developers I
> would like to suggest these patches to consider to include Xtensa ESP
> chips support for binutils and gdb.
>
> The chips support was done in a not common binutils way. There are a
> few reasons for that:
> As I know, the Xtensa specific code for binutils and gdb is generated
> with tools from Tensilica Inc.
> To build binutils and gdb with other chip presets we need to override
> some related files (https://github.com/espressif/xtensa-overlays).
> To make it easy to integrate new chips I did refactor the code a bit to
> make a possibility to just copy and paste Tensilica-generated files and
> modify a few lines in a code to make it supported in binutils and gdb.
>
> Please consider these changes to merge, I believe this could be a
> pretty improvement to extend supported CPUs without using third-party
> sources patches.
>
> Alexey Lapshin (5):
>   bfd: xtensa: move common code from ld and gas
>   gas: xtensa: add endianness, loops, booleans options
>   ld: xtensa: use default LD command line options to specify endianness
>   gas: xtensa: add esp32, esp32s2, esp32s3 isa-modules options
>   gdb: xtensa: add support for esp32, esp32s2, esp32s3 isa-modules

I'd like to review this series but it has two technical issues:
- the patches are not in plain text format and so git am is unable to apply them
- the 4th patch of the series didn't make it through the list, I guess
it's too big

I guess the easiest way for me to review this series is if you could point
me to a public git repository with these patches. Alternatively you could
use git send-email to send the patches, split patch 4 into pieces (e.g.
one core per patch) and add me to the cc: list.
  

Hi Max,

Thank you for reaching out. Yes, patch 4/5 is too big and should be
split into at least 6 parts to upload here.

I uploaded changes to the github:
https://github.com/espressif/binutils-gdb/commits/feature/binutils-2_39-port-esp-chips
Regards,
Alexey


On Tue, 2022-10-25 at 12:13 -0700, Max Filippov wrote:
> [External: This email originated outside Espressif]
> 
> Hi Alexey,
> 
> On Sat, Oct 22, 2022 at 5:51 AM Alexey Lapshin via Binutils
> <binutils@sourceware.org> wrote:
> > 
> > Since ESP chips are getting more and more popular for developers I
> > would like to suggest these patches to consider to include Xtensa
> > ESP
> > chips support for binutils and gdb.
> > 
> > The chips support was done in a not common binutils way. There are
> > a
> > few reasons for that:
> > As I know, the Xtensa specific code for binutils and gdb is
> > generated
> > with tools from Tensilica Inc.
> > To build binutils and gdb with other chip presets we need to
> > override
> > some related files (https://github.com/espressif/xtensa-overlays).
> > To make it easy to integrate new chips I did refactor the code a
> > bit to
> > make a possibility to just copy and paste Tensilica-generated files
> > and
> > modify a few lines in a code to make it supported in binutils and
> > gdb.
> > 
> > Please consider these changes to merge, I believe this could be a
> > pretty improvement to extend supported CPUs without using third-
> > party
> > sources patches.
> > 
> > Alexey Lapshin (5):
> >   bfd: xtensa: move common code from ld and gas
> >   gas: xtensa: add endianness, loops, booleans options
> >   ld: xtensa: use default LD command line options to specify
> > endianness
> >   gas: xtensa: add esp32, esp32s2, esp32s3 isa-modules options
> >   gdb: xtensa: add support for esp32, esp32s2, esp32s3 isa-modules
> 
> I'd like to review this series but it has two technical issues:
> - the patches are not in plain text format and so git am is unable to
> apply them
> - the 4th patch of the series didn't make it through the list, I
> guess
> it's too big
> 
> I guess the easiest way for me to review this series is if you could
> point
> me to a public git repository with these patches. Alternatively you
> could
> use git send-email to send the patches, split patch 4 into pieces
> (e.g.
> one core per patch) and add me to the cc: list.
> 
> --
> Thanks.
> -- Max
  

Hi Alexey,

On Tue, Oct 25, 2022 at 1:17 PM Alexey Lapshin
<alexey.lapshin@espressif.com> wrote:
> I uploaded changes to the github:
> https://github.com/espressif/binutils-gdb/commits/feature/binutils-2_39-port-esp-chips

Thank you. I have taken a look and have run a couple tests.
It is nice to see an effort to improve the current state of the xtensa
toolchain, however this series has a number of issues that I consider
important and think that they must be addressed.

First issue is that these changes break existing workflows for the
xtensa toolchain customization. I believe that it is possible to add
support for multiple xtensa cores without breaking the current
configuration mechanism.

Second issue is that these changes are not internally coherent.
For example I would expect that choosing a core that we're
assembling for would result in production of an object file with
correct endianness for the chosen core, but this is not the case:
xtensa-elf-as --isa-module=esp32 will produce an object file
marked as elf32-xtensa-be. New switches that control endianness
only do partial job, affecting literals and data, but not instructions
(which they cannot do by design).
There's an --isa-module switch for the assembler, but nothing
matching it for the objdump. To experience the issue try to
assemble the 'salt' instruction (available in esp32s3) and get it
back from the disassembler. Recording core ID in the .xtensa.info
section is a clever idea that could potentially help in this situation,
but 1) AFAICT it is not used for that purpose now and 2) what's
the option for disassembling code that lacks the .xtensa.info?
Also, recording the sequential number in this section doesn't look
like the right choice to me.
The switches --[no-]booleans and --[no-]loops are not really
documented and AFAICT they do nothing at this point. They don't
control which code is accepted or generated, they only affect
available relaxation transformations, but I don't see any
transformations that depend on the presence of the boolean
option or zero overhead loops.
There are remaining core-specific macros, like XCHAL_HAVE_BE
in the code that is only compiled once, which means that this code
gets definitions for these macros from the default configuration,
leaving the reader of this code with the question "how is it
supposed to work for cores that define these macros differently"?

Third issue is related to the second, this series adds support for
the new cores meaning that other interested parties could follow
suit, but since it's done incoherently it does not set the right
example that could actually be followed.

I still think that the series that I posted back in 2017 here
  https://sourceware.org/pipermail/binutils/2017-May/098159.html
was a step in the right direction, providing basis for dynamic
configuration, yet not excluding the opportunity to have core
support built into the toolchain. It's a shame that I couldn't
complete it back then, I guess I should try to do it this time.
  

Thank you for the review 

> I still think that the series that I posted back in 2017 here
>   https://sourceware.org/pipermail/binutils/2017-May/098159.html

FYI we already use your approach in GDB (with some improvements and
modifications).
Only the difference is that we pass chip name over command-line option,
not through ENV variable.
https://github.com/espressif/esp-xtensaconfig-lib
Line in GDB to use it:
https://github.com/espressif/binutils-gdb/commit/add3053905e646af67692ae1a67fd5ee76e84723#diff-a4fc3be128b23679672d7d28616e553d81c0631f38e9205774721678bbabfcb7R102
The main disadvantage of this is that we need to have duplicated source
files from binutils inside this library. And be careful when upgrading
to another binutils version because some structure declarations could
change.


> First issue is that these changes break existing workflows for the
> xtensa toolchain customization. I believe that it is possible to add
> support for multiple xtensa cores without breaking the current
> configuration mechanism.

Could you elaborate on this? I'm very new here and do not fully
understand the existing workflow and what was broken.


> xtensa-elf-as --isa-module=esp32 will produce an object file
> marked as elf32-xtensa-be. New switches that control endianness
> only do partial job, affecting literals and data, but not
> instructions
> (which they cannot do by design).

I was disappointed here too because in the default binutils
configuration we have:
#define XCHAL_HAVE_BE 1

But in xtensa-module.c:
xtensa_isa_internal xtensa_modules = {
  0 /* little-endian */, 


> what's the option for disassembling code that lacks the .xtensa.info?

Another option could be to write cpu to the elf's e_flags. The initial
code exists. Needs just to add another machines:
https://github.com/bminor/binutils-gdb/blob/1eeb0316304f2d4e2c48aa8887e28c936bfe4f4d/include/elf/xtensa.h#L104
But yes, the problem still exists with any approach for files generated
before these changes (I suppose also for yours from 2017 ). As a
workaround, it could be added command-line options for tools to force
use specified chip configuration..


What if I redo this patch with removing the most definitions from
xtensa-config.h? XCHAL_HAVE_BE, XCHAL_HAVE_ABS, XCHAL_HAVE_ADDX, ..., 
and most all other hardcoded definitions could be gotten from xtensa-
modules.c


On Thu, 2022-10-27 at 08:39 -0700, Max Filippov wrote:
> [External: This email originated outside Espressif]
> 
> Hi Alexey,
> 
> On Tue, Oct 25, 2022 at 1:17 PM Alexey Lapshin
> <alexey.lapshin@espressif.com> wrote:
> > I uploaded changes to the github:
> > https://github.com/espressif/binutils-gdb/commits/feature/binutils-2_39-port-esp-chips
> 
> Thank you. I have taken a look and have run a couple tests.
> It is nice to see an effort to improve the current state of the
> xtensa
> toolchain, however this series has a number of issues that I consider
> important and think that they must be addressed.
> 
> First issue is that these changes break existing workflows for the
> xtensa toolchain customization. I believe that it is possible to add
> support for multiple xtensa cores without breaking the current
> configuration mechanism.
> 
> Second issue is that these changes are not internally coherent.
> For example I would expect that choosing a core that we're
> assembling for would result in production of an object file with
> correct endianness for the chosen core, but this is not the case:
> xtensa-elf-as --isa-module=esp32 will produce an object file
> marked as elf32-xtensa-be. New switches that control endianness
> only do partial job, affecting literals and data, but not
> instructions
> (which they cannot do by design).
> There's an --isa-module switch for the assembler, but nothing
> matching it for the objdump. To experience the issue try to
> assemble the 'salt' instruction (available in esp32s3) and get it
> back from the disassembler. Recording core ID in the .xtensa.info
> section is a clever idea that could potentially help in this
> situation,
> but 1) AFAICT it is not used for that purpose now and 2) what's
> the option for disassembling code that lacks the .xtensa.info?
> Also, recording the sequential number in this section doesn't look
> like the right choice to me.
> The switches --[no-]booleans and --[no-]loops are not really
> documented and AFAICT they do nothing at this point. They don't
> control which code is accepted or generated, they only affect
> available relaxation transformations, but I don't see any
> transformations that depend on the presence of the boolean
> option or zero overhead loops.
> There are remaining core-specific macros, like XCHAL_HAVE_BE
> in the code that is only compiled once, which means that this code
> gets definitions for these macros from the default configuration,
> leaving the reader of this code with the question "how is it
> supposed to work for cores that define these macros differently"?
> 
> Third issue is related to the second, this series adds support for
> the new cores meaning that other interested parties could follow
> suit, but since it's done incoherently it does not set the right
> example that could actually be followed.
> 
> I still think that the series that I posted back in 2017 here
>   https://sourceware.org/pipermail/binutils/2017-May/098159.html
> was a step in the right direction, providing basis for dynamic
> configuration, yet not excluding the opportunity to have core
> support built into the toolchain. It's a shame that I couldn't
> complete it back then, I guess I should try to do it this time.
> 
> --
> Thanks.
> -- Max
  

On Thu, Oct 27, 2022 at 12:39 PM Alexey Lapshin
<alexey.lapshin@espressif.com> wrote:
> > I still think that the series that I posted back in 2017 here
> >   https://sourceware.org/pipermail/binutils/2017-May/098159.html
>
> FYI we already use your approach in GDB (with some improvements and
> modifications).

Wow, cool. I didn't know about that.

> Only the difference is that we pass chip name over command-line option,
> not through ENV variable.

Is there any advantage to it? I know that IDF requires some environment setup
and choosing the target core anyway.

> https://github.com/espressif/esp-xtensaconfig-lib
> Line in GDB to use it:
> https://github.com/espressif/binutils-gdb/commit/add3053905e646af67692ae1a67fd5ee76e84723#diff-a4fc3be128b23679672d7d28616e553d81c0631f38e9205774721678bbabfcb7R102
> The main disadvantage of this is that we need to have duplicated source
> files from binutils inside this library.

I'm curious why? IIRC I got rid of all such dependencies in my version,
but then I could have missed something as I haven't been using it a lot.
Certainly I paid very little attention to the gdb.

> > First issue is that these changes break existing workflows for the
> > xtensa toolchain customization. I believe that it is possible to add
> > support for multiple xtensa cores without breaking the current
> > configuration mechanism.
>
> Could you elaborate on this? I'm very new here and do not fully
> understand the existing workflow and what was broken.

Sure, it's the replacement of the binutils overlay files that wouldn't
work with this change. Description is available here:
 http://wiki.linux-xtensa.org/index.php/Toolchain_Overlay_File
and overlay application is a part of toolchain build systems like
crosstool-NG:
  https://github.com/crosstool-ng/crosstool-ng/blob/crosstool-ng-1.25.0/scripts/functions#L2413
and buildroot:
  https://github.com/buildroot/buildroot/blob/2022.08/package/binutils/binutils.mk#L115

> > what's the option for disassembling code that lacks the .xtensa.info?
>
> Another option could be to write cpu to the elf's e_flags. The initial
> code exists. Needs just to add another machines:
> https://github.com/bminor/binutils-gdb/blob/1eeb0316304f2d4e2c48aa8887e28c936bfe4f4d/include/elf/xtensa.h#L104

Any kind of numbering means that there must be an authority that
will at least make sure that the numbers are unique.
I think we're much better off recording core name in the .xtensa.info
while also providing a fallback mechanism for the case this information
is missing or there's a need to override it.

I think that I need to research why Tensilica hasn't done anything
like that, neither here nor in its internal toolchain.

> What if I redo this patch with removing the most definitions from
> xtensa-config.h? XCHAL_HAVE_BE, XCHAL_HAVE_ABS, XCHAL_HAVE_ADDX, ...,
> and most all other hardcoded definitions could be gotten from xtensa-
> modules.c

That will not remove these definitions from the existing overlays. And also
there's a copy of this header in the gcc tree, which I'd like to keep in sync.
xtensa-dynconfig series was able to deal with these names by redefining
the X?HAL_* macros. I think that we first need to complete getting rid of
the macro use in preprocessor conditionals inside the toolchain source
code. And then provide these macros as compiler built-ins for the target
libraries.
  

On Fri, Oct 28, 2022 at 8:48 AM Max Filippov <jcmvbkbc@gmail.com> wrote:
> I think that we first need to complete getting rid of
> the macro use in preprocessor conditionals inside the toolchain source

...and in other static contexts of course, like static initializers,
array sizes, etc.
  

Thank you for clarification. Now I see that my approach is not really
fit to the current workflow.

I could port your patch from 2017 (with espressif's changes and
comments resolves) to the latest binutils and send it to mailing list.
If you don't mind.

Regards,
Alexey
  

On Sun, Oct 30, 2022 at 11:38 PM Alexey Lapshin
<alexey.lapshin@espressif.com> wrote:
> I could port your patch from 2017 (with espressif's changes and
> comments resolves) to the latest binutils and send it to mailing list.
> If you don't mind.

Sure, I guess we can start with that. I also have a couple improvements in mind,
let's see if we can make something solid this time.