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.

Sign up for GitHub

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

Jump to bottom

Fix event buffer size boundary check #25234

Merged
merged 2 commits into from
Feb 22, 2023
Merged

Fix event buffer size boundary check #25234

merged 2 commits into from
Feb 22, 2023

Conversation

yunhanw-google
Copy link
Contributor

@yunhanw-google yunhanw-google commented Feb 22, 2023 •
edited
Loading

In TLVCircularBuffer::GetNewBuffer, it would evict head when mQueueLength == mQueueSize, EventManagement would use this TLVCircularBuffer to store events, once our written data size is equal to this queue size, it would evict head in TLVCircularBuffer, in order to resolve this issue, we need override OnInit on CircularEventBuffer and not evict head in TLVCircularBuffer. We do the below things

  1. Implement an OnInit override on CircularEventBuffer.
  2. Implement a protected function on TLVCircularBuffer that non-destructively returns the actual state of what our current available buffer space is. That is, basically everything from TLVCircularBuffer::GetNewBuffer after the block where we evict the head.
  3. Call that function from both TLVCircularBuffer::GetNewBuffer and our OnInit override.

@github-actions github-actions bot added the app label Feb 22, 2023
@pullapprove pullapprove bot requested a review from yufengwangca February 22, 2023 00:31
@pullapprove pullapprove bot added the app label Feb 22, 2023
@yunhanw-google yunhanw-google force-pushed the bug/fix_event_buffer_size branch from 0893638 to d75b823 Compare February 22, 2023 00:33
@github-actions
Copy link

PR #25234: Size comparison from 30188f5 to d75b823

Increases (2 builds for cc32xx, qpg)
platform target config section 30188f5 d75b823 change % change
cc32xx lock CC3235SF_LAUNCHXL .debug_info 20254059 20254061 2 0.0
qpg lock-app qpg6105+debug (read/write) 1118348 1118356 8 0.0
.text 565448 565456 8 0.0
Full report (4 builds for cc32xx, mbed, qpg)
platform target config section 30188f5 d75b823 change % change
cc32xx lock CC3235SF_LAUNCHXL 0 0 0 0.0
(read only) 642545 642545 0 0.0
(read/write) 203664 203664 0 0.0
.ARM.attributes 44 44 0 0.0
.ARM.exidx 8 8 0 0.0
.bss 197064 197064 0 0.0
.comment 194 194 0 0.0
.data 1480 1480 0 0.0
.debug_abbrev 930168 930168 0 0.0
.debug_aranges 87272 87272 0 0.0
.debug_frame 299752 299752 0 0.0
.debug_info 20254059 20254061 2 0.0
.debug_line 2655143 2655143 0 0.0
.debug_loc 2795383 2795383 0 0.0
.debug_ranges 281592 281592 0 0.0
.debug_str 3017876 3017876 0 0.0
.ramVecs 780 780 0 0.0
.resetVecs 64 64 0 0.0
.rodata 105817 105817 0 0.0
.shstrtab 232 232 0 0.0
.stab 204 204 0 0.0
.stabstr 441 441 0 0.0
.stack 2048 2048 0 0.0
.strtab 377610 377610 0 0.0
.symtab 256144 256144 0 0.0
.text 534604 534604 0 0.0
mbed lock-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2466784 2466784 0 0.0
.bss 215780 215780 0 0.0
.data 5880 5880 0 0.0
.text 1429428 1429428 0 0.0
qpg lighting-app qpg6105+debug (read/write) 1151356 1151356 0 0.0
.bss 99796 99796 0 0.0
.data 852 852 0 0.0
.text 598452 598452 0 0.0
lock-app qpg6105+debug (read/write) 1118348 1118356 8 0.0
.bss 96284 96284 0 0.0
.data 864 864 0 0.0
.text 565448 565456 8 0.0

@github-actions github-actions bot added the tests label Feb 22, 2023
@github-actions
Copy link

github-actions bot commented Feb 22, 2023 •
edited
Loading

PR #25234: Size comparison from 30188f5 to 641a0c2

