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

[Michael Eager <eager@eagercon.com>]

On 10/9/23 23:54, Frager, Neal wrote:
> 
> Hi Michael,
> 

> I just submitted v4 of the patch with the following updates:
>    - Cleaned up all of the GNU coding standard issues.
>    - Removed any line modifications which were unnecessary, so v4 of the patch looks much cleaner now.
>    - Tested this patch on the AMD functional side in that it not only builds, but the built binutils are able to build the zynqmp pmufw application correctly.
>    - Verified that the 32-bit host machine build issue is resolved.

Being able to build and execute in your environment is not adequate.
You need to verify that it does not break other targets.

> As far as I can tell, the only thing missing is a test case for the gas testsuite to verify the relocation works as expected.
> 
> Would you be ok with applying the current v4 of the patch while I work on this last part?

The test suite needs to run cleanly, without regressions, before the 
patch can be applied.

A test case to confirm that the relocation works would be good, but is 
not a requirement.

>> 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.
> 
> Are you sure that the problems Maciej reported can be reproduced outside of the test suite?  If so, how?

Build binutils using the options that Maciej used.  Do not apply your 
latest patch.

Run the test suite ("make check").

Look at testsuite/gas.log.  Save it.

Apply the patch, run make, run make check.

Look at testsuite/gas.log.  Diff with saved version.

In the log, there are results of running each test, indicating failures.
You can run these same commands (as-new, readelf) from the command line.

FAIL: DWARF2 1
../as-new  --compress-debug-sections  -o tmpdir/dwarf2-2.o 
binutils/gas/testsuite/gas/elf/dwarf2-2.s
Executing on host: sh -c {../as-new  --compress-debug-sections  -o 
tmpdir/dwarf2-2.o binutils/gas/testsuite/gas/elf/dwarf2-2.s 2>&1} 
/dev/null dump.tmp (timeout = 300)
spawn [open ...]
build/gas/testsuite/../../binutils/readelf  -w tmpdir/dwarf2-2.o > 
tmpdir/dump.out
Executing on host: sh -c {build/gas/testsuite/../../binutils/readelf  -w 
tmpdir/dwarf2-2.o > tmpdir/dump.out 2>dump.tmp}  /dev/null  (timeout = 300)
spawn [open ...]
exited abnormally with 1, output:readelf: Warning: Corrupt unit length 
(got 0x4e00004e expected at most 0x4e) in section .debug_info

>> 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.

Look at binutils/gas/testsuite/gas/microblaze/reloc_sym.{s,d}.


> 
> Understood and thank you for your help.  I will work on this now.
>   
> Thank you for your support!
>   
> Best regards,
> Neal Frager
> AMD

-- 
Michael Eager