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

[Michael Eager <eager@eagercon.com>]

On 10/10/23 09:16, Michael Eager wrote:
> 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.

To be clear:  the test suite regressions are in the MicroBlaze 
assembler, not in other targets.  These need to be resolved.

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