lowRISC / opentitan

OpenTitan: Open source silicon root of trust
https://www.opentitan.org
Apache License 2.0
2.44k stars 730 forks source link

[dvsim ] primary_build_mode: default in <IP> cfg is invalid #23647

Open desorya opened 4 weeks ago

desorya commented 4 weeks ago

Greetings @rswarbrick ,

I'd like to ask if it is mandatory for a regression to contain the test case that using the default build mode, otherwise it will trigger a primary_build_mode: default error.

For example, a part of my hjson as shown below. I have several build modes regarding different defined flags' values, and a regression contains the test cases using different build modes except default.

build_opts: ["+define+ARM_PROP_DELAY=0"]

build_modes: [
            {
                name: s100
                build_opts: ["+define+SUCCESS_100"]
                is_sim_mode: 1
            }

            {
                name: s50_cal100
                build_opts: [
                             "+define+SUCCESS_50",
                             "+define+CAL_BITS_100"
                            ]
                is_sim_mode: 1
            }
]

    tests: [
        {
            name:           sleep_awake
            uvm_test_seq:   sleep_awake_seq
            uvm_test:       sleep_awake_test
        }

        {
            name:           chn_random_fsm
            uvm_test_seq:   chn_random_fsm_seq
            uvm_test:       chn_random_fsm_test
            build_mode:     "s50_cal100"
            run_opts:      [
                            "+define+SUCCESS_50",
                            "+define+CAL_BITS_100"
                           ]
        }

        {
            name:           cal_read_debug
            uvm_test_seq:   cs_cal_read_debug_seq
            uvm_test:       cs_cal_read_debug_test
            build_mode:     "s100"
            run_opts:      ["+define+SUCCESS_100"]
        }
]

    regressions: [
        {
            name: latest_failed
            tests: [
                    "chn_random_fsm",
                    "cal_read_debug"
            ]
        }
]

However, when I run the regression it raises the error

ERROR: [SimCfg] "primary_build_mode: default" in single_macro cfg is invalid. Please pick from {'s50_cal100', 's100'}

But it works well by adding an test case(sleep_awake here) using default build mode.

Thank you!

rswarbrick commented 3 weeks ago

That's a bit silly and sounds like a bug in the dvsim tool (which we hadn't seen, because we've always got a "default" build mode). The error message is coming from SimCfg.py, line 473. I've just done a bit of debugging and the problem comes from the fact that the default behaviour in SimCfg.py (line 198) is to set primary_build_mode to equal its "build_mode". This, in turn, comes from a few lines earlier where it gets set to "default".

That initial values comes from commit 09a81e92da2, which is where dvsim got initially implemented(!). That, in itself, seems a little silly: we should probably start off with None and then pick a default value once things have been assigned a bit later.

If you're happy to try to sort this out, that would be fantastic! Please file a PR! If not, I'm afraid you'll have to do the workaround you described already. (In either case, please leave this issue open: it's describing a genuine bug in the tooling).

desorya commented 3 weeks ago

Seems this error only occurs when running the regression.

For my case itself, I temporarily applied a trick to the block starting from line 472 in SimCfg.py into:

if self.primary_build_mode not in build_mode_names and self.primary_build_mode!="default":
            log.error(f"\"primary_build_mode: {self.primary_build_mode}\" "
                      f"in {self.name} cfg is invalid. Please pick from "
                      f"{build_mode_names}.")
            sys.exit(1)

to skip this `error`, but I'm afraid this should not be a robust solution and the solutions to different tools may also be different(I only have license to VCS).

Several simple questions are:

Personal opinion, I think if a regression doesn't contain cases under default mode at all, this missing can be then ignored. And if the question I is correct, it seems that the default mode will no miss at all and it is somehow reasonable to ignore missing-default-mode-error. In this case, we may only need set a reminder that the default mode will be regarding as a specific term and user should avoid using this name as the name of their modes.