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

Problems attempting to redefine VL_PRINTF #1598

Closed
veripoolbot opened this issue Nov 14, 2019 · 7 comments
Closed

Problems attempting to redefine VL_PRINTF #1598

veripoolbot opened this issue Nov 14, 2019 · 7 comments

Comments

@veripoolbot
Copy link

@veripoolbot veripoolbot commented Nov 14, 2019


Author Name: Julien Margetts
Original Redmine Issue: 1598 from https://www.veripool.org

Original Assignee: Julien Margetts


I need my verilated model to call something other than printf to perform output $display, and the intended way to do that (according to verilated.h) appears to be to re-define VL_PRINTF

However doing so seems to cause a number of unexpected warnings. It is also unclear what the proper approach should be to make the verilator generated make file 'aware' of the declaration of my new output function (I.e. how do I tell verilator to put "#include "my_printf.h" into the verilated output code)

I'm sure I'm making a schoolboy error here but any pointers would be helpful.

verilator -cc --exe -O3 -Wno-lint -Wno-UNOPTFLAT -Wno-COMBDLY -LDFLAGS "-fPIC --shared" --top-module xxxx_wrapper --unroll-count 150 -Mdir obj -CFLAGS "-fPIC -DVL_PRINTF=my_printf" +incdir+. -f ../core.flist --clk clk verilated_xxxx_wrapper.cpp -o ../verilated_xxxx_wrapper.so
make -s -j 4 -C obj OPT=-Os -f Vnvme_core_wrapper.mk 
make[2]: Entering directory `xxxxx/verilator/obj'
<command-line>:0:0: warning: "VL_PRINTF" redefined [enabled by default]
<command-line>:0:0: note: this is the location of the previous definition
<command-line>:0:0: warning: "VL_PRINTF" redefined [enabled by default]
<command-line>:0:0: note: this is the location of the previous definition
<command-line>:0:0: warning: "VL_PRINTF" redefined [enabled by default]
<command-line>:0:0: note: this is the location of the previous definition
<command-line>:0:0: warning: "VL_PRINTF" redefined [enabled by default]
<command-line>:0:0: note: this is the location of the previous definition
<command-line>:0:0: warning: "VL_PRINTF" redefined [enabled by default]
<command-line>:0:0: note: this is the location of the previous definition
In file included from /tools/verilator/verilator-4.018/include/verilated.cpp:29:0:
/tools/verilator/verilator-4.018/include/verilated_imp.h: In static member function ‘static void VerilatedImp::userDump()’:
/tools/verilator/verilator-4.018/include/verilated_imp.h:279:54: error: ‘my_printf’ was not declared in this scope
              if (first) { VL_PRINTF_MT("  userDump:\n"); first=false; }

NOTE: I can work around this by overiding printf itself (and not using -DVL_PRINTF=my_printf) by adding my own version of printf as a wrapper for my new output function to the verilator input files, but I would rather use -DVL_PRINTF=xxxx if that was the intended approach.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Nov 15, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-11-15T08:39:11Z


Originally VL_PRINTF was intended to be overridden in a .mk file which overrode the default verilator.mk file. For this reason VL_PRINTF is also defined in verilated.mk, hence the GCC warning about multiple definitions of it. That overriding also edited the make rules to insert a special #include.

What you are doing is perfectly reasonable, and probably a better way.

Please try removing VL_PRINTF from verilated.mk.in, and reinstall verilator (or just hack verilated.mk yourself.)

Then you have the missing definition problem... For that I'd suggest we add to the top of verilatedos.h

#ifdef VL_VERILATED_INCLUDE
1. include VL_VERILATED_INCLUDE
#endif

then add -DVL_VERILATED_INCLUDE="foo"

If this all works out send a patch and I'll merge it upstream.

Thanks.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Nov 15, 2019


Original Redmine Comment
Author Name: Julien Margetts
Original Date: 2019-11-15T09:36:41Z


Thanks for the pointers, I will persevere with that aproach and try and get a patch out ...

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Dec 3, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-12-03T01:49:00Z


Did the redefinition etc work for you? if so, can you send back a patch of what worked out? Thanks.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Dec 3, 2019


Original Redmine Comment
Author Name: Julien Margetts
Original Date: 2019-12-03T12:05:57Z


Just got round to this, yes, the attached patch works when combined with adding the following to the Verilator command line:

my_printf.cpp --CFLAGS '-DVL_PRINTF=my_printf -DVL_VERILATED_INCLUDE="my_printf.h" -I..'

Thanks

PS: I included VL_VERILATED_INCLUDE in verilated.h rather than verilatedos.h since it seems a better fit (given it is intended only for verilated output, not the host side tool itself?)

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Dec 3, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-12-03T12:22:39Z


Yes, that looks like a fine place. Do you agree this (and future so don't need to ask again) contributions are open sourced as per the Developer Certificate of Origin (https://developercertificate.org/)?

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Dec 3, 2019


Original Redmine Comment
Author Name: Julien Margetts
Original Date: 2019-12-03T15:52:57Z


Yes, Agreed

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Dec 4, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-12-04T00:12:29Z


Thanks for your work. Pushed to git towards eventual 4.024 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.