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

v3errorEndFatal marked noreturn but GCC thinks it returns #1209

Closed
veripoolbot opened this issue Sep 13, 2017 · 6 comments
Closed

v3errorEndFatal marked noreturn but GCC thinks it returns #1209

veripoolbot opened this issue Sep 13, 2017 · 6 comments

Comments

@veripoolbot
Copy link

@veripoolbot veripoolbot commented Sep 13, 2017


Author Name: Mike Popoloski
Original Redmine Issue: 1209 from https://www.veripool.org

Original Assignee: Wilson Snyder (@wsnyder)


GCC is unhappy with the recently added v3errorEndFatal changes, which mark the function as attribute ((noreturn)). I fixed them up by adding __builtin_unreachable(); after the assert(0) in each case, but a more robust solution is probably required for compilers that don't support __builtin_unreachable (MSVC?)

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Sep 13, 2017


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2017-09-13T23:28:23Z


That's just there to appease the static tools. I think it might be unnecessary with the assert() now there, but I added it anyways.

BTW which GCC did you see a problem with? I test with both very old and the almost latest. Please give it a try as I didn't see the problem, and if it breaks send a patch, thanks.

Fixed in git towards 3.911.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Sep 14, 2017


Original Redmine Comment
Author Name: Mike Popoloski
Original Date: 2017-09-14T14:22:34Z


We build with GCC7 with a very strict set of warning checks. Here is the set in case you're curious:
-Wall -Werror -Wextra -Wshadow -Wduplicated-cond -Wlogical-op

When building Verilator I had to explicitly disable some warnings that probably shouldn't have to be disabled:
-Wno-nonnull-compare -Wno-implicit-fallthrough -Wno-shadow

And without this flag the optimizer will remove incorrect null checks in V3AstNodes and cause segfaults:
-fno-delete-null-pointer-checks

For our Clang build I additionally need:
-Wno-undefined-bool-conversion

I've pulled your changes and uploaded a patch that fixes the additional areas that warn. Even though I use -Wno-shadow for builds of Verilator itself, building simulation binaries uses our full set of warnings so any that occur in verilated.h / verilated.cpp will also be caught. There's a shadowing warning that I fixed there and also included in the patch.

Aside: I made this patch with git format-patch. Let me know if it works better than the git diff produced ones I was submitting before.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Sep 15, 2017


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2017-09-15T03:28:08Z


You should "setenv VERILATOR_AUTHOR_SITE 1" in your .cshrc/.bashrc so in the future when you configure warnings will be on by default. This will turn off warnings that are not clean and generally cannot be fixed without a lot of effort, but you're welcome to make patches to beat them down!

I had only rarely been doing stronger than -Wall, but I added -Wextra.

I ended up turning off -Wshadow in the default makefile for generated code, because sometimes the user has global values which are shadowed, and can't be prevented, but it is clean for the moment. There's also a bug in my version of GCC:

if (int foo=...)
else if (int foo=...)

this is not a shadow as they are parallel scopes. Looks like GCC later fixed that but I don't want to make the code less readable with different names.

There was a single fallthrough related to the mess with NORETURN. I fixed that error but it'll probably cause later cppcheck warnings as it seems impossible to keep every tool happy as each deals with these assertion cases slightly differently.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Sep 15, 2017


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2017-09-15T03:30:14Z


Oh also -fno-delete-null-pointer-checks is a known problem discussed in another bug. To fix it I'd need to make 10K+ lines of code less readable which for now I won't do; this isn't needed on Verilated code however, only verilator itself.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Sep 15, 2017


Original Redmine Comment
Author Name: Mike Popoloski
Original Date: 2017-09-15T15:07:12Z


Yeah, I build Verilator within our own build system tree here, which is probably unusual. I'm happy with the current state of things, just felt I should upload any source modifications for others to benefit from as well.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Sep 23, 2017


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2017-09-23T14:15:00Z


In 3.912.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.