lib: remove pure attribute from functions that modify memory #8876

idryzhov · 2021-06-18T10:06:58Z

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

LabN-CI · 2021-06-18T10:36:23Z

Outdated results 💚

Basic BGPD CI results: SUCCESS, 0 tests failed

_	_
Result	SUCCESS git merge/8876 `c5e7038`
Date	06/18/2021
Start	06:10:47
Finish	06:36:19
Run-Time	25:32
Total	1815
Pass	1815
Fail	0
Valgrind-Errors
Valgrind-Loss
Details	vncregress-2021-06-18-06:10:47.txt
Log	autoscript-2021-06-18-06:12:00.log.bz2
Memory	511 515 429

For details, please contact louberger

mjstapp

Is the "ext_pure" macro used anywhere else? maybe it should be removed entirely so that it doesn't tempt anyone else?

donaldsharp · 2021-06-18T12:45:19Z

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?

mjstapp · 2021-06-18T12:54:15Z

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.

idryzhov · 2021-06-18T12:58:55Z

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 - route_table_count and route_table_prefix_iter_cmp. They are never called multiple times from the same function, so I don't think this optimization ever takes place.
I'll remove the macro completely if @donaldsharp agrees.

qlyoung · 2021-06-18T15:48:53Z

Wow, great find.

NetDEF-CI · 2021-06-18T16:12:36Z

Continuous Integration Result: SUCCESSFUL

Congratulations, 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.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

idryzhov · 2021-06-18T16:36:24Z

@donaldsharp, please agree with removing the macro completely, or merge this if you think we should keep the macro.

donaldsharp · 2021-06-18T16:39:51Z

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>

LabN-CI · 2021-06-18T17:26:20Z

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Results table

_	_
Result	SUCCESS git merge/8876 `4f08c71`
Date	06/18/2021
Start	13:00:46
Finish	13:26:16
Run-Time	25:30
Total	1815
Pass	1815
Fail	0
Valgrind-Errors
Valgrind-Loss
Details	vncregress-2021-06-18-13:00:46.txt
Log	autoscript-2021-06-18-13:02:02.log.bz2
Memory	486 492 430

For details, please contact louberger

eqvinox · 2021-06-18T18:49:57Z

Great find. Ouch. As the person who added ext_pure and introduced these bugs — sigh — I agree it should be removed, being too hard to use correctly. It's been sitting there b0rked for 2 years :(

eqvinox · 2021-06-18T19:11:48Z

FWIW the typesafe lists also use __attribute__((pure)), but that's only for _count, _first, _next and _next_safe - which indeed don't modify any memory. Double checked just to be safe. (And for these the attribute/optimization actually does make some sense.)

idryzhov · 2021-06-18T22:34:37Z

ci:rerun

NetDEF-CI · 2021-06-19T00:32:43Z

Continuous Integration Result: FAILED

See below for issues.
CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-19694/

This is a comment from an automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

Get source / Pull Request: Successful

Building Stage: Successful

Basic Tests: Failed

Topotests 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:

2021-06-18 21:30:01,143 ERROR: 'router_json_cmp' failed after 131.40 seconds
2021-06-18 21:30:01,144 ERROR: assert failed at "bgp_ipv6_rtadv.test_bgp_ipv6_rtadv/test_protocols_convergence": "r2" JSON output mismatches
assert Generated JSON diff error report:
  
  > $: d2 has key '10.254.254.1/32' which is not present in d1
2021-06-18 21:33:31,009 ERROR: Traceback (most recent call last):
  File "/root/bamboo-agent-home/xml-data/build-dir/FRR-FRRPULLREQ-TOPO4U18I386/topotests/lib/common_config.py", line 2530, in create_bgp_community_lists
    tgen, router, config_data, "bgp_community_list", build=build
  File "/root/bamboo-agent-home/xml-data/build-dir/FRR-FRRPULLREQ-TOPO4U18I386/topotests/lib/common_config.py", line 355, in create_common_configuration
    load_config_to_router(tgen, router)
  File "/root/bamboo-agent-home/xml-data/build-dir/FRR-FRRPULLREQ-TOPO4U18I386/topotests/lib/common_config.py", line 619, in load_config_to_router
    raise InvalidCLIError("%s" % output)
