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

snprintf() in verilator.h not supported in MSVC++ 2010 #927

Closed
veripoolbot opened this issue Jun 4, 2015 · 14 comments
Closed

snprintf() in verilator.h not supported in MSVC++ 2010 #927

veripoolbot opened this issue Jun 4, 2015 · 14 comments
Assignees
Labels
area: configure/compiling Issue involves configuring or compilating Verilator itself resolution: fixed Closed; fixed

Comments

@veripoolbot
Copy link
Contributor


Author Name: Hans Tichelaar
Original Redmine Issue: 927 from https://www.veripool.org
Original Date: 2015-06-04
Original Assignee: Wilson Snyder (@wsnyder)


Verilator 3.873 (git checkout HEAD yesterday) has file include/verilated_vpi.h with snprintf() calls in lines 923-926. The snprintf() is a C++11 feature and not supported in MSVC++ 2010 (and successors most likely although not checked by me). Workaround is given by URL http://stackoverflow.com/questions/2915672/snprintf-and-visual-studio-2010. Adding workaround at the beginning of include/verilated_vpi.h solved my issue temporarily (until next update of verilator).

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Hans Tichelaar
Original Date: 2015-06-04T07:09:10Z


File name in title should be verilated_vpi.h

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Hans Tichelaar
Original Date: 2015-06-04T07:16:25Z


Workaround added to verilated.h (and not verilated_vpi.h).

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2015-06-04T23:37:11Z


Fixed in git.

Please give the git version a try as I can't test it myself thanks.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2015-06-06T18:05:21Z


In 3.874.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Hans Tichelaar
Original Date: 2015-06-08T09:20:18Z


Unfortunately your fix didn't work and broke more. Now both verilated_vpi.h and verilatedos.h are impacted. For the sake of clarity please note that it is only about building a verilated project with MSVC++ 2010 (not building verilator itself - I wouldn't dare nor attempt with MSVC++). A fix like I proposed earlier in file verilated.h did help me to build my project (but I don't have the overview nor knowledge like you have to judge impact overall).

Note that your fix also impacted building verilator with MinGW since you used _WIN32 compiler flags only and not _MSC_VER. I could build release 3.872 verilator with MinGW by simply working around setenv() in file V3Os.cpp, which is not supported by MinGW. Will post separate on that one.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2015-06-08T11:17:08Z


Can you provide a patch that fixes it? Is it just using _MSC_VER?

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Hans Tichelaar
Original Date: 2015-06-08T19:37:20Z


A better patch than previous specified has been found in URL [[http://www.juce.com/forum/topic/solved-error-vs2015-regarding-snprintf]]. The file verilated_vpi.h is impacted only. Add following code snippet just after the #include section of file verilated_vpi.h:

#if defined(_WIN32) && defined(_MSC_VER)
// Visual Studio 2015 RC supports snprintf() whereas earlier versions do not
#if (_MSC_VER < 1900)
#define snprintf _snprintf
#endif // #if (_MSC_VER < 1900)
#endif // #if defined(_WIN32) && defined(_MSC_VER)

That's all. Note that the _WIN32 macro in the compiler switch is probably redundant but cannot hurt either.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2015-06-09T00:49:24Z


verilatedos.h is really the right point. Can you please edit the #ifdef and replace the function appropriately (to define VL_SNPRINTF) and test/attach the patch?

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Hans Tichelaar
Original Date: 2015-06-16T08:07:23Z


With all due respect but I don't see immediately the point in updating verilatedos.h except from reversing or adapting a previous fix you made with respect of this topic (macro VL_VSNPRINTF in verilatedos.h breaks building verilator with MinGW currently). In verilatedos.h the macro VL_VSNPRINTF is defined. However, this macro is not used in verilated_vpi.h at lines 923-926. There snprintf() is called directly without being redefined by macro as far as I can judge.

If verilatedos.h is indeed the right point then a new macro VL_SNPRINTF should be defined with a dependency on _WIN32 and _MSC_VER and subsequently used in verilated_vpi.h at lines 923-926. Also macro VL_VSNPRINTF must either be removed or redefined to prevent breaking build of verilator with MinGW. Both verilatedos.h and verilated_vpi.h files will be touched. Is that what you want?

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2015-06-16T11:14:54Z


Please give the trunk a try.

I got confused by the web reference in your original message and only thought vsnprintf was wrong. BTW it's important to put portability stuff in verilatedos.h otherwise future code is likely to misuse vsnprintf leading to later complaints. Likewise it's better to define VL_SNPRINTF than just 'snprintf' because when Microsoft finally fixes their silly limitation it won't override their snprintf.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Hans Tichelaar
Original Date: 2015-06-19T10:55:47Z


Saw meanwhile that you already updated the trunk. Thanks for that! What is missing is the removal of the original fix (it was all about snprintf and not vsnprintf!) and the _MSC_VER version check to prevent potential future PRs of MSVC++ 2015 RC users. The code snippet around VL_SNPRINTF macro in file verilatedos.h should be adjusted as follows:

#if defined(_WIN32) && defined(_MSC_VER)
1. if (_MSC_VER < 1900)
1.  define VL_SNPRINTF _snprintf
1. else
1.  define VL_SNPRINTF snprintf
1. endif
1. define VL_VSNPRINTF vsnprintf
#else
1. define VL_SNPRINTF snprintf
1. define VL_VSNPRINTF vsnprintf
#endif

Would appreciate if you could again make an update of the trunk. (I don't dear and like to make an update directly in the trunk with the risk to mess things up. Would normally use a branch for that, which merges into trunk if approved.).

After applying above change, I tested a build of verilator with MinGW, which went Ok. Verilating an example with the new (MinGW) verilator.exe and building the result in a MSVC++ 2010 environment (Amir Gonnen's approach and original contribution - see URL [[http://www.veripool.org/boards/1/topics/945-Verilator-Verilator-binaries-for-windows]]) went smoothless.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Hans Tichelaar
Original Date: 2015-06-21T18:22:10Z


Slip of the finger "went smooth" :-).

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2015-06-22T01:02:28Z


Patched as suggested, thanks.

Fixed in git towards 3.875.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2015-08-13T01:44:03Z


In 3.876.

@veripoolbot veripoolbot added area: configure/compiling Issue involves configuring or compilating Verilator itself resolution: fixed Closed; fixed labels Dec 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: configure/compiling Issue involves configuring or compilating Verilator itself resolution: fixed Closed; fixed
Projects
None yet
Development

No branches or pull requests

2 participants