riscv-isa-sim icon indicating copy to clipboard operation
riscv-isa-sim copied to clipboard

Teach cfg_t to "freeze" and add isa parsing to it

Open rswarbrick opened this issue 2 years ago • 3 comments

Split out from #938

The idea here is that we want to compute some "derived" configuration in cfg_t, which can depend on more than one of the config arguments. Specifically, we've got an isa_parser_t that we want to construct based on the "isa" and "priv" strings.

One way to do that would be to parse the strings on the fly in some sort of getter function. A problem there is that we'd end up parsing them multiple times (on each call to the getter).

To avoid that re-computation, we could add a local cache. But that's a bit awkward too: we'd like to pass a "const cfg_t *" around, but then we'd have to mess around with const_casts to allow us to mutate the cache. What's more, it would probably be better to do the parsing at a known time so that we can generate any error messages at a time we expect.

The "proper RAII way" to do this is probably to define some expanded version of cfg_t, whose constructor does all the parsing. Then we'd pass that expanded configuration around afterwards. Here, we do something slightly more lightweight and just expect users not to alter any of the (public) configuration arguments once they've called freeze().

rswarbrick avatar Apr 12 '22 14:04 rswarbrick

@scottj97: Passing the const object is essentially what we're doing already. The bool frozen field is just checking that we didn't mess up in spike.cc and forget to call the freeze method.

You're right that we could use mutable here, but I'm still not a big fan: the isa_parser_t constructor can fail (e.g. if the ISA string is bogus) and it feels cleaner to me to know when exactly we're calling it.

I'm happy to remove the frozen status and just leave it up to the callers. Or I'm happy to do the "frozen_cfg_t" class approach. But it looks like you and @aswaterman slightly disagree, which makes it difficult to know what's best to do :-D

Any thoughts?

rswarbrick avatar Apr 13 '22 19:04 rswarbrick

@scottj97's suggestion is also fine with me: it renders my suggestion moot, so you can ignore mine.

aswaterman avatar Apr 13 '22 19:04 aswaterman

I think what I was picturing before all your recent work is a cfg_t class that could merge two instances of itself, creating a new merged cfg_t object. We would construct one from the cmdline, one from the device tree, then merge them.

Or perhaps there's a third one built first with all the default values? Then merge in the cmdline, then merge in the device tree. The tricky part will be dealing with conflicts, but I suspect this will simplify things versus the current direction.

Ideally, each object would be immutable after construction. Certainly, the merged object would be immutable after construction. This way you could do everything at a well-defined time in the constructor.

scottj97 avatar Apr 13 '22 19:04 scottj97