InvalidCLIError: % Malformed community-list value
line 2: Failure to communicate[13] to bgpd, line: bgp community-list standard ANY permit 0:-1 



2021-06-18 21:33:31,317 ERROR: Traceback (most recent call last):
  File "/root/bamboo-agent-home/xml-data/build-dir/FRR-FRRPULLREQ-TOPO4U18I386/topotests/lib/common_config.py", line 2530, in create_bgp_community_lists
    tgen, router, config_data, "bgp_community_list", build=build
  File "/root/bamboo-agent-home/xml-data/build-dir/FRR-FRRPULLREQ-TOPO4U18I386/topotests/lib/common_config.py", line 355, in create_common_configuration
    load_config_to_router(tgen, router)
  File "/root/bamboo-agent-home/xml-data/build-dir/FRR-FRRPULLREQ-TOPO4U18I386/topotests/lib/common_config.py", line 619, in load_config_to_router
    raise InvalidCLIError("%s" % output)
InvalidCLIError: % Malformed community-list value
line 2: Failure to communicate[13] to bgpd, line: bgp community-list standard ANY permit 0:65536 



2021-06-18 21:33:31,638 ERROR: Traceback (most recent call last):
  File "/root/bamboo-agent-home/xml-data/build-dir/FRR-FRRPULLREQ-TOPO4U18I386/topotests/lib/common_config.py", line 2530, in create_bgp_community_lists
    tgen, router, config_data, "bgp_community_list", build=build
  File "/root/bamboo-agent-home/xml-data/build-dir/FRR-FRRPULLREQ-TOPO4U18I386/topotests/lib/common_config.py", line 355, in create_common_configuration
    load_config_to_router(tgen, router)
  File "/root/bamboo-agent-home/xml-data/build-dir/FRR-FRRPULLREQ-TOPO4U18I386/topotests/lib/common_config.py", line 619, in load_config_to_router
    raise InvalidCLIError("%s" % output)
InvalidCLIError: % Malformed community-list value
line 2: Failure to communicate[13] to bgpd, line: bgp large-community-list standard ANY permit 0:4294967296 



2021-06-18 21:33:31,919 ERROR: Traceback (most recent call last):
  File "/root/bamboo-agent-home/xml-data/build-dir/FRR-FRRPULLREQ-TOPO4U18I386/topotests/lib/common_config.py", line 2530, in create_bgp_community_lists
    tgen, router, config_data, "bgp_community_list", build=build
  File "/root/bamboo-agent-home/xml-data/build-dir/FRR-FRRPULLREQ-TOPO4U18I386/topotests/lib/common_config.py", line 355, in create_common_configuration
    load_config_to_router(tgen, router)
  File "/root/bamboo-agent-home/xml-data/build-dir/FRR-FRRPULLREQ-TOPO4U18I386/topotests/lib/common_config.py", line 619, in load_config_to_router
    raise InvalidCLIError("%s" % output)
InvalidCLIError: % Malformed community-list value
line 2: Failure to communicate[13] to bgpd, line: bgp large-community-list standard ANY permit 0:-1:1 



2021-06-18 21:33:33,706 ERROR: Traceback (most recent call last):
  File "/root/bamboo-agent-home/xml-data/build-dir/FRR-FRRPULLREQ-TOPO4U18I386/topotests/lib/common_config.py", line 2530, in create_bgp_community_lists
    tgen, router, config_data, "bgp_community_list", build=build
  File "/root/bamboo-agent-home/xml-data/build-dir/FRR-FRRPULLREQ-TOPO4U18I386/topotests/lib/common_config.py", line 355, in create_common_configuration
    load_config_to_router(tgen, router)
  File "/root/bamboo-agent-home/xml-data/build-dir/FRR-FRRPULLREQ-TOPO4U18I386/topotests/lib/common_config.py", line 619, in load_config_to_router
    raise InvalidCLIError("%s" % output)
