-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
lib: remove pure attribute from functions that modify memory #8876
Conversation
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the "ext_pure" macro used anywhere else? maybe it should be removed entirely so that it doesn't tempt anyone else?
in theory having the ability to specify that a function doesn't modify memory is good right? I can see places where it would make sense. We just didn't apply it properly in this case? |
I guess that's the question: I think it's an attractive nuisance, and the micro-optimization isn't worth the time to figure out how to use it safely, or the time to figure out how to review its use. |
I agree with Mark here. This is such a micro-optimization that can be used improperly and lead to a disastrous bugs like this one. There are only two functions now that use the macro - |
Wow, great find. |
Continuous Integration Result: SUCCESSFULContinuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-19686/ This is a comment from an automated CI system. |
@donaldsharp, please agree with removing the macro completely, or merge this if you think we should keep the macro. |
go for it |
Almost all functions currently marked with pure attribute acquire a route_node lock. By marking them pure we allow compiler to optimize the code and not call them when it already knows the return value. This is completely incorrect. Only two of eleven functions can be marked as pure. And they still won't be optimized because they are never called from the same function twice. Let's remove the ext_pure macro completely to reduce the chance of repeating this mistake in the future. Fixes FRRouting#8866, FRRouting#8809, FRRouting#8595, FRRouting#6992. Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
Great find. Ouch. As the person who added |
FWIW the typesafe lists also use |
ci:rerun |
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopotests Ubuntu 18.04 i386 part 4: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO4U18I386-19694/test Topology Tests failed for Topotests Ubuntu 18.04 i386 part 4:
see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-19694/artifact/TOPO4U18I386/ErrorLog/log_topotests.txt Successful on other platforms/tests
|
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-19698/ This is a comment from an automated CI system. |
This should be very widely backported even to branches we no longer consider active; @idryzhov do you want me to do that or are you going to? |
@Mergifyio backport dev/8.0 stable/7.5 stable/7.4 |
@eqvinox you can just post a comment similar to the one above if you want to backport to older branches. |
Command
|
ah cool I hadn't used the bot yet. let's see what the oldest branch this affects is... (i would even backport to 6.0 if applicable, since that's what's in upstream Debian 10) |
@Mergifyio backport stable/7.3 stable/7.2 |
Command
|
(7.2 is the old release this affects.) |
lib: remove pure attribute from functions that modify memory (backport #8876)
lib: remove pure attribute from functions that modify memory (backport #8876)
lib: remove pure attribute from functions that modify memory (backport #8876)
lib: remove pure attribute from functions that modify memory (backport #8876)
lib: remove pure attribute from functions that modify memory (backport #8876)
All these functions acquire a route_node lock. Marking them as pure
means that the compiler can optimize the code to not call them multiple
times when it already knows the return value. This is insane.
Fixes #8866
Fixes #8809
Fixes #8595
Fixes #6992
Signed-off-by: Igor Ryzhov iryzhov@nfware.com