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

[Max Filippov <jcmvbkbc@gmail.com>]

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