InvalidCLIError: % Malformed community-list value
line 2: Failure to communicate[13] to bgpd, line: bgp large-community-list standard ANY permit 1:a:2 



2021-06-18 21:36:45,813 ERROR: Traceback (most recent call last):
  File "/root/bamboo-agent-home/xml-data/build-dir/FRR-FRRPULLREQ-TOPO4U18I386/topotests/lib/common_config.py", line 2530, in create_bgp_community_lists
    tgen, router, config_data, "bgp_community_list", build=build
  File "/root/bamboo-agent-home/xml-data/build-dir/FRR-FRRPULLREQ-TOPO4U18I386/topotests/lib/common_config.py", line 355, in create_common_configuration
    load_config_to_router(tgen, router)
  File "/root/bamboo-agent-home/xml-data/build-dir/FRR-FRRPULLREQ-TOPO4U18I386/topotests/lib/common_config.py", line 619, in load_config_to_router
    raise InvalidCLIError("%s" % output)
InvalidCLIError: line 2: % Command incomplete[4]: bgp large-community-list standard Test1 permit

see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-19694/artifact/TOPO4U18I386/ErrorLog/log_topotests.txt

Successful on other platforms/tests

Addresssanitizer topotests part 4
Topotests Ubuntu 18.04 amd64 part 1
Addresssanitizer topotests part 2
Topotests Ubuntu 18.04 arm8 part 3
Topotests debian 10 amd64 part 6
CentOS 7 rpm pkg check
Topotests Ubuntu 18.04 amd64 part 8
Topotests Ubuntu 18.04 i386 part 2
Topotests debian 10 amd64 part 1
Debian 9 deb pkg check
Topotests Ubuntu 18.04 i386 part 7
Addresssanitizer topotests part 9
Topotests Ubuntu 18.04 arm8 part 7
Topotests Ubuntu 18.04 arm8 part 2
Topotests debian 10 amd64 part 5
Topotests Ubuntu 18.04 i386 part 8
Topotests debian 10 amd64 part 0
Topotests Ubuntu 18.04 i386 part 3
Topotests Ubuntu 18.04 i386 part 6
Topotests Ubuntu 18.04 i386 part 1
Topotests debian 10 amd64 part 2
Topotests Ubuntu 18.04 amd64 part 2
Topotests Ubuntu 18.04 arm8 part 8
Addresssanitizer topotests part 7
Topotests debian 10 amd64 part 4
Topotests Ubuntu 18.04 amd64 part 6
Topotests Ubuntu 18.04 arm8 part 0
Static analyzer (clang)
Topotests Ubuntu 18.04 arm8 part 5
Topotests debian 10 amd64 part 3
Ubuntu 18.04 deb pkg check
Addresssanitizer topotests part 5
Ubuntu 16.04 deb pkg check
Topotests Ubuntu 18.04 amd64 part 5
Topotests Ubuntu 18.04 i386 part 0
Topotests Ubuntu 18.04 i386 part 5
Addresssanitizer topotests part 3
Addresssanitizer topotests part 0
IPv4 ldp protocol on Ubuntu 18.04
Topotests Ubuntu 18.04 i386 part 9
Topotests Ubuntu 18.04 arm8 part 1
Topotests Ubuntu 18.04 amd64 part 3
Addresssanitizer topotests part 1
IPv6 protocols on Ubuntu 18.04
Topotests Ubuntu 18.04 amd64 part 0
Topotests Ubuntu 18.04 amd64 part 4
Topotests Ubuntu 18.04 arm8 part 9
Topotests Ubuntu 18.04 arm8 part 4
Addresssanitizer topotests part 6
Addresssanitizer topotests part 8
Topotests Ubuntu 18.04 amd64 part 7
Debian 10 deb pkg check
Topotests debian 10 amd64 part 7
Topotests Ubuntu 18.04 arm8 part 6
IPv4 protocols on Ubuntu 18.04
Topotests Ubuntu 18.04 amd64 part 9
Topotests debian 10 amd64 part 9
Fedora 29 rpm pkg check
Ubuntu 20.04 deb pkg check
Topotests debian 10 amd64 part 8