Increases (10 builds for bl602, bl702, cc32xx, esp32, nrfconnect, psoc6, qpg, telink)
platform target config section 30188f5 641a0c2 change % change
bl602 lighting-app bl602 (read/write) 1349546 1349554 8 0.0
.text 1025616 1025620 4 0.0
bl702 lighting-app bl702 .debug_info 40594421 40594422 1 0.0
.text 954966 954968 2 0.0
cc32xx lock CC3235SF_LAUNCHXL .debug_info 20254059 20254060 1 0.0
esp32 all-clusters-app m5stack (read/write) 501607 501611 4 0.0
.flash.rodata 250964 250968 4 0.0
nrfconnect all-clusters-minimal-app nrf52840dk_nrf52840 (read/write) 1110092 1110108 16 0.0
text 775984 775988 4 0.0
psoc6 all-clusters cy8ckit_062s2_43012 .debug_info 27970741 27970743 2 0.0
lock cy8ckit_062s2_43012 .debug_info 23109057 23109058 1 0.0
qpg lock-app qpg6105+debug (read/write) 1118348 1118356 8 0.0
.text 565448 565456 8 0.0
telink all-clusters-app tlsr9518adk80d text 687146 687150 4 0.0
all-clusters-minimal-app tlsr9518adk80d (read/write) 951868 951876 8 0.0
text 648548 648550 2 0.0
Decreases (8 builds for bl702, cc13x2_26x2, esp32, psoc6, telink)
platform target config section 30188f5 641a0c2 change % change
bl702 lighting-app bl702+rpc .debug_info 45006708 45006707 -1 -0.0
.text 1032378 1032376 -2 -0.0
cc13x2_26x2 all-clusters-minimal-app LP_CC2652R7 (read only) 643495 643487 -8 -0.0
.text 564824 564816 -8 -0.0
esp32 all-clusters-app c3devkit (read/write) 1585562 1585554 -8 -0.0
.flash.rodata 222240 222232 -8 -0.0
psoc6 all-clusters-minimal cy8ckit_062s2_43012 .debug_info 27515341 27515340 -1 -0.0
light cy8ckit_062s2_43012 .debug_info 22857641 22857640 -1 -0.0
telink lighting-app tlsr9518adk80d (read/write) 950432 950424 -8 -0.0
text 659366 659364 -2 -0.0
ota-requestor-app tlsr9518adk80d (read/write) 883308 883300 -8 -0.0
text 602834 602832 -2 -0.0
thermostat tlsr9518adk80d (read/write) 875776 875768 -8 -0.0
text 594344 594340 -4 -0.0
Full report (43 builds for bl602, bl702, cc13x2_26x2, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
platform target config section 30188f5 641a0c2 change % change
bl602 lighting-app bl602 (read/write) 1349546 1349554 8 0.0
.bss 94666 94666 0 0.0
.data 9752 9752 0 0.0
.text 1025616 1025620 4 0.0
bl602+rpc (read/write) 1394986 1394986 0 0.0
.bss 102714 102714 0 0.0
.data 10144 10144 0 0.0
.text 1056552 1056552 0 0.0
bl702 lighting-app bl702 (read only) 3358 3358 0 0.0
(read/write) 1187907 1187907 0 0.0
.bleromro 6342 6342 0 0.0
.bleromrw 124 124 0 0.0
.boot2 292 292 0 0.0
.bss 69777 69777 0 0.0
.bss_psram 30064 30064 0 0.0
.comment 48 48 0 0.0
.data 4080 4080 0 0.0
.debug_abbrev 1551725 1551725 0 0.0
.debug_aranges 134232 134232 0 0.0
.debug_frame 492104 492104 0 0.0
.debug_info 40594421 40594422 1 0.0
.debug_line 5277244 5277244 0 0.0
.debug_loc 3415747 3415747 0 0.0
.debug_ranges 371920 371920 0 0.0
.debug_str 3574317 3574317 0 0.0
.hbn 536 536 0 0.0
.hbn_noinit 260 260 0 0.0
.init 342 342 0 0.0
.init_array 144 144 0 0.0
.psram 0 0 0 0.0
.riscv.attributes 47 47 0 0.0
.rodata 107904 107904 0 0.0
.rsvd 2960 2960 0 0.0
.sha_ocram 72 72 0 0.0
.shstrtab 304 304 0 0.0
.stack 2048 2048 0 0.0
.strtab 573930 573930 0 0.0
.symtab 173568 173568 0 0.0
.tcm_data 36 36 0 0.0
.tcmcode 3358 3358 0 0.0
.text 0 0 0 0.0
954966 954968 2 0.0
bl702+rpc (read only) 3358 3358 0 0.0
(read/write) 1281039 1281039 0 0.0
.bleromro 6342 6342 0 0.0
.bleromrw 124 124 0 0.0
.boot2 292 292 0 0.0
.bss 77825 77825 0 0.0
.bss_psram 30320 30320 0 0.0
.comment 48 48 0 0.0
.data 4624 4624 0 0.0
.debug_abbrev 1699956 1699956 0 0.0
.debug_aranges 142472 142472 0 0.0
.debug_frame 519868 519868 0 0.0
.debug_info 45006708 45006707 -1 -0.0
.debug_line 5676178 5676178 0 0.0
.debug_loc 3612745 3612745 0 0.0
.debug_ranges 395696 395696 0 0.0
.debug_str 3977854 3977854 0 0.0
.hbn 536 536 0 0.0
.hbn_noinit 260 260 0 0.0
.init 342 342 0 0.0
.init_array 160 160 0 0.0
.psram 0 0 0 0.0
.riscv.attributes 47 47 0 0.0
.rodata 122544 122544 0 0.0
.rsvd 2960 2960 0 0.0
.sha_ocram 72 72 0 0.0
.shstrtab 304 304 0 0.0
.stack 2048 2048 0 0.0
.strtab 634994 634994 0 0.0
.symtab 192032 192032 0 0.0
.tcm_data 36 36 0 0.0
.tcmcode 3358 3358 0 0.0
.text 0 0 0 0.0
1032378 1032376 -2 -0.0
cc13x2_26x2 all-clusters-app LP_CC2652R7 (read only) 680287 680287 0 0.0
(read/write) 170776 170776 0 0.0
.bss 80756 80756 0 0.0
.data 3352 3352 0 0.0
.rodata 88487 88487 0 0.0
.text 591484 591484 0 0.0
all-clusters-minimal-app LP_CC2652R7 (read only) 643495 643487 -8 -0.0
(read/write) 157416 157416 0 0.0
.bss 79948 79948 0 0.0
.data 3352 3352 0 0.0
.rodata 78351 78351 0 0.0
.text 564824 564816 -8 -0.0
lock-ftd LP_CC2652R7 (read only) 676471 676471 0 0.0
(read/write) 171960 171960 0 0.0
.bss 78212 78212 0 0.0
.data 3316 3316 0 0.0
.rodata 76679 76679 0 0.0
.text 599312 599312 0 0.0
lock-mtd LP_CC2652R7 (read only) 663107 663107 0 0.0
(read/write) 180580 180580 0 0.0
.bss 73468 73468 0 0.0
.data 3316 3316 0 0.0
.rodata 103411 103411 0 0.0
.text 559216 559216 0 0.0
pump-app LP_CC2652R7 (read only) 689699 689699 0 0.0
(read/write) 159468 159468 0 0.0
.bss 78180 78180 0 0.0
.data 3280 3280 0 0.0
.rodata 91099 91099 0 0.0
.text 598120 598120 0 0.0
pump-controller-app LP_CC2652R7 (read only) 674643 674643 0 0.0
(read/write) 174668 174668 0 0.0
.bss 78324 78324 0 0.0
.data 3304 3304 0 0.0
.rodata 86971 86971 0 0.0
.text 587192 587192 0 0.0
shell LP_CC2652R7 (read only) 671502 671502 0 0.0
(read/write) 181632 181632 0 0.0
.bss 82828 82828 0 0.0
.data 3348 3348 0 0.0
.rodata 85230 85230 0 0.0
.text 585960 585960 0 0.0
cc32xx lock CC3235SF_LAUNCHXL (read only) 642545 642545 0 0.0
(read/write) 203664 203664 0 0.0
.ARM.attributes 44 44 0 0.0
.ARM.exidx 8 8 0 0.0
.bss 197064 197064 0 0.0
.comment 194 194 0 0.0
.data 1480 1480 0 0.0
.debug_abbrev 930168 930168 0 0.0
.debug_aranges 87272 87272 0 0.0
.debug_frame 299752 299752 0 0.0
.debug_info 20254059 20254060 1 0.0
.debug_line 2655143 2655143 0 0.0
.debug_loc 2795383 2795383 0 0.0
.debug_ranges 281592 281592 0 0.0
.debug_str 3017876 3017876 0 0.0
.ramVecs 780 780 0 0.0
.resetVecs 64 64 0 0.0
.rodata 105817 105817 0 0.0
.shstrtab 232 232 0 0.0
.stab 204 204 0 0.0
.stabstr 441 441 0 0.0
.stack 2048 2048 0 0.0
.strtab 377610 377610 0 0.0
.symtab 256144 256144 0 0.0
.text 0 0 0 0.0
534604 534604 0 0.0
cyw30739 light cyw930739m2evb_01 (read/write) 586130 586130 0 0.0
.app_xip_area 463028 463028 0 0.0
.bss 65544 65544 0 0.0
.data 740 740 0 0.0
.rodata 0 0 0 0.0
.text 112 112 0 0.0
lock cyw930739m2evb_01 (read/write) 590066 590066 0 0.0
.app_xip_area 461668 461668 0 0.0
.bss 70832 70832 0 0.0
.data 748 748 0 0.0
.rodata 0 0 0 0.0
.text 112 112 0 0.0
ota-requestor-no-progress-logging cyw930739m2evb_01 (read/write) 551102 551102 0 0.0
.app_xip_area 433496 433496 0 0.0
.bss 60088 60088 0 0.0
.data 696 696 0 0.0
.rodata 0 0 0 0.0
.text 112 112 0 0.0
efr32 lighting-app BRD4161A+rs9116 (read/write) 1041360 1041360 0 0.0
.bss 181476 181476 0 0.0
.data 2048 2048 0 0.0
.text 857816 857816 0 0.0
BRD4187C (read/write) 1025704 1025704 0 0.0
.bss 150384 150384 0 0.0
.data 2684 2684 0 0.0
.text 848040 848040 0 0.0
lock-app BRD4161A+wf200 (read/write) 1068368 1068368 0 0.0
.bss 153012 153012 0 0.0
.data 2056 2056 0 0.0
.text 913280 913280 0 0.0
window-app BRD4187C (read/write) 1141112 1141112 0 0.0
.bss 134808 134808 0 0.0
.data 2576 2576 0 0.0
.text 979132 979132 0 0.0
esp32 all-clusters-app c3devkit (read only) 1050238 1050238 0 0.0
(read/write) 1585562 1585554 -8 -0.0
.dram0.bss 77800 77800 0 0.0
.dram0.data 13752 13752 0 0.0
.flash.rodata 222240 222232 -8 -0.0
.flash.text 1050238 1050238 0 0.0
.iram0.text 72896 72896 0 0.0
m5stack (read only) 1101691 1101691 0 0.0
(read/write) 501607 501611 4 0.0
.dram0.bss 82832 82832 0 0.0
.dram0.data 34040 34040 0 0.0
.flash.rodata 250964 250968 4 0.0
.flash.text 1096307 1096307 0 0.0
.iram0.text 124855 124855 0 0.0
k32w contact k32w0+release (read/write) 669928 669928 0 0.0
.bss 77548 77548 0 0.0
.data 2204 2204 0 0.0
.text 571064 571064 0 0.0
light k32w0+release (read/write) 668996 668996 0 0.0
.bss 77228 77228 0 0.0
.data 2192 2192 0 0.0
.text 570464 570464 0 0.0
lock k32w0+release (read/write) 625640 625640 0 0.0
.bss 75356 75356 0 0.0
.data 2136 2136 0 0.0
.text 545420 545420 0 0.0
linux chip-tool-ipv6only arm64 (read only) 12061444 12061444 0 0.0
(read/write) 729224 729224 0 0.0
.bss 34136 34136 0 0.0
.data 3008 3008 0 0.0
.data.rel.ro 671552 671552 0 0.0
.dynamic 560 560 0 0.0
.got 15328 15328 0 0.0
.init 24 24 0 0.0
.init_array 208 208 0 0.0
.rodata 585876 585876 0 0.0
.text 9745012 9745012 0 0.0
thermostat-no-ble arm64 (read only) 2516492 2516492 0 0.0
(read/write) 145096 145096 0 0.0
.bss 56312 56312 0 0.0
.data 1832 1832 0 0.0
.data.rel.ro 77568 77568 0 0.0
.dynamic 560 560 0 0.0
.got 5336 5336 0 0.0
.init 24 24 0 0.0
.init_array 432 432 0 0.0
.rodata 151320 151320 0 0.0
.text 2103024 2103024 0 0.0
mbed lock-app CY8CPROTO_062_4343W+release (read only) 6224 6224 0 0.0
(read/write) 2466784 2466784 0 0.0
.bss 215780 215780 0 0.0
.data 5880 5880 0 0.0
.text 1429428 1429428 0 0.0
nrfconnect all-clusters-app nrf52840dk_nrf52840 (read only) 4 4 0 0.0
(read/write) 1164856 1164856 0 0.0
bss 143411 143411 0 0.0
rodata 134852 134852 0 0.0
text 806744 806744 0 0.0
nrf7002dk_nrf5340_cpuapp (read only) 4 4 0 0.0
(read/write) 1370260 1370260 0 0.0
bss 105874 105874 0 0.0
rodata 212288 212288 0 0.0
text 766580 766580 0 0.0
all-clusters-minimal-app nrf52840dk_nrf52840 (read only) 4 4 0 0.0
(read/write) 1110092 1110108 16 0.0
bss 142567 142567 0 0.0
rodata 111816 111816 0 0.0
text 775984 775988 4 0.0
psoc6 all-clusters cy8ckit_062s2_43012 (read only) 841024 841024 0 0.0
(read/write) 1761028 1761028 0 0.0
.ARM.attributes 46 46 0 0.0
.ARM.exidx 8 8 0 0.0
.bss 189680 189680 0 0.0
.comment 200 200 0 0.0
.copy.table 24 24 0 0.0
.cy_m0p_image 6216 6216 0 0.0
.cy_sharedmem 8 8 0 0.0
.data 2640 2640 0 0.0
.debug_abbrev 1253562 1253562 0 0.0
.debug_aranges 111384 111384 0 0.0
.debug_frame 374332 374332 0 0.0
.debug_info 27970741 27970743 2 0.0
.debug_line 3790151 3790151 0 0.0
.debug_loc 3691419 3691419 0 0.0
.debug_ranges 363400 363400 0 0.0
.debug_str 3527042 3527042 0 0.0
.heap 841024 841024 0 0.0
.noinit 148 148 0 0.0
.ramVectors 736 736 0 0.0
.shstrtab 288 288 0 0.0
.stab 156 156 0 0.0
.stabstr 335 335 0 0.0
.stack_dummy 4096 4096 0 0.0
.strtab 580564 580564 0 0.0
.symtab 425488 425488 0 0.0
.text 1560320 1560320 0 0.0
.zero.table 8 8 0 0.0
text 0 0 0 0.0
all-clusters-minimal cy8ckit_062s2_43012 (read only) 841840 841840 0 0.0
(read/write) 1701388 1701388 0 0.0
.ARM.attributes 46 46 0 0.0
.ARM.exidx 8 8 0 0.0
.bss 188872 188872 0 0.0
.comment 200 200 0 0.0
.copy.table 24 24 0 0.0
.cy_m0p_image 6216 6216 0 0.0
.cy_sharedmem 8 8 0 0.0
.data 2632 2632 0 0.0
.debug_abbrev 1238895 1238895 0 0.0
.debug_aranges 110616 110616 0 0.0
.debug_frame 376668 376668 0 0.0
.debug_info 27515341 27515340 -1 -0.0
.debug_line 3797650 3797650 0 0.0
.debug_loc 3674621 3674621 0 0.0
.debug_ranges 361336 361336 0 0.0
.debug_str 3514164 3514164 0 0.0
.heap 841840 841840 0 0.0
.noinit 148 148 0 0.0
.ramVectors 736 736 0 0.0
.shstrtab 288 288 0 0.0
.stab 156 156 0 0.0
.stabstr 335 335 0 0.0
.stack_dummy 4096 4096 0 0.0
.strtab 540273 540273 0 0.0
.symtab 410400 410400 0 0.0
.text 1501496 1501496 0 0.0
.zero.table 0 0 0 0.0
8 8 0 0.0
light cy8ckit_062s2_43012 (read only) 850096 850096 0 0.0
(read/write) 1615100 1615100 0 0.0
.ARM.attributes 46 46 0 0.0
.ARM.exidx 8 8 0 0.0
.bss 180784 180784 0 0.0
.comment 200 200 0 0.0
.copy.table 24 24 0 0.0
.cy_m0p_image 6216 6216 0 0.0
.cy_sharedmem 8 8 0 0.0
.data 2464 2464 0 0.0
.debug_abbrev 1074237 1074237 0 0.0
.debug_aranges 102824 102824 0 0.0
.debug_frame 346960 346960 0 0.0
.debug_info 22857641 22857640 -1 -0.0
.debug_line 3350586 3350586 0 0.0
.debug_loc 3350195 3350195 0 0.0
.debug_ranges 319856 319856 0 0.0
.debug_str 3312910 3312910 0 0.0
.heap 850096 850096 0 0.0
.noinit 148 148 0 0.0
.ramVectors 736 736 0 0.0
.shstrtab 288 288 0 0.0
.stab 156 156 0 0.0
.stabstr 335 335 0 0.0
.stack_dummy 4096 4096 0 0.0
.strtab 475990 475990 0 0.0
.symtab 378384 378384 0 0.0
.text 1423464 1423464 0 0.0
.zero.table 0 0 0 0.0
8 8 0 0.0
lock cy8ckit_062s2_43012 (read only) 845120 845120 0 0.0
(read/write) 1648788 1648788 0 0.0
.ARM.attributes 46 46 0 0.0
.ARM.exidx 8 8 0 0.0
.bss 185744 185744 0 0.0
.comment 200 200 0 0.0
.copy.table 24 24 0 0.0
.cy_m0p_image 6216 6216 0 0.0
.cy_sharedmem 8 8 0 0.0
.data 2480 2480 0 0.0
.debug_abbrev 1076165 1076165 0 0.0
.debug_aranges 103232 103232 0 0.0
.debug_frame 348740 348740 0 0.0
.debug_info 23109057 23109058 1 0.0
.debug_line 3352734 3352734 0 0.0
.debug_loc 3374594 3374594 0 0.0
.debug_ranges 322416 322416 0 0.0
.debug_str 3331847 3331847 0 0.0
.heap 845120 845120 0 0.0
.noinit 148 148 0 0.0
.ramVectors 736 736 0 0.0
.shstrtab 288 288 0 0.0
.stab 156 156 0 0.0
.stabstr 335 335 0 0.0
.stack_dummy 4096 4096 0 0.0
.strtab 479826 479826 0 0.0
.symtab 380592 380592 0 0.0
.text 1452176 1452176 0 0.0
.zero.table 0 0 0 0.0
8 8 0 0.0
qpg lighting-app qpg6105+debug (read/write) 1151356 1151356 0 0.0
.bss 99796 99796 0 0.0
.data 852 852 0 0.0
.text 598452 598452 0 0.0
lock-app qpg6105+debug (read/write) 1118348 1118356 8 0.0
.bss 96284 96284 0 0.0
.data 864 864 0 0.0
.text 565448 565456 8 0.0
telink all-clusters-app tlsr9518adk80d (read only) 4 4 0 0.0
(read/write) 1016400 1016400 0 0.0
bss 97748 97748 0 0.0
text 687146 687150 4 0.0
all-clusters-minimal-app tlsr9518adk80d (read only) 4 4 0 0.0
(read/write) 951868 951876 8 0.0
bss 96800 96800 0 0.0
text 648548 648550 2 0.0
contact-sensor-app tlsr9518adk80d (read only) 4 4 0 0.0
(read/write) 856024 856024 0 0.0
bss 88968 88968 0 0.0
text 578926 578926 0 0.0
light-switch-app tlsr9518adk80d (read only) 4 4 0 0.0
(read/write) 871736 871736 0 0.0
bss 89052 89052 0 0.0
text 592932 592932 0 0.0
lighting-app tlsr9518adk80d (read only) 4 4 0 0.0
(read/write) 950432 950424 -8 -0.0
bss 97176 97176 0 0.0
text 659366 659364 -2 -0.0
ota-requestor-app tlsr9518adk80d (read only) 4 4 0 0.0
(read/write) 883308 883300 -8 -0.0
bss 89980 89980 0 0.0
text 602834 602832 -2 -0.0
thermostat tlsr9518adk80d (read only) 4 4 0 0.0
(read/write) 875776 875768 -8 -0.0
bss 90444 90444 0 0.0
text 594344 594340 -4 -0.0

@bzbarsky-apple
Copy link
Contributor

once our written data size is equal to this queue size, it would evict head in TLVCircularBuffer

In particular, this sounds like someone is calling GetNewBuffer at a point when we just finished writing and the buffer is full. Who is doing that and why? That seems to be the real bug: if we finished writing, why do we need to GetNewBuffer?

@bzbarsky-apple
Copy link
Contributor

bzbarsky-apple commented Feb 22, 2023 •
edited
Loading

OK, did some debugging and the actual issue is that TLVCircularBuffer::OnInit (the version taking TLVWriter) is destructive: if there is no space left in the buffer it will evict as part of init. This is not a completely insane assumption for it to make, but it's the wrong assumption for CircularEventBuffer.

What we should imo do is:

  1. Implement an OnInit override on CircularEventBuffer.
  2. Implement a (protected? public?) function on TLVCircularBuffer that non-destructively returns the actual state of what our current available buffer space is. That is, basically everything from TLVCircularBuffer::GetNewBuffer after the block where we evict the head.
  3. Call that function from both TLVCircularBuffer::GetNewBuffer and our OnInit override.

@yunhanw-google yunhanw-google force-pushed the bug/fix_event_buffer_size branch from 641a0c2 to e110bcf Compare February 22, 2023 19:04
bzbarsky-apple
bzbarsky-apple reviewed Feb 22, 2023
View reviewed changes
src/lib/core/TLVCircularBuffer.cpp Outdated Show resolved Hide resolved
src/lib/core/TLVCircularBuffer.cpp Outdated Show resolved Hide resolved
src/lib/core/TLVCircularBuffer.cpp Outdated Show resolved Hide resolved
src/lib/core/TLVCircularBuffer.cpp Outdated Show resolved Hide resolved
src/app/tests/TestEventLogging.cpp Show resolved Hide resolved
src/app/EventManagement.cpp Outdated Show resolved Hide resolved
@github-actions
Copy link

PR #25234: Size comparison from c6cadc8 to e110bcf

Increases (1 build for cc32xx)
platform target config section c6cadc8 e110bcf change % change
cc32xx lock CC3235SF_LAUNCHXL (read only) 642545 642569 24 0.0
.debug_frame 299752 299768 16 0.0
.debug_loc 2795342 2795460 118 0.0
.debug_str 3017877 3018032 155 0.0
.strtab 377630 377768 138 0.0
.symtab 256192 256256 64 0.0
.text 534604 534628 24 0.0
Decreases (1 build for cc32xx)
platform target config section c6cadc8 e110bcf change % change
cc32xx lock CC3235SF_LAUNCHXL .debug_abbrev 930168 930122 -46 -0.0
.debug_info 20254045 20249764 -4281 -0.0
.debug_line 2655138 2655113 -25 -0.0
Full report (1 build for cc32xx)
platform target config section c6cadc8 e110bcf change % change
cc32xx lock CC3235SF_LAUNCHXL 0 0 0 0.0
(read only) 642545 642569 24 0.0
(read/write) 203664 203664 0 0.0
.ARM.attributes 44 44 0 0.0
.ARM.exidx 8 8 0 0.0
.bss 197064 197064 0 0.0
.comment 194 194 0 0.0
.data 1480 1480 0 0.0
.debug_abbrev 930168 930122 -46 -0.0
.debug_aranges 87272 87272 0 0.0
.debug_frame 299752 299768 16 0.0
.debug_info 20254045 20249764 -4281 -0.0
.debug_line 2655138 2655113 -25 -0.0
.debug_loc 2795342 2795460 118 0.0
.debug_ranges 281592 281592 0 0.0
.debug_str 3017877 3018032 155 0.0
.ramVecs 780 780 0 0.0
.resetVecs 64 64 0 0.0
.rodata 105817 105817 0 0.0
.shstrtab 232 232 0 0.0
.stab 204 204 0 0.0
.stabstr 441 441 0 0.0
.stack 2048 2048 0 0.0
.strtab 377630 377768 138 0.0
.symtab 256192 256256 64 0.0
.text 534604 534628 24 0.0

@yunhanw-google yunhanw-google mentioned this pull request Feb 22, 2023
Open
bzbarsky-apple
bzbarsky-apple reviewed Feb 22, 2023
View reviewed changes
src/app/EventManagement.cpp Outdated Show resolved Hide resolved
src/app/EventManagement.cpp Outdated Show resolved Hide resolved
src/app/tests/TestEventLogging.cpp Outdated Show resolved Hide resolved
@yunhanw-google yunhanw-google force-pushed the bug/fix_event_buffer_size branch from 99e6038 to cc84c2e Compare February 22, 2023 21:33
@github-actions
Copy link

PR #25234: Size comparison from c6cadc8 to cc84c2e

Increases (1 build for cc32xx)
platform target config section c6cadc8 cc84c2e change % change
cc32xx lock CC3235SF_LAUNCHXL (read only) 642545 642569 24 0.0
.debug_frame 299752 299764 12 0.0
.debug_line 2655138 2655140 2 0.0
.debug_loc 2795342 2795460 118 0.0
.debug_str 3017877 3018033 156 0.0
.strtab 377630 377761 131 0.0
.symtab 256192 256256 64 0.0
.text 534604 534628 24 0.0
Decreases (1 build for cc32xx)
platform target config section c6cadc8 cc84c2e change % change
cc32xx lock CC3235SF_LAUNCHXL .debug_abbrev 930168 930138 -30 -0.0
.debug_info 20254045 20249762 -4283 -0.0
Full report (1 build for cc32xx)
platform target config section c6cadc8 cc84c2e change % change
cc32xx lock CC3235SF_LAUNCHXL 0 0 0 0.0
(read only) 642545 642569 24 0.0
(read/write) 203664 203664 0 0.0
.ARM.attributes 44 44 0 0.0
.ARM.exidx 8 8 0 0.0
.bss 197064 197064 0 0.0
.comment 194 194 0 0.0
.data 1480 1480 0 0.0
.debug_abbrev 930168 930138 -30 -0.0
.debug_aranges 87272 87272 0 0.0
.debug_frame 299752 299764 12 0.0
.debug_info 20254045 20249762 -4283 -0.0
.debug_line 2655138 2655140 2 0.0
.debug_loc 2795342 2795460 118 0.0
.debug_ranges 281592 281592 0 0.0
.debug_str 3017877 3018033 156 0.0
.ramVecs 780 780 0 0.0
.resetVecs 64 64 0 0.0
.rodata 105817 105817 0 0.0
.shstrtab 232 232 0 0.0
.stab 204 204 0 0.0
.stabstr 441 441 0 0.0
.stack 2048 2048 0 0.0
.strtab 377630 377761 131 0.0
.symtab 256192 256256 64 0.0
.text 534604 534628 24 0.0

@yunhanw-google yunhanw-google enabled auto-merge (squash) February 22, 2023 21:49
@yunhanw-google yunhanw-google force-pushed the bug/fix_event_buffer_size branch from cc84c2e to 3e57f6d Compare February 22, 2023 21:52
@github-actions
Copy link

PR #25234: Size comparison from c6cadc8 to 3e57f6d

Increases (1 build for cc32xx)
platform target config section c6cadc8 3e57f6d change % change
cc32xx lock CC3235SF_LAUNCHXL (read only) 642545 642569 24 0.0
.debug_frame 299752 299764 12 0.0
.debug_line 2655138 2655140 2 0.0
.debug_loc 2795342 2795460 118 0.0
.debug_str 3017877 3018033 156 0.0
.strtab 377630 377761 131 0.0
.symtab 256192 256256 64 0.0
.text 534604 534628 24 0.0
Decreases (1 build for cc32xx)
platform target config section c6cadc8 3e57f6d change % change
cc32xx lock CC3235SF_LAUNCHXL .debug_abbrev 930168 930138 -30 -0.0
.debug_info 20254045 20249763 -4282 -0.0
Full report (1 build for cc32xx)
platform target config section c6cadc8 3e57f6d change % change
cc32xx lock CC3235SF_LAUNCHXL 0 0 0 0.0
(read only) 642545 642569 24 0.0
(read/write) 203664 203664 0 0.0
.ARM.attributes 44 44 0 0.0
.ARM.exidx 8 8 0 0.0
.bss 197064 197064 0 0.0
.comment 194 194 0 0.0
.data 1480 1480 0 0.0
.debug_abbrev 930168 930138 -30 -0.0
.debug_aranges 87272 87272 0 0.0
.debug_frame 299752 299764 12 0.0
.debug_info 20254045 20249763 -4282 -0.0
.debug_line 2655138 2655140 2 0.0
.debug_loc 2795342 2795460 118 0.0
.debug_ranges 281592 281592 0 0.0
.debug_str 3017877 3018033 156 0.0
.ramVecs 780 780 0 0.0
.resetVecs 64 64 0 0.0
.rodata 105817 105817 0 0.0
.shstrtab 232 232 0 0.0
.stab 204 204 0 0.0
.stabstr 441 441 0 0.0
.stack 2048 2048 0 0.0
.strtab 377630 377761 131 0.0
.symtab 256192 256256 64 0.0
.text 534604 534628 24 0.0

@yunhanw-google yunhanw-google merged commit 015f11c into project-chip:master Feb 22, 2023
lecndav pushed a commit to lecndav/connectedhomeip that referenced this pull request Mar 22, 2023
@yunhanw-google
Fix event buffer size boundary check (project-chip#25234)
b6ae732 
* improve event log dump

* Fix event buffer size boundary check
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andy31415 andy31415 andy31415 approved these changes

@bzbarsky-apple bzbarsky-apple bzbarsky-apple approved these changes

@amitnj amitnj Awaiting requested review from amitnj

@anush-apple anush-apple Awaiting requested review from anush-apple

@arkq arkq Awaiting requested review from arkq

@Byungjoo-Lee Byungjoo-Lee Awaiting requested review from Byungjoo-Lee

@carol-apple carol-apple Awaiting requested review from carol-apple

@chrisdecenzo chrisdecenzo Awaiting requested review from chrisdecenzo

@chshu chshu Awaiting requested review from chshu

@chulspro chulspro Awaiting requested review from chulspro

@CodeChronos928 CodeChronos928 Awaiting requested review from CodeChronos928

@Damian-Nordic Damian-Nordic Awaiting requested review from Damian-Nordic

@dhrishi dhrishi Awaiting requested review from dhrishi

@electrocucaracha electrocucaracha Awaiting requested review from electrocucaracha

@franck-apple franck-apple Awaiting requested review from franck-apple

@gjc13 gjc13 Awaiting requested review from gjc13

@harimau-qirex harimau-qirex Awaiting requested review from harimau-qirex

@harsha-rajendran harsha-rajendran Awaiting requested review from harsha-rajendran

@hawk248 hawk248 Awaiting requested review from hawk248

@jelderton jelderton Awaiting requested review from jelderton

@jepenven-silabs jepenven-silabs Awaiting requested review from jepenven-silabs

@jmartinez-silabs jmartinez-silabs Awaiting requested review from jmartinez-silabs

@jmeg-sfy jmeg-sfy Awaiting requested review from jmeg-sfy

@jtung-apple jtung-apple Awaiting requested review from jtung-apple

@kcoppock kcoppock Awaiting requested review from kcoppock

@kkasperczyk-no kkasperczyk-no Awaiting requested review from kkasperczyk-no

@ksperling-apple ksperling-apple Awaiting requested review from ksperling-apple

@lazarkov lazarkov Awaiting requested review from lazarkov

@lpbeliveau-silabs lpbeliveau-silabs Awaiting requested review from lpbeliveau-silabs

@LuDuda LuDuda Awaiting requested review from LuDuda

@mlepage-google mlepage-google Awaiting requested review from mlepage-google

@mrjerryjohns mrjerryjohns Awaiting requested review from mrjerryjohns

@msandstedt msandstedt Awaiting requested review from msandstedt

@mspang mspang Awaiting requested review from mspang

@nivi-apple nivi-apple Awaiting requested review from nivi-apple

@pjzander-signify pjzander-signify Awaiting requested review from pjzander-signify

@saurabhst saurabhst Awaiting requested review from saurabhst

@selissia selissia Awaiting requested review from selissia

@tcarmelveilleux tcarmelveilleux Awaiting requested review from tcarmelveilleux

@tecimovic tecimovic Awaiting requested review from tecimovic

@tehampson tehampson Awaiting requested review from tehampson

@turon turon Awaiting requested review from turon

@vijs vijs Awaiting requested review from vijs

@vivien-apple vivien-apple Awaiting requested review from vivien-apple

@woody-apple woody-apple Awaiting requested review from woody-apple

@xylophone21 xylophone21 Awaiting requested review from xylophone21

@yufengwangca yufengwangca Awaiting requested review from yufengwangca

Assignees
No one assigned
Labels
app core lib review - approved tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yunhanw-google @bzbarsky-apple @andy31415