Bug 19921

Hi Jamey,

  I would prefer the option with the least impact on the current sources.  Ie the one which would introduce the least number of potential new bugs.

  To me this sounds like your third option - a new option to objcopy - since in theory the new code would be restricted to just that program, and not need to affect the BFD library itself.  Does this make sense ?

Cheers
  Nick

After talking about it with others who use the verilog output, a new option seems best because it is the most flexible. The data width of a ROM does not necessarily match the data width of the processor.

I think the changes will be very small in this case. A new option in objcopy and a change to verilog.c to use the specified data width.

Created attachment 9170 [details]
patch adding --verilog-data-width option to objcopy

This patch adds --verilog-data-width option to objcopy. It includes several test cases.

This works fine, but the byte ordering caused by "for (i = VerilogDataWidth-1; i >= 0; i--)" is a bit unexpected, at least on OpenRISC. I'm not sure yet if we need to change the ordering only on BE targets, or if should be reversed on both LE and BE targets

Hi Jamey,

> This patch adds --verilog-data-width option to objcopy. It includes several
> test cases.

The BE vs LE issue mentioned by Olof seems to be the most important issue, but
there are a couple more comments that I would like to add:

  * Please do not use C++ style // comments.

  * You should add a line to the binutils/NEWS file mentioning this new feature.
    You should also add some text to binutils/doc/binutils.texi describing the
    new objcopy option and how to use it.

Cheers
  Nick

Created attachment 9179 [details]
revised patch

Hi Jamey,

> revised patch

Thanks for this - but you need to update the tests to cope with both types of endian target.

Cheers
  Nick

Full ack from the OpenRISC side.

Would be nice to get rid of the Windows (CR-LF) line endings as well in the verilog backend, but that can wait for another patch I guess

Adding a comment here to ask if someone could actually work on this bug. 
I reported it on the RISC-V tools github here.

https://github.com/riscv/riscv-tools/issues/168#issuecomment-358165027

When I first reported it was assumed it would have been fixed and pushed upstream by now. Apparently we were wrong and now someone else also wants the same functionality and  found my original report. 

Can someone please fix this? This functionality is useful for bare metal RISC-V Verilog processors.

The master branch has been updated by Nick Clifton <nickc@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=37d0d09177dc02e0002ab8b90d9b7bc402af9240

commit 37d0d09177dc02e0002ab8b90d9b7bc402af9240
Author: Jamey Hicks <jamey.hicks@gmail.com>
Date:   Tue May 14 10:40:04 2019 +0100

    Add new option to objcopy: --verilog-data-width.  Use this option to set the size of byte bundles generated in verilog format files.
    
    	PR 19921
    binutils* objcopy.c: Add new option --verilog-data-width.  Use it to set
    	the value of VerilogDataWidth.
    	* doc/binutils.texi: Document the new option.
    	* testsuite/binutils-all/objcopy.exp: Run tests of new option.
    	* testsuite/binutils-all/verilog-1.hex: New file.
    	* testsuite/binutils-all/verilog-2.hex: New file.
    	* testsuite/binutils-all/verilog-4.hex: New file.
    	* testsuite/binutils-all/verilog-8.hex: New file.
    	* NEWS: Mention the new feature.
    
    bfd	* verilog.c: (VerilogDataWidth): New variable.
    	(verilog_write_record): Emit bytes in VerilogDataWidth bundles.

(In reply to Donato Kava from comment #9)
> Adding a comment here to ask if someone could actually work on this bug. 

Well nobody else seems to want to fix the testsuite problems, so I have
gone ahead and done so myself.  A little bit of code tidying as well,
and the patch is now in.

Cheers
  Nick

Created attachment 11773 [details]
attachment-42343-0.html

Thank you, Nick!


On Tue, May 14, 2019 at 5:51 AM nickc at redhat dot com <
sourceware-bugzilla@sourceware.org> wrote:

> https://sourceware.org/bugzilla/show_bug.cgi?id=19921
>
> Nick Clifton <nickc at redhat dot com> changed:
>
>            What    |Removed                     |Added
>
> ----------------------------------------------------------------------------
>              Status|NEW                         |RESOLVED
>          Resolution|---                         |FIXED
>
> --- Comment #11 from Nick Clifton <nickc at redhat dot com> ---
> (In reply to Donato Kava from comment #9)
> > Adding a comment here to ask if someone could actually work on this bug.
>
> Well nobody else seems to want to fix the testsuite problems, so I have
> gone ahead and done so myself.  A little bit of code tidying as well,
> and the patch is now in.
>
> Cheers
>   Nick
>
> --
> You are receiving this mail because:
> You reported the bug.