NetDEF-CI · 2021-06-19T04:42:41Z

Continuous Integration Result: SUCCESSFUL

Congratulations, 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.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

eqvinox · 2021-06-19T12:33:21Z

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?

idryzhov · 2021-06-19T12:38:13Z

@Mergifyio backport dev/8.0 stable/7.5 stable/7.4

idryzhov · 2021-06-19T12:39:25Z

@eqvinox you can just post a comment similar to the one above if you want to backport to older branches.

mergify · 2021-06-19T12:39:33Z

Command backport dev/8.0 stable/7.5 stable/7.4: success

Backports have been created

#8879 lib: remove pure attribute from functions that modify memory (backport #8876) has been created for branch dev/8.0

#8880 lib: remove pure attribute from functions that modify memory (backport #8876) has been created for branch stable/7.5

#8881 lib: remove pure attribute from functions that modify memory (backport #8876) has been created for branch stable/7.4

eqvinox · 2021-06-19T12:53:53Z

@eqvinox you can just post a comment similar to the one above if you want to backport to older branches.

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)

eqvinox · 2021-06-19T12:55:34Z

@Mergifyio backport stable/7.3 stable/7.2

mergify · 2021-06-19T12:56:40Z

Command backport stable/7.3 stable/7.2: success

Backports have been created

#8885 lib: remove pure attribute from functions that modify memory (backport #8876) has been created for branch stable/7.3

#8886 lib: remove pure attribute from functions that modify memory (backport #8876) has been created for branch stable/7.2

eqvinox · 2021-06-19T12:57:55Z

(7.2 is the old release this affects.)

