riscv-software-src / riscv-tools

RISC-V Tools (ISA Simulator and Tests)
1.14k stars 446 forks source link

elf2hex base does not need to be power of 2 just 64bit aligned #268

Open iperryman-xeda opened 5 years ago

iperryman-xeda commented 5 years ago

Had this email exchange with Wei:

Will post a pull request with changes.

Wei Song Tue, Jan 1, 2019 9:13 AM to andrew, me  Hello Ian Perryman,

I think you are right, there is no need to limit the base to power of 2. I think I was trying to limit the base to some aligned address but carelessly set the limit to power of 2.

Would you please revise it and submit a PR. Andrew (cc.) is the maintainer and he will review your PR.

Thanks, Wei

 On 31/12/2018 23:09, Ian Perryman wrote: Happy New Year Wei!

I have a question about this commit you made to the RISCV fesvr project:

b351922b9b6a0c4927923be319be428fcbfed54b Author: Wei Song wsong83@gmail.com AuthorDate: Wed Feb 1 12:46:29 2017 +0000 Commit: Andrew Waterman aswaterman@gmail.com CommitDate: Wed Feb 1 04:46:29 2017 -0800

The change added a new argument to elf2hex for the "base", and there was some code added to ensure the base was a power of 2. Why is the base restricted to a power of 2?

We are building a system where we have multiple RISCV cores, and in our address space we wanted non power of 2 base addresses for some of the cores. Core 0 is always on a power of 2 but the others are not.

Thanks for you help.

Ian Perryman Principal Engineer XtremeEDA

iperryman-xeda commented 5 years ago

Not real sure on the process here.

Could not seem to be able to push my branch back to the origin.

17:10:16 [iperryman@eng-05 riscv-fesvr]$ git push -u origin mpp_elf2hex Username for 'https://github.com': ianperryman Password for 'https://ianperryman@github.com': remote: Permission to riscv/riscv-fesvr.git denied to ianperryman. fatal: unable to access 'https://github.com/riscv/riscv-fesvr.git/': The requested URL returned error: 403

So here is a diff of what I think should be changed:

*** /tmp/ediff328108ly 2019-01-02 17:21:14.122733543 -0500 --- /tmp/ediff32810uvB 2019-01-02 17:21:14.126733873 -0500


* 16,24 ** unsigned long long int base = 0; if(argc==5) { base = atoll(argv[4]); ! if((base & (base-1))) { ! std::cerr << "base must be a power of 2" << std::endl; return 1; } } --- 16,24 ---- unsigned long long int base = 0; if(argc==5) { base = atoll(argv[4]); ! if((base % 8)) { ! std::cerr << "base must be on a 8 byte (64 bit) boundary" << std::endl; return 1; } }

iperryman-xeda commented 5 years ago

Hmmmm paste of diff did not work well in markdown.

"!" should be in margin .... indicated the lines that were changed. They are not part of the code.

iperryman-xeda commented 5 years ago
*** /tmp/ediff328108ly  2019-01-02 17:21:14.122733543 -0500
--- /tmp/ediff32810uvB  2019-01-02 17:21:14.126733873 -0500
***************
*** 16,24 ****
    unsigned long long int base = 0;
    if(argc==5) {
      base = atoll(argv[4]);
!     if((base & (base-1)))
      {
!       std::cerr << "base must be a power of 2" << std::endl;
        return 1;
      }
    }
--- 16,24 ----
    unsigned long long int base = 0;
    if(argc==5) {
      base = atoll(argv[4]);
!     if((base % 8))
      {
!       std::cerr << "base must be on a 8 byte (64 bit) boundary" << std::endl;
        return 1;
      }
    }
aswaterman commented 5 years ago

I don't think the magic constant 8 is what you want - the base must be divisible by whatever the width is. I'll fix it up and push it to the repo.

iperryman-xeda commented 5 years ago

You're right of course!

aswaterman commented 5 years ago

https://github.com/riscv/riscv-fesvr/pull/59

jim-wilson commented 5 years ago

Basic process is that you create a fork of the riscv/riscv-fesvr tree, create a branch in your fork, push a patch to your branch, and then create a pull request in riscv/riscv-fesvr pointing at the patch on your branch in your fork. Github will helpfully ask you if you want to create a pull request when you push a patch to a branch in a fork, and will create an appropriate pull request for you, so the last bit should be easy.

iperryman-xeda commented 5 years ago

Fix for this has been made in riscv-fesvr, but submodule has not been bumped in riscv-tools to pick it up.