Revisions to Inet::IPAddress #10791

kpschoedel · 2021-10-21T18:57:59Z

Problem

Inet::IPAddress has some redundancies and outdated comments.

Change overview

Convert IPAddress::From…() functions that are semantically
constructors into constructors.
Replace several identical unions with IPAddress::SockAddr.
Fix outdated documentation regarding IPv4-in-IPv6.
Fix a use of IPAddress::kMaxStringLength (from PR# 10450 review).

Testing

CI; no changes to functionality intended.

#### Problem `Inet::IPAddress` has some redundancies and outdated comments. #### Change overview - Convert `IPAddress::From…()` functions that are semantically constructors into constructors. - Replace several identical `union`s with `IPAddress::SockAddr`. - Fix outdated documentation regarding IPv4-in-IPv6. - Fix a use of `IPAddress::kMaxStringLength` (from project-chip#10450 review). #### Testing CI; no changes to functionality intended.

kpschoedel · 2021-10-21T19:01:27Z

Split off from PR #10450

github-actions · 2021-10-21T19:24:14Z

PR #10791: Size comparison from 1b9ac5f to 4410644

8 builds (for k32w, p6, qpg, telink)

platform	target	config	section	`1b9ac5f`	`4410644`	change	% change
k32w	lock-app	k32w061+debug	.bss	69060	69060	0	0.0
			.data	1864	1864	0	0.0
			.text	515200	515152	-48	-0.0
	shell	k32w061+debug	.bss	55080	55080	0	0.0
			.data	672	672	0	0.0
			.text	357416	357368	-48	-0.0
	lighting-app	k32w061+se05x+release	.bss	78584	78584	0	0.0
			.data	1900	1900	0	0.0
			.text	613984	613936	-48	-0.0
p6	lock-app	default	.bss	68208	68208	0	0.0
			.data	2416	2416	0	0.0
			.heap	962720	962720	0	0.0
			.text	1126088	1126088	0	0.0
qpg	lighting-app	qpg6100+debug	.bss	53552	53552	0	0.0
			.data	996	996	0	0.0
			.text	486164	486148	-16	-0.0
	lock-app	qpg6100+debug	.bss	52496	52496	0	0.0
			.data	952	952	0	0.0
			.text	462240	462216	-24	-0.0
	persistent-storage-app	qpg6100+debug	.bss	17778	17778	0	0.0
			.data	280	280	0	0.0
			.text	102756	102756	0	0.0
telink	lighting-app	tlsr9518adk80d	bss	70992	70992	0	0.0
			noinit	33216	33216	0	0.0
			text	457820	457802	-18	-0.0

14 builds (for efr32, linux, mbed)

platform	target	config	section	`1b9ac5f`	`4410644`	change	% change
efr32	lighting-app	BRD4161A	.bss	118028	118028	0	0.0
			.data	1800	1800	0	0.0
			.text	782424	782392	-32	-0.0
	lock-app	BRD4161A	.bss	115884	115884	0	0.0
			.data	1760	1760	0	0.0
			.text	761544	761528	-16	-0.0
	window-app	BRD4161A	.bss	116204	116204	0	0.0
			.data	1764	1764	0	0.0
			.text	762460	762428	-32	-0.0
	lighting-app	BRD4161A+rpc	.bss	131356	131356	0	0.0
			.data	1852	1852	0	0.0
			.text	762124	762108	-16	-0.0
linux	all-clusters-app	debug	.bss	52176	52176	0	0.0
			.data	978	978	0	0.0
			.data.rel.ro	59072	59072	0	0.0
			.dynamic	592	592	0	0.0
			.got	4080	4080	0	0.0
			.init	27	27	0	0.0
			.init_array	512	512	0	0.0
			.rodata	136149	136149	0	0.0
			.text	1341346	1341458	112	0.0
	chip-tool	debug	.bss	17520	17520	0	0.0
			.data	1584	1584	0	0.0
			.data.rel.ro	91984	91984	0	0.0
			.dynamic	592	592	0	0.0
			.got	4344	4344	0	0.0
			.init	27	27	0	0.0
			.init_array	416	416	0	0.0
			.rodata	183492	183492	0	0.0
			.text	3456277	3456389	112	0.0
	ota-provider-app	debug	.bss	37440	37440	0	0.0
			.data	752	752	0	0.0
			.data.rel.ro	23176	23176	0	0.0
			.dynamic	592	592	0	0.0
			.got	4008	4008	0	0.0
			.init	27	27	0	0.0
			.init_array	440	440	0	0.0
			.rodata	109928	109928	0	0.0
			.text	1009010	1009122	112	0.0
	ota-requestor-app	debug	.bss	205696	205696	0	0.0
			.data	752	752	0	0.0
			.data.rel.ro	24488	24488	0	0.0
			.dynamic	592	592	0	0.0
			.got	4136	4136	0	0.0
			.init	27	27	0	0.0
			.init_array	512	512	0	0.0
			.rodata	128072	128072	0	0.0
			.text	1127554	1127666	112	0.0
	shell	debug	.bss	16072	16072	0	0.0
			.data	242	242	0	0.0
			.data.rel.ro	35120	35120	0	0.0
			.dynamic	592	592	0	0.0
			.got	3496	3496	0	0.0
			.init	27	27	0	0.0
			.init_array	336	336	0	0.0
			.rodata	71823	71823	0	0.0
			.text	570146	570242	96	0.0
	tv-app	debug	.bss	216560	216560	0	0.0
			.data	2032	2032	0	0.0
			.data.rel.ro	56096	56096	0	0.0
			.dynamic	592	592	0	0.0
			.got	4400	4400	0	0.0
			.init	27	27	0	0.0
			.init_array	608	608	0	0.0
			.rodata	152680	152680	0	0.0
			.text	1442370	1442482	112	0.0
	bridge-app	debug+rpc	.bss	52880	52880	0	0.0
			.data	976	976	0	0.0
			.data.rel.ro	25784	25784	0	0.0
			.dynamic	592	592	0	0.0
			.got	3944	3944	0	0.0
			.init	27	27	0	0.0
			.init_array	400	400	0	0.0
			.rodata	110524	110524	0	0.0
			.text	1050277	1050437	160	0.0
	lighting-app	debug+rpc	.bss	42200	42200	0	0.0
			.data	1106	1106	0	0.0
			.data.rel.ro	52496	52496	0	0.0
			.dynamic	608	608	0	0.0
			.got	4104	4104	0	0.0
			.init	27	27	0	0.0
			.init_array	528	528	0	0.0
			.rodata	127697	127697	0	0.0
			.text	1250434	1250546	112	0.0
mbed	lighting-app	CY8CPROTO_062_4343W+release	.bss	172092	172092	0	0.0
			.data	5464	5464	0	0.0
			.heap	858888	858888	0	0.0
			.text	1219728	1219664	-64	-0.0
	lock-app	CY8CPROTO_062_4343W+release	.bss	171012	171012	0	0.0
			.data	5432	5432	0	0.0
			.heap	860000	860000	0	0.0
			.text	1197624	1197560	-64	-0.0

12 builds (for esp32, nrfconnect)

platform	target	config	section	`1b9ac5f`	`4410644`	change	% change
esp32	all-clusters-app	c3devkit	.dram0.bss	60296	60296	0	0.0
			.dram0.data	16432	16432	0	0.0
			.flash.rodata	199072	199072	0	0.0
			.flash.text	873322	873282	-40	-0.0
			.iram0.text	57564	57564	0	0.0
		m5stack	.dram0.bss	62800	62800	0	0.0
			.dram0.data	32084	32084	0	0.0
			.flash.rodata	207832	207832	0	0.0
			.flash.text	904379	904343	-36	-0.0
			.iram0.text	125115	125115	0	0.0
nrfconnect	lighting-app	nrf52840dk_nrf52840	bss	113172	113172	0	0.0
			rodata	97740	97740	0	0.0
			text	577332	577332	0	0.0
	lock-app	nrf52840dk_nrf52840	bss	112240	112240	0	0.0
			rodata	94076	94076	0	0.0
			text	558820	558820	0	0.0
	pigweed-app	nrf52840dk_nrf52840	bss	51824	51824	0	0.0
			rodata	45776	45776	0	0.0
			text	339456	339456	0	0.0
	pump-app	nrf52840dk_nrf52840	bss	112308	112308	0	0.0
			rodata	95056	95056	0	0.0
			text	561972	561972	0	0.0
	pump-controller-app	nrf52840dk_nrf52840	bss	112248	112248	0	0.0
			rodata	94136	94136	0	0.0
			text	558612	558612	0	0.0
	shell	nrf52840dk_nrf52840	bss	109068	109068	0	0.0
			rodata	71644	71644	0	0.0
			text	519104	519104	0	0.0
	lighting-app	nrf52840dk_nrf52840+rpc	bss	109416	109416	0	0.0
			rodata	88524	88524	0	0.0
			text	550532	550532	0	0.0
		nrf5340dk_nrf5340_cpuapp	bss	114548	114548	0	0.0
			rodata	92980	92980	0	0.0
			text	506800	506800	0	0.0
	lock-app	nrf5340dk_nrf5340_cpuapp	bss	113612	113612	0	0.0
			rodata	89336	89336	0	0.0
			text	488284	488284	0	0.0
	shell	nrf5340dk_nrf5340_cpuapp	bss	110052	110052	0	0.0
			rodata	66288	66288	0	0.0
			text	439720	439720	0	0.0

github-actions · 2021-10-21T19:38:10Z

Size increase report for "gn_qpg-example-build" from 1b9ac5f

File	Section	File	VM
chip-qpg6100-lighting-example.out	.text	-16	-16

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-qpg6100-lighting-example.out and ./pull_artifact/chip-qpg6100-lighting-example.out:

sections,vmsize,filesize
.strtab,0,26
.symtab,0,16
[Unmapped],0,16
.debug_abbrev,0,4
.shstrtab,0,-2
.debug_aranges,0,-8
.text,-16,-16
.debug_ranges,0,-32
.debug_frame,0,-36
.debug_line,0,-81
.debug_str,0,-103
.debug_loc,0,-474
.debug_info,0,-2010

Comparing ./master_artifact/chip-qpg6100-lighting-example.out.map and ./pull_artifact/chip-qpg6100-lighting-example.out.map:

BLOAT EXECUTION FAILED WITH CODE 1:
bloaty: unknown file type for file './pull_artifact/chip-qpg6100-lighting-example.out.map'

andy31415 · 2021-10-21T19:45:00Z

Candidate for fast track (code refactor), however I would like one more reviewer beside myself since the changes are reasonably large.

src/inet/IPAddress.cpp

src/inet/IPAddress.h

src/inet/IPAddress.cpp

github-actions · 2021-10-21T20:13:04Z

Size increase report for "esp32-example-build" from 1b9ac5f

File	Section	File	VM
chip-all-clusters-app.elf	.flash.text	-40	-40

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-all-clusters-app.elf and ./pull_artifact/chip-all-clusters-app.elf:

sections,vmsize,filesize
.debug_info,0,2195
.strtab,0,87
.debug_str,0,56
.debug_abbrev,0,53
.symtab,0,48
[Unmapped],0,40
.riscv.attributes,0,1
.shstrtab,0,-3
.debug_frame,0,-16
.debug_loc,0,-32
.flash.text,-40,-40
.debug_ranges,0,-48
.debug_line,0,-157

github-actions · 2021-10-21T22:07:40Z

PR #10791: Size comparison from 1b9ac5f to b60c337

22 builds (for efr32, k32w, linux, mbed, p6, qpg, telink)

platform	target	config	section	`1b9ac5f`	`b60c337`	change	% change
efr32	lighting-app	BRD4161A	.bss	118028	118028	0	0.0
			.data	1800	1800	0	0.0
			.text	782424	781600	-824	-0.1
	lock-app	BRD4161A	.bss	115884	115884	0	0.0
			.data	1760	1760	0	0.0
			.text	761544	760744	-800	-0.1
	window-app	BRD4161A	.bss	116204	116204	0	0.0
			.data	1764	1764	0	0.0
			.text	762460	761652	-808	-0.1
	lighting-app	BRD4161A+rpc	.bss	131356	131356	0	0.0
			.data	1852	1852	0	0.0
			.text	762124	761316	-808	-0.1
k32w	lock-app	k32w061+debug	.bss	69060	69060	0	0.0
			.data	1864	1864	0	0.0
			.text	515200	514504	-696	-0.1
	shell	k32w061+debug	.bss	55080	55080	0	0.0
			.data	672	672	0	0.0
			.text	357416	357240	-176	-0.0
	lighting-app	k32w061+se05x+release	.bss	78584	78584	0	0.0
			.data	1900	1900	0	0.0
			.text	613984	613272	-712	-0.1
linux	all-clusters-app	debug	.bss	52176	52176	0	0.0
			.data	978	978	0	0.0
			.data.rel.ro	59072	59072	0	0.0
			.dynamic	592	592	0	0.0
			.got	4080	4080	0	0.0
			.init	27	27	0	0.0
			.init_array	512	512	0	0.0
			.rodata	136149	136117	-32	-0.0
			.text	1341346	1343746	2400	0.2
	chip-tool	debug	.bss	17520	17520	0	0.0
			.data	1584	1584	0	0.0
			.data.rel.ro	91984	91984	0	0.0
			.dynamic	592	592	0	0.0
			.got	4344	4344	0	0.0
			.init	27	27	0	0.0
			.init_array	416	416	0	0.0
			.rodata	183492	183524	32	0.0
			.text	3456277	3459701	3424	0.1
	ota-provider-app	debug	.bss	37440	37440	0	0.0
			.data	752	752	0	0.0
			.data.rel.ro	23176	23176	0	0.0
			.dynamic	592	592	0	0.0
			.got	4008	4008	0	0.0
			.init	27	27	0	0.0
			.init_array	440	440	0	0.0
			.rodata	109928	109960	32	0.0
			.text	1009010	1012578	3568	0.4
	ota-requestor-app	debug	.bss	205696	205696	0	0.0
			.data	752	752	0	0.0
			.data.rel.ro	24488	24488	0	0.0
			.dynamic	592	592	0	0.0
			.got	4136	4136	0	0.0
			.init	27	27	0	0.0
			.init_array	512	512	0	0.0
			.rodata	128072	128104	32	0.0
			.text	1127554	1131122	3568	0.3
	shell	debug	.bss	16072	16072	0	0.0
			.data	242	242	0	0.0
			.data.rel.ro	35120	35120	0	0.0
			.dynamic	592	592	0	0.0
			.got	3496	3496	0	0.0
			.init	27	27	0	0.0
			.init_array	336	336	0	0.0
			.rodata	71823	71887	64	0.1
			.text	570146	574146	4000	0.7
	tv-app	debug	.bss	216560	216560	0	0.0
			.data	2032	2032	0	0.0
			.data.rel.ro	56096	56096	0	0.0
			.dynamic	592	592	0	0.0
			.got	4400	4400	0	0.0
			.init	27	27	0	0.0
			.init_array	608	608	0	0.0
			.rodata	152680	152616	-64	-0.0
			.text	1442370	1444738	2368	0.2
	bridge-app	debug+rpc	.bss	52880	52880	0	0.0
			.data	976	976	0	0.0
			.data.rel.ro	25784	25784	0	0.0
			.dynamic	592	592	0	0.0
			.got	3944	3944	0	0.0
			.init	27	27	0	0.0
			.init_array	400	400	0	0.0
			.rodata	110524	110492	-32	-0.0
			.text	1050277	1052485	2208	0.2
	lighting-app	debug+rpc	.bss	42200	42200	0	0.0
			.data	1106	1106	0	0.0
			.data.rel.ro	52496	52496	0	0.0
			.dynamic	608	608	0	0.0
			.got	4104	4104	0	0.0
			.init	27	27	0	0.0
			.init_array	528	528	0	0.0
			.rodata	127697	127665	-32	-0.0
			.text	1250434	1252738	2304	0.2
mbed	lighting-app	CY8CPROTO_062_4343W+release	.bss	172092	172092	0	0.0
			.data	5464	5464	0	0.0
			.heap	858888	858888	0	0.0
			.text	1219728	1219008	-720	-0.1
	lock-app	CY8CPROTO_062_4343W+release	.bss	171012	171012	0	0.0
			.data	5432	5432	0	0.0
			.heap	860000	860000	0	0.0
			.text	1197624	1196840	-784	-0.1
p6	lock-app	default	.bss	68208	68208	0	0.0
			.data	2416	2416	0	0.0
			.heap	962720	962720	0	0.0
			.text	1126088	`1125280`	-808	-0.1
qpg	lighting-app	qpg6100+debug	.bss	53552	53552	0	0.0
			.data	996	996	0	0.0
			.text	486164	485484	-680	-0.1
	lock-app	qpg6100+debug	.bss	52496	52496	0	0.0
			.data	952	952	0	0.0
			.text	462240	461572	-668	-0.1
	persistent-storage-app	qpg6100+debug	.bss	17778	17778	0	0.0
			.data	280	280	0	0.0
			.text	102756	102628	-128	-0.1
telink	lighting-app	tlsr9518adk80d	bss	70992	70992	0	0.0
			noinit	33216	33216	0	0.0
			text	457820	457112	-708	-0.2

12 builds (for esp32, nrfconnect)

platform	target	config	section	`1b9ac5f`	`b60c337`	change	% change
esp32	all-clusters-app	c3devkit	.dram0.bss	60296	60296	0	0.0
			.dram0.data	16432	16432	0	0.0
			.flash.rodata	199072	199072	0	0.0
			.flash.text	873322	873090	-232	-0.0
			.iram0.text	57564	57564	0	0.0
		m5stack	.dram0.bss	62800	62800	0	0.0
			.dram0.data	32084	32084	0	0.0
			.flash.rodata	207832	207832	0	0.0
			.flash.text	904379	904231	-148	-0.0
			.iram0.text	125115	125115	0	0.0
nrfconnect	lighting-app	nrf52840dk_nrf52840	bss	113172	113172	0	0.0
			rodata	97740	97668	-72	-0.1
			text	577332	576752	-580	-0.1
	lock-app	nrf52840dk_nrf52840	bss	112240	112240	0	0.0
			rodata	94076	94004	-72	-0.1
			text	558820	558240	-580	-0.1
	pigweed-app	nrf52840dk_nrf52840	bss	51824	51824	0	0.0
			rodata	45776	45776	0	0.0
			text	339456	339456	0	0.0
	pump-app	nrf52840dk_nrf52840	bss	112308	112308	0	0.0
			rodata	95056	94988	-68	-0.1
			text	561972	561396	-576	-0.1
	pump-controller-app	nrf52840dk_nrf52840	bss	112248	112248	0	0.0
			rodata	94136	94064	-72	-0.1
			text	558612	558032	-580	-0.1
	shell	nrf52840dk_nrf52840	bss	109068	109068	0	0.0
			rodata	71644	71644	0	0.0
			text	519104	518980	-124	-0.0
	lighting-app	nrf52840dk_nrf52840+rpc	bss	109416	109416	0	0.0
			rodata	88524	88448	-76	-0.1
			text	550532	549952	-580	-0.1
		nrf5340dk_nrf5340_cpuapp	bss	114548	114548	0	0.0
			rodata	92980	92908	-72	-0.1
			text	506800	506224	-576	-0.1
	lock-app	nrf5340dk_nrf5340_cpuapp	bss	113612	113612	0	0.0
			rodata	89336	89264	-72	-0.1
			text	488284	487704	-580	-0.1
	shell	nrf5340dk_nrf5340_cpuapp	bss	110052	110052	0	0.0
			rodata	66288	66288	0	0.0
			text	439720	439592	-128	-0.0

#### Problem When LwIP is configured for IPv6 only, its type `ip_addr_t` is identical to `ip6_addr_t`; otherwise they are different. PR project-chip#10791 made the incorrect assumption that we would never build CHIP without IPv4 when then platform LwIP is configured with IPv4, and left out the constructor overload necessary for that case. #### Change overview Enable the `ip_addr_t` constructor if `LWIP_IPV4` is true. #### Testing Built ``` scripts/build/build_examples.py \ --target esp32-m5stack-all-clusters-ipv6only build` ```

#### Problem When LwIP is configured for IPv6 only, its type `ip_addr_t` is identical to `ip6_addr_t`; otherwise they are different. PR #10791 made the incorrect assumption that we would never build CHIP without IPv4 when then platform LwIP is configured with IPv4, and left out the constructor overload necessary for that case. #### Change overview Enable the `ip_addr_t` constructor if `LWIP_IPV4` is true. #### Testing Built ``` scripts/build/build_examples.py \ --target esp32-m5stack-all-clusters-ipv6only build` ```

boring-cyborg bot added inet lib linux openthread platform labels Oct 21, 2021

pullapprove bot requested review from robszewczyk, sagar-apple, saurabhst, tcarmelveilleux, vivien-apple, wbschiller and woody-apple October 21, 2021 19:03

pullapprove bot added the review - pending label Oct 21, 2021

andy31415 approved these changes Oct 21, 2021

View reviewed changes

andy31415 reviewed Oct 21, 2021

View reviewed changes

src/inet/IPAddress.cpp Outdated Show resolved Hide resolved

andy31415 reviewed Oct 21, 2021

View reviewed changes

src/inet/IPAddress.cpp Show resolved Hide resolved

msandstedt reviewed Oct 21, 2021

View reviewed changes

src/inet/IPAddress.h Show resolved Hide resolved

andy31415 reviewed Oct 21, 2021

View reviewed changes

src/inet/IPAddress.h Show resolved Hide resolved

msandstedt reviewed Oct 21, 2021

View reviewed changes

src/inet/IPAddress.cpp Show resolved Hide resolved

review

b60c337

msandstedt approved these changes Oct 21, 2021

View reviewed changes

andy31415 added the fast track label Oct 22, 2021

andy31415 merged commit 33ca522 into project-chip:master Oct 22, 2021

kpschoedel deleted the x7715-inet-3d-ipaddress branch October 22, 2021 14:59

kpschoedel mentioned this pull request Oct 22, 2021

Fix IPv6-only builds where platform LwIP has IPv4 #10879

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revisions to Inet::IPAddress #10791

Revisions to Inet::IPAddress #10791

kpschoedel commented Oct 21, 2021

kpschoedel commented Oct 21, 2021

github-actions bot commented Oct 21, 2021 •

edited

Loading

github-actions bot commented Oct 21, 2021

andy31415 commented Oct 21, 2021

github-actions bot commented Oct 21, 2021

github-actions bot commented Oct 21, 2021 •

edited

Loading

Revisions to Inet::IPAddress #10791

Revisions to Inet::IPAddress #10791

Conversation

kpschoedel commented Oct 21, 2021

Problem

Change overview

Testing

kpschoedel commented Oct 21, 2021

github-actions bot commented Oct 21, 2021 • edited Loading

github-actions bot commented Oct 21, 2021

andy31415 commented Oct 21, 2021

github-actions bot commented Oct 21, 2021

github-actions bot commented Oct 21, 2021 • edited Loading

github-actions bot commented Oct 21, 2021 •

edited

Loading

github-actions bot commented Oct 21, 2021 •

edited

Loading