Re: [PATCH v2 1/1] opcodes: microblaze: Add new bit-field instructions

[Michael Eager <eager@eagerm.com>]

On 10/8/23 23:32, Frager, Neal wrote:
> Hi Michael, Maciej,
> 
>> Neal --
> 
>> Please resubmit this patch.
> 
>> Add test cases for new instructions.
>> Fix GNU Coding Standard issues.
>> Run binutils test suite.
> 
> Thank you for feedback.  Could you possibly help me with resolving the GNU coding standard issues and the build issue Maciej is reporting?

See other messages for GNU Coding Standards.

Maciej did not report a build issue: there are test case regressions.
See his instructions on running the binutils test suite.

> 
> Currently, the problem we are facing is that the upstream binutils is unable to build the zynqmp pmufw application.  I am trying to resolve this by upstreaming the toolchain patches AMD is currently using in its microblaze toolchain release.
> 
> This is the only patch remaining that is preventing the zynqmp pmufw application from building, as the application needs the microblaze bsefi and bsifi instructions.
> 
> My patch for adding these instructions is coming from the following two patches which AMD currently applies on its own microblaze toolchain that comes with our yocto releases:
> https://github.com/Xilinx/meta-xilinx/blob/master/meta-microblaze/recipes-devtools/binutils/binutils/0004-LOCAL-Fix-relaxation-of-assembler-resolved-reference.patch
> https://github.com/Xilinx/meta-xilinx/blob/master/meta-microblaze/recipes-devtools/binutils/binutils/0008-Add-new-bit-field-instructions.patch

I don't have time to research patches in a out-of-tree repository.  I 
did look at the other patches you submitted to see if there was some 
information about what problem the patch was solving and found nothing.
If there is relevant information in these other patches, include it with 
your patch.

> 
> As I am far away from being a toolchain or GNU binutils expert, could you please help me in the simplest of terms understand what is not following the GNU coding standard in these patches, so that I can fix it?

See other message.

> 
> My current way of testing is verifying that the master branch build of GNU binutils is able to build the zynqmp pmufw application and that it executes correctly on zynqmp hardware.  So I have verified on hardware that the v2 version of the patch I submitted is indeed working.  If you could help me with resolving these regressions, so that this patch can solve the problem I am trying to solve without causing another problem, I would appreciate any support you can provide.

That may be satisfactory for AMD's requirements, but your patch has to 
work with other targets.  I'm happy to help you, but my time is limited. 
  You (or someone else) will have to do the heavy lifting to resolve 
regressions.

For each of the test suite failures which Maciej reported, reproduce the 
problem outside of the test suite.  I believe there are error messages.

> 
> Also, are there any instructions for coding standard for generating test cases for the bsefi and bsifi instructions?  As I have never written GNU binutils test cases before, perhaps you can point me to something that will help me to save time with this as well?

To test the functions in your patch, you will need to create a small 
assembler test case which invokes the correct relocations.  The bare 
bones "is the opcode correct" test which you added will not do this.

I'll take a look at the MB test cases and see if there is one you can 
adapt.

> 
> Thank you for your support!
> 
> Best regards,
> Neal Frager
> AMD
> 
>> On Fri, 6 Oct 2023, Michael Eager wrote:
>>
>>>> This patch has been tested for years of AMD Xilinx Yocto releases as
>>>> part of the following patch set:
>>>>
>>>> https://github.com/Xilinx/meta-xilinx/tree/master/meta-microblaze/re
>>>> cipes-devtools/binutils/binutils
>>>>
>>>> Signed-off-by: nagaraju <nagaraju.mekala@amd.com>
>>>> Signed-off-by: Ibai Erkiaga <ibai.erkiaga-elorza@amd.com>
>>>> Signed-off-by: Neal Frager <neal.frager@amd.com>
>>>
>>> Committed.
>>
>>    Yet it has caused numerous regressions:
>>
>> microblaze-elf  +FAIL: unordered .debug_info references to
>> .debug_ranges microblaze-elf  +FAIL: binutils-all/pr26548
>> microblaze-elf  +FAIL: readelf -Wwi pr26548e (reason: unexpected
>> output) microblaze-elf  +FAIL: readelf --debug-dump=loc locview-1
>> (reason: unexpected output) microblaze-elf  +FAIL: readelf
>> --debug-dump=loc locview-2 (reason: unexpected output) microblaze-elf
>> +FAIL: readelf -wiaoRlL dw5 microblaze-elf  +FAIL: readelf -wi
>> dwarf-attributes (reason: unexpected output) microblaze-elf  +FAIL:
>> readelf -wKis -P debuglink (reason: unexpected output) microblaze-elf
>> +FAIL: readelf --debug-dump=links --debug-dump=no-follow-links dwo
>> microblaze-elf  +FAIL: DWARF2 1 microblaze-elf  +FAIL: DWARF2 2
>> microblaze-elf  +FAIL: DWARF2 3 microblaze-elf  +FAIL: DWARF2 5
>> microblaze-elf  +FAIL: DWARF2 6 microblaze-elf  +FAIL: DWARF2 7
>> microblaze-elf  +FAIL: DWARF2 11 microblaze-elf  +FAIL: DWARF2 12
>> microblaze-elf  +FAIL: DWARF2 13 microblaze-elf  +FAIL: DWARF2 14
>> microblaze-elf  +FAIL: DWARF2 15 microblaze-elf  +FAIL: DWARF2 16
>> microblaze-elf  +FAIL: DWARF2 17 microblaze-elf  +FAIL: DWARF2 18
>> microblaze-elf  +FAIL: DWARF2 19 microblaze-elf  +FAIL: DWARF2_20:
>> debug ranges ignore non-code sections microblaze-elf  +FAIL: DWARF2 21
>> microblaze-elf  +FAIL: DWARF5 .loc 0 microblaze-elf  +FAIL: DWARF4 CU
>> microblaze-elf  +FAIL: DWARF5 CU microblaze-elf  +FAIL: Check line
>> table is produced with .nops microblaze-elf  +FAIL: line number
>> entries for section changes inside .irp microblaze-elf  +FAIL: line
>> number entries for .macro expansions microblaze-elf  +FAIL: line
>> number entries for expansions of .macro coming from .include
>> microblaze-elf  +FAIL: lns-duplicate microblaze-elf  +FAIL:
>> lns-common-1 microblaze-elf  +FAIL: ld-elf/pr22450
>>
>> They all seem to follow a similar pattern, e.g:
>>
>> exited abnormally with 1, output:readelf: Warning: Corrupt unit length
>> (got 0x4e00004e expected at most 0x4e) in section .debug_info
>>
>> FAIL: DWARF2 1
>>
>> or:
>>
>> exited abnormally with 0, output:readelf: Warning: Corrupt unit length
>> (got 0x20000020 expected at most 0x20) in section .debug_info
>>
>> FAIL: Check line table is produced with .nops
>>
>> Configured with:
>>
>> $ /path/to/configure --disable-nls --disable-gdb --disable-gdbserver
>> --disable-gprofng --disable-libbacktrace --disable-libdecnumber
>> --disable-readline --disable-sim --enable-obsolete --enable-plugins
>> --build=powerpc64le-linux --target=microblaze-elf
>>
>>    Just reporting in case it's useful as it has popped up in unrelated
>> verification; I won't do anything else here.
>>
>>    NB I've skimmed over the change and noticed it does not follow the
>> GNU Coding Standards in many places, most prominently in `get_field_imm5width'
>> and `get_field_rfsl', but also elsewhere (which is easy to spot
>> looking through the diff).
>>
>>     Maciej
>>
> 
>> --
>> Michael Eager