lib: remove pure attribute from functions that modify memory (backport #8876)

polychaeta added the libfrr label Jun 18, 2021

mjstapp reviewed Jun 18, 2021

View reviewed changes

idryzhov added the backport label Jun 18, 2021

idryzhov force-pushed the remove-pure branch from c5e7038 to 4f08c71 Compare June 18, 2021 16:55

eqvinox merged commit ca4849b into FRRouting:master Jun 19, 2021

This was referenced Jun 19, 2021

lib: remove pure attribute from functions that modify memory (backport #8876) #8879

Merged

lib: remove pure attribute from functions that modify memory (backport #8876) #8880

Merged

mergify bot mentioned this pull request Jun 19, 2021

lib: remove pure attribute from functions that modify memory (backport #8876) #8881

Merged

mergify bot mentioned this pull request Jun 19, 2021

lib: remove pure attribute from functions that modify memory (backport #8876) #8885

Merged

mergify bot mentioned this pull request Jun 19, 2021

lib: remove pure attribute from functions that modify memory (backport #8876) #8886

Merged

donaldsharp added a commit that referenced this pull request Jun 19, 2021

Merge pull request #8880 from FRRouting/mergify/bp/stable/7.5/pr-8876

8f96d15

lib: remove pure attribute from functions that modify memory (backport #8876)

donaldsharp added a commit that referenced this pull request Jun 19, 2021

Merge pull request #8879 from FRRouting/mergify/bp/dev/8.0/pr-8876

102f028

lib: remove pure attribute from functions that modify memory (backport #8876)

donaldsharp added a commit that referenced this pull request Jun 19, 2021

Merge pull request #8886 from FRRouting/mergify/bp/stable/7.2/pr-8876

80d2eaa

lib: remove pure attribute from functions that modify memory (backport #8876)

donaldsharp added a commit that referenced this pull request Jun 19, 2021

Merge pull request #8885 from FRRouting/mergify/bp/stable/7.3/pr-8876

97935a2

lib: remove pure attribute from functions that modify memory (backport #8876)

donaldsharp added a commit that referenced this pull request Jun 19, 2021

Merge pull request #8881 from FRRouting/mergify/bp/stable/7.4/pr-8876

f0d4336

lib: remove pure attribute from functions that modify memory (backport #8876)

idryzhov mentioned this pull request Aug 23, 2021

"show ip ospf interface" crashes ospfd #9463

Closed

idryzhov mentioned this pull request Dec 2, 2021

ospfd in bullseye crashes on 'show ip ospf interface' #10162

Closed

idryzhov mentioned this pull request Dec 13, 2021

The show ip ospf interface command in VTY causes ospfd to crash. #10203

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: remove pure attribute from functions that modify memory #8876

lib: remove pure attribute from functions that modify memory #8876

idryzhov commented Jun 18, 2021 •

edited

Loading

LabN-CI commented Jun 18, 2021 •

edited

Loading

Basic BGPD CI results: SUCCESS, 0 tests failed

mjstapp left a comment

donaldsharp commented Jun 18, 2021

mjstapp commented Jun 18, 2021

idryzhov commented Jun 18, 2021

qlyoung commented Jun 18, 2021

NetDEF-CI commented Jun 18, 2021 •

edited

Loading

Continuous Integration Result: SUCCESSFUL

idryzhov commented Jun 18, 2021

donaldsharp commented Jun 18, 2021

LabN-CI commented Jun 18, 2021

eqvinox commented Jun 18, 2021

eqvinox commented Jun 18, 2021

idryzhov commented Jun 18, 2021

NetDEF-CI commented Jun 19, 2021 •

edited

Loading

Continuous Integration Result: FAILED

Get source / Pull Request: Successful

Building Stage: Successful

Basic Tests: Failed

NetDEF-CI commented Jun 19, 2021

eqvinox commented Jun 19, 2021

idryzhov commented Jun 19, 2021

idryzhov commented Jun 19, 2021

mergify bot commented Jun 19, 2021

eqvinox commented Jun 19, 2021

eqvinox commented Jun 19, 2021

mergify bot commented Jun 19, 2021

eqvinox commented Jun 19, 2021

lib: remove pure attribute from functions that modify memory #8876

lib: remove pure attribute from functions that modify memory #8876

Conversation

idryzhov commented Jun 18, 2021 • edited Loading

LabN-CI commented Jun 18, 2021 • edited Loading

Basic BGPD CI results: SUCCESS, 0 tests failed

mjstapp left a comment

Choose a reason for hiding this comment

donaldsharp commented Jun 18, 2021

mjstapp commented Jun 18, 2021

idryzhov commented Jun 18, 2021

qlyoung commented Jun 18, 2021

NetDEF-CI commented Jun 18, 2021 • edited Loading

Continuous Integration Result: SUCCESSFUL

idryzhov commented Jun 18, 2021

donaldsharp commented Jun 18, 2021

LabN-CI commented Jun 18, 2021

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

eqvinox commented Jun 18, 2021

eqvinox commented Jun 18, 2021

idryzhov commented Jun 18, 2021

NetDEF-CI commented Jun 19, 2021 • edited Loading

Continuous Integration Result: FAILED

Get source / Pull Request: Successful

Building Stage: Successful

Basic Tests: Failed

NetDEF-CI commented Jun 19, 2021

Continuous Integration Result: SUCCESSFUL

eqvinox commented Jun 19, 2021

idryzhov commented Jun 19, 2021

idryzhov commented Jun 19, 2021

mergify bot commented Jun 19, 2021

eqvinox commented Jun 19, 2021

eqvinox commented Jun 19, 2021

mergify bot commented Jun 19, 2021

eqvinox commented Jun 19, 2021

idryzhov commented Jun 18, 2021 •

edited

Loading

LabN-CI commented Jun 18, 2021 •

edited

Loading

NetDEF-CI commented Jun 18, 2021 •

edited

Loading

NetDEF-CI commented Jun 19, 2021 •

edited

Loading