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

Runtime debug options [WIP] #170

Closed
wants to merge 3 commits into from
Closed

Runtime debug options [WIP] #170

wants to merge 3 commits into from

Conversation

jakelang
Copy link
Member

fixes #169

@axic
Copy link
Member

axic commented Mar 27, 2018

Sorry I wasn't clear (was just moving issues from the old testnet issue). I'd like to:

  • keep the ifdef, but set the ifdef on by default
  • have the runtime option, but set the debugging off by default

e.g. always compile support for debug, but don't run with debug. Also have the option to compile a much more optimised version (for benchmarking?)

src/eei.h Outdated
@@ -115,6 +119,9 @@ struct EthereumInterface : ShellExternalInterface {
evm_message const& msg;
std::vector<uint8_t> lastReturnData;
ExecutionResult & result;
const bool debug;
/* FIXME */
// static std::ostream& DebugStream;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@axic :
What I am doing here to replace HERA_DEBUG (previously defined by build options) is an ostream object reference that is defined for each instance of EthereumInterface, based on the debug flag. The issue I am having is that I want to use the NullStream struct we have defined already (the one without the large amount of useless inherited public methods), but this will not work because it is not an ostream like std::cerr. What is the best way to do this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chfast can you check this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give an example what is not working?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing is broken at the moment, but I am not sure what is the best way to implement the HERA_DEBUG stream such that it works with runtime debug options.
What I do at the moment is give EthereumInterface an object reference to a stream, which is either NullStream or std::cerr depending on what value of "debug" is passed to the constructor. This doesn't work because NullStream is not an ostream and so DebugStream cannot refer to it.

The two solutions I have in mind are:

  1. Implement NullStream with std::ostream as the base class and inherit a bunch of stuff we don't need
  2. preprocessor hell

This is not super important because I am redoing this later, retaining compile-time options, but I would like to know how to approach this type of issue.

@jakelang
Copy link
Member Author

@axic oh, ok. I would still however like to hear the answer to the question I asked above about the debugstream ^^^

@axic
Copy link
Member

axic commented Mar 30, 2018

Why did you close it? :)

@jakelang
Copy link
Member Author

I am redoing this on a different branch, where i keep the compile-time options and don't do stupid things like pass the debug bool to each function that needs it.

@axic
Copy link
Member

axic commented Mar 30, 2018

You can force push into this branch. Just make sure to reopen it before force pushing, otherwise it wont let it to be reopened.

@jakelang
Copy link
Member Author

@axic I think it makes sense to build this on top of the VM abstraction layer.
It would be much easier to do that first and then integrate debugging streams into that because it is much more strictly object-oriented than the mixed bag of imperative and OO style we have right now.

@jakelang
Copy link
Member Author

This will be implemented on top of the VM abstraction layer, when it is ready.

@axic
Copy link
Member

axic commented Nov 29, 2018

@jakelang plans to resume this or #239 ?

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.

Make debugging a runtime option via set_option
3 participants