Re: [PATCH 0/5] Add Xtensa ESP chips support

[Alexey Lapshin <alexey.lapshin@espressif.com>]

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