B-Lang-org / bsc

Bluespec Compiler (BSC)
Other
954 stars 145 forks source link

Bluesim: Fix function call arguments in debug function #702

Closed quark17 closed 5 months ago

quark17 commented 5 months ago

The EventQueue::print() function was calling bk_clock_name() with the wrong number of arguments, resulting in the warning reported in #698:

Bluesim object created: model_mkTbGCD.{h,o}
../../../../src/bluesim/event_queue.cxx:8:24: warning: ‘bk_clock_name’ violates the C++ One Definition Rule [-Wodr]
    8 | extern "C" const char* bk_clock_name(tClock handle);
      |                        ^
../../../../src/bluesim/kernel.cxx:1093:13: note: type mismatch in parameter 1
 1093 | const char* bk_clock_name(tSimStateHdl simHdl, tClock clk)
      |             ^

If the print() function were ever called, this would probably segfault. Fortunately, print() is not used anywhere, as it only exists for use when debugging.

Originally, Bluesim only had one global simulation state, but to support having multiple simulations running in the same program, the Bluesim kernel functions were changed to take a pointer to the simulation state (tSimStateHdl). It's likely that, during that change, the print() function -- and the extern declaration for bk_clock_name -- was overlooked.

This PR fixes the extern declaration to have the right arguments, by adding a tSimStateHdl argument. The print() method then needs to have a tSimStateHdl argument, to be able to pass to the call to bk_clock_name.

That resolves the warning. Although we also could have resolved the warning by deleting print() and the extern declaration, since it's unused.

I notice that many functions in the EventQueue class take the tSimStateHdl as an argument. I don't believe that we need a single EventQueue to support multiple simulations, so it might make sense for the EventQueue constructor to take the simulation state and store it, and then the function calls wouldn't need it as an argument. That would be a bigger change than I'm prepared to do now, though.

Vekhir commented 5 months ago

I can confirm that this commit removes the warning and lets the tests pass.