riscvarchive / riscv-fesvr

RISC-V Frontend Server
Other
62 stars 83 forks source link

Approach for exposing HTIF options to other tools #25

Closed seldridge closed 6 years ago

seldridge commented 7 years ago

This provides one strategy for exposing what options are supported by the host to other tools. This uses getopt_long for all argument parsing and exposes the data structures necessary for the parsing in htif_t.h.

As it's necessary to still provide backwards support for spike and elf2hex, two additional legacy constructors are provided for htif_t:

Summarily:

Fixes #24.

Due to the use of the legacy std::vector<std::string> this should, theoretically, not break anything. However, it may... I was unaware until trying to build riscv-torture that elf2hex wants to pass empty options into the htif_t constructor.

This is intended to be backed up by freechipsproject/rocket-chip#597.

aswaterman commented 6 years ago

Is there a getopt_long that isn’t GPL? I thought it was infectious, but maybe I’m misremembering.

seldridge commented 6 years ago

Yes, there's a BSD implementation (https://github.com/freebsd/freebsd/blob/master/include/getopt.h).

Also, getopt/getopt_long are libc (which in my understanding is LGPL and avoids any problems here). Equivalently, I think this is the same territory as include <stdio.h>.

I defer to an expert here, as this isn't something that you want to get wrong...

Some earlier discussion of this here: https://github.com/freechipsproject/rocket-chip/issues/484#issuecomment-265831119

aswaterman commented 6 years ago

Indeed, my memory is faulty. @palmer-dabbelt any objections?

palmer-dabbelt commented 6 years ago

It looks like it's a GNU extension, but that's not a GPL thing -- we're in the same license territory as linking against anything else in libc. I'm OK with it.

neelgala commented 6 years ago

Hi, There are zero argument commands in spike as well: spike --dump-dts. (A binary should not be needed to generate dts). However, htif fails on this and I believe this particular commit/pull-request is responsible for this.

should I be filing a new issue or would you prefer opening this again?

seldridge commented 6 years ago

@neelgala: thanks, you are right about this. Spike should be able to handle a one-argument option. I think the fix is a lot simpler initially, however... spike creates a sim_t object before it knows if it's going to exit (https://github.com/riscv/riscv-isa-sim/blob/9d1e10a36e771bf8cfbf515e07e856e021c1007a/spike_main/spike.cc#L157).

For the time being, the binary doesn't appear to have to actually exist:

spike --dump-dts foo

This is a new issue on spike. I expect I can get a PR together that will fix this and a second that aligns spike's option parsing with fesvr.

Edit: Ignore my comment about moving the sim_t object. Totally wrong. I looked at this too quickly.

seldridge commented 6 years ago

Digging into this more... the existing (though undocumented) way to specify a "no-binary htif" is by passing "none" as the program (see: https://github.com/riscv/riscv-fesvr/blob/master/fesvr/htif.cc#L79). This code path is never hit by spike --dump-dts none, but this appears to be the (ancient) kludge that's in there now. If this is the way that we want to go, then this "none" escape should be reflected in the option parsing.

This probably warrants a broader discussion related to whether or not htif should be mandating a program to load. @aswaterman: what do you think?