Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bluesim: Fix function call arguments in debug function #702

Merged
merged 1 commit into from
May 28, 2024

Conversation

quark17
Copy link
Collaborator

@quark17 quark17 commented May 27, 2024

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.

The EventQueue::print() function was calling bk_clock_name() with the
wrong number of arguments, but fortunately print() is not used anywhere,
as it only exists for use when debugging.  The issue is resolved by
adding an additional parameter to print().  However, since so many
functions in the EventQueue class take the tSimStateHdl as an argument,
it might make sense for the EventQueue constructor to take that value
and store it.
@Vekhir
Copy link
Contributor

Vekhir commented May 28, 2024

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

@quark17 quark17 merged commit e6f95a7 into B-Lang-org:main May 28, 2024
49 checks passed
@quark17 quark17 deleted the bluesim-event-queue-fix branch May 28, 2024 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants