Build failure with trunk GCC 15 in fs/btrfs/print-tree.c (-Wunterminated-string-initialization)

Build failure with trunk GCC 15 in fs/btrfs/print-tree.c (-Wunterminated-string-initialization)

[Sam James]

GCC 15 introduces a new warning -Wunterminated-string-initialization
which causes, with the kernel's -Werror=..., the following:
```
/var/tmp/portage/sys-kernel/gentoo-kernel-6.6.41/work/linux-6.6/fs/btrfs/print-tree.c:29:49: error: initializer-string for array of ‘char’ is too long [-Werror=unterminated-string-initialization]
   29 |         { BTRFS_BLOCK_GROUP_TREE_OBJECTID,      "BLOCK_GROUP_TREE"      },
      |
      ^~~~~~~~~~~~~~~~~~
```

It was introduced in https://gcc.gnu.org/PR115185. I don't have time
today to check the case to see what the best fix is, but CCing Alex who
wrote the warning implementation in case he has a chance.

thanks,
sam

Re: Build failure with trunk GCC 15 in fs/btrfs/print-tree.c (-Wunterminated-string-initialization)

[Alejandro Colomar]

[-- Attachment #1: Type: text/plain, Size: 3733 bytes --]

Hi Sam!

On Thu, Jul 18, 2024 at 10:54:40PM GMT, Sam James wrote:
> GCC 15 introduces a new warning -Wunterminated-string-initialization
> which causes, with the kernel's -Werror=..., the following:
> ```
> /var/tmp/portage/sys-kernel/gentoo-kernel-6.6.41/work/linux-6.6/fs/btrfs/print-tree.c:29:49: error: initializer-string for array of ‘char’ is too long [-Werror=unterminated-string-initialization]
>    29 |         { BTRFS_BLOCK_GROUP_TREE_OBJECTID,      "BLOCK_GROUP_TREE"      },
>       |
>       ^~~~~~~~~~~~~~~~~~
> ```
> 
> It was introduced in https://gcc.gnu.org/PR115185. I don't have time
> today to check the case to see what the best fix is, but CCing Alex who
> wrote the warning implementation in case he has a chance.

Thanks for forwarding the report.  It looks like a legit diagnostic.  It
seems like a bug.

	$ sed -n 15,34p fs/btrfs/print-tree.c;
	struct root_name_map {
		u64 id;
		char name[16];
	};

	static const struct root_name_map root_map[] = {
		{ BTRFS_ROOT_TREE_OBJECTID,		"ROOT_TREE"		},
		{ BTRFS_EXTENT_TREE_OBJECTID,		"EXTENT_TREE"		},
		{ BTRFS_CHUNK_TREE_OBJECTID,		"CHUNK_TREE"		},
		{ BTRFS_DEV_TREE_OBJECTID,		"DEV_TREE"		},
		{ BTRFS_FS_TREE_OBJECTID,		"FS_TREE"		},
		{ BTRFS_CSUM_TREE_OBJECTID,		"CSUM_TREE"		},
		{ BTRFS_TREE_LOG_OBJECTID,		"TREE_LOG"		},
		{ BTRFS_QUOTA_TREE_OBJECTID,		"QUOTA_TREE"		},
		{ BTRFS_UUID_TREE_OBJECTID,		"UUID_TREE"		},
		{ BTRFS_FREE_SPACE_TREE_OBJECTID,	"FREE_SPACE_TREE"	},
		{ BTRFS_BLOCK_GROUP_TREE_OBJECTID,	"BLOCK_GROUP_TREE"	},
		{ BTRFS_DATA_RELOC_TREE_OBJECTID,	"DATA_RELOC_TREE"	},
		{ BTRFS_RAID_STRIPE_TREE_OBJECTID,	"RAID_STRIPE_TREE"	},
	};

The non-string is stored in 'root_map'.  It seems only used in one
function:

	$ grepc -tu root_map fs/btrfs/print-tree.c;
	fs/btrfs/print-tree.c:const char *btrfs_root_name(const struct btrfs_key *key, char *buf)
	{
		int i;

		if (key->objectid == BTRFS_TREE_RELOC_OBJECTID) {
			snprintf(buf, BTRFS_ROOT_NAME_BUF_LEN,
				 "TREE_RELOC offset=%llu", key->offset);
			return buf;
		}

		for (i = 0; i < ARRAY_SIZE(root_map); i++) {
			if (root_map[i].id == key->objectid)
				return root_map[i].name;
		}

		snprintf(buf, BTRFS_ROOT_NAME_BUF_LEN, "%llu", key->objectid);
		return buf;
	}

That function returns the non-string, and also seems to be used exactly
in one place:

	$ find * -type f \
	| grep '\.[hc]$' \
	| xargs grepc -tu btrfs_root_name;
	fs/btrfs/disk-io.c:void btrfs_check_leaked_roots(struct btrfs_fs_info *fs_info)
	{
	#ifdef CONFIG_BTRFS_DEBUG
		struct btrfs_root *root;

		while (!list_empty(&fs_info->allocated_roots)) {
			char buf[BTRFS_ROOT_NAME_BUF_LEN];

			root = list_first_entry(&fs_info->allocated_roots,
						struct btrfs_root, leak_list);
			btrfs_err(fs_info, "leaked root %s refcount %d",
				  btrfs_root_name(&root->root_key, buf),
				  refcount_read(&root->refs));
			WARN_ON_ONCE(1);
			while (refcount_read(&root->refs) > 1)
				btrfs_put_root(root);
			btrfs_put_root(root);
		}
	#endif
	}

This caller is using the non-string in a "%s" in btrfs_err(), which
itself is a wrapper around btrfs_printk(), which I won't follow too
much, but I expect to treat "%s" the same as printf(3) does.

The fix would be to add at least one byte to that array size.  Possibly
make it 32 for alignment.  But I don't know if that array size is fixed
by any ABI, so the maintainer will be better placed to find the suitable
fix.

The only alternatives I see are

-  Use a larger number of elements for that array (1 would be enough).
-  Use a shorter string so that it fits the 16 bytes.

Have a lovely night!
Alex

-- 
<https://www.alejandro-colomar.es/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Build failure with trunk GCC 15 in fs/btrfs/print-tree.c (-Wunterminated-string-initialization)

[Qu Wenruo]

在 2024/7/19 07:56, Alejandro Colomar 写道:
> Hi Sam!
>
> On Thu, Jul 18, 2024 at 10:54:40PM GMT, Sam James wrote:
>> GCC 15 introduces a new warning -Wunterminated-string-initialization
>> which causes, with the kernel's -Werror=..., the following:
>> ```
>> /var/tmp/portage/sys-kernel/gentoo-kernel-6.6.41/work/linux-6.6/fs/btrfs/print-tree.c:29:49: error: initializer-string for array of ‘char’ is too long [-Werror=unterminated-string-initialization]
>>     29 |         { BTRFS_BLOCK_GROUP_TREE_OBJECTID,      "BLOCK_GROUP_TREE"      },
>>        |
>>        ^~~~~~~~~~~~~~~~~~
>> ```

Great new GCC feature!

And it's indeed too long, not only for the block group tree, but also
for the future RAID_STRIPE_TREE too.

I believe we can just enlarge the string to 32 bytes for now.
I'll send out a fix soon.


On the other hand, it's better to do build time verification on those
string length.

Any good advice to craft some build time macro/functions to find the max
string length of a const array?
Or at least check against the array size.

Thank you guys!
Qu
>>
>> It was introduced in https://gcc.gnu.org/PR115185. I don't have time
>> today to check the case to see what the best fix is, but CCing Alex who
>> wrote the warning implementation in case he has a chance.
>
> Thanks for forwarding the report.  It looks like a legit diagnostic.  It
> seems like a bug.
>
> 	$ sed -n 15,34p fs/btrfs/print-tree.c;
> 	struct root_name_map {
> 		u64 id;
> 		char name[16];
> 	};
>
> 	static const struct root_name_map root_map[] = {
> 		{ BTRFS_ROOT_TREE_OBJECTID,		"ROOT_TREE"		},
> 		{ BTRFS_EXTENT_TREE_OBJECTID,		"EXTENT_TREE"		},
> 		{ BTRFS_CHUNK_TREE_OBJECTID,		"CHUNK_TREE"		},
> 		{ BTRFS_DEV_TREE_OBJECTID,		"DEV_TREE"		},
> 		{ BTRFS_FS_TREE_OBJECTID,		"FS_TREE"		},
> 		{ BTRFS_CSUM_TREE_OBJECTID,		"CSUM_TREE"		},
> 		{ BTRFS_TREE_LOG_OBJECTID,		"TREE_LOG"		},
> 		{ BTRFS_QUOTA_TREE_OBJECTID,		"QUOTA_TREE"		},
> 		{ BTRFS_UUID_TREE_OBJECTID,		"UUID_TREE"		},
> 		{ BTRFS_FREE_SPACE_TREE_OBJECTID,	"FREE_SPACE_TREE"	},
> 		{ BTRFS_BLOCK_GROUP_TREE_OBJECTID,	"BLOCK_GROUP_TREE"	},
> 		{ BTRFS_DATA_RELOC_TREE_OBJECTID,	"DATA_RELOC_TREE"	},
> 		{ BTRFS_RAID_STRIPE_TREE_OBJECTID,	"RAID_STRIPE_TREE"	},
> 	};
>
> The non-string is stored in 'root_map'.  It seems only used in one
> function:
>
> 	$ grepc -tu root_map fs/btrfs/print-tree.c;
> 	fs/btrfs/print-tree.c:const char *btrfs_root_name(const struct btrfs_key *key, char *buf)
> 	{
> 		int i;
>
> 		if (key->objectid == BTRFS_TREE_RELOC_OBJECTID) {
> 			snprintf(buf, BTRFS_ROOT_NAME_BUF_LEN,
> 				 "TREE_RELOC offset=%llu", key->offset);
> 			return buf;
> 		}
>
> 		for (i = 0; i < ARRAY_SIZE(root_map); i++) {
> 			if (root_map[i].id == key->objectid)
> 				return root_map[i].name;
> 		}
>
> 		snprintf(buf, BTRFS_ROOT_NAME_BUF_LEN, "%llu", key->objectid);
> 		return buf;
> 	}
>
> That function returns the non-string, and also seems to be used exactly
> in one place:
>
> 	$ find * -type f \
> 	| grep '\.[hc]$' \
> 	| xargs grepc -tu btrfs_root_name;
> 	fs/btrfs/disk-io.c:void btrfs_check_leaked_roots(struct btrfs_fs_info *fs_info)
> 	{
> 	#ifdef CONFIG_BTRFS_DEBUG
> 		struct btrfs_root *root;
>
> 		while (!list_empty(&fs_info->allocated_roots)) {
> 			char buf[BTRFS_ROOT_NAME_BUF_LEN];
>
> 			root = list_first_entry(&fs_info->allocated_roots,
> 						struct btrfs_root, leak_list);
> 			btrfs_err(fs_info, "leaked root %s refcount %d",
> 				  btrfs_root_name(&root->root_key, buf),
> 				  refcount_read(&root->refs));
> 			WARN_ON_ONCE(1);
> 			while (refcount_read(&root->refs) > 1)
> 				btrfs_put_root(root);
> 			btrfs_put_root(root);
> 		}
> 	#endif
> 	}
>
> This caller is using the non-string in a "%s" in btrfs_err(), which
> itself is a wrapper around btrfs_printk(), which I won't follow too
> much, but I expect to treat "%s" the same as printf(3) does.
>
> The fix would be to add at least one byte to that array size.  Possibly
> make it 32 for alignment.  But I don't know if that array size is fixed
> by any ABI, so the maintainer will be better placed to find the suitable
> fix.
>
> The only alternatives I see are
>
> -  Use a larger number of elements for that array (1 would be enough).
> -  Use a shorter string so that it fits the 16 bytes.
>
> Have a lovely night!
> Alex
>

Re: Build failure with trunk GCC 15 in fs/btrfs/print-tree.c (-Wunterminated-string-initialization)

[Alejandro Colomar]

[-- Attachment #1: Type: text/plain, Size: 1584 bytes --]

Hi Qu,

On Fri, Jul 19, 2024 at 10:49:09AM GMT, Qu Wenruo wrote:
> 
> 
> 在 2024/7/19 07:56, Alejandro Colomar 写道:
> > Hi Sam!
> > 
> > On Thu, Jul 18, 2024 at 10:54:40PM GMT, Sam James wrote:
> > > GCC 15 introduces a new warning -Wunterminated-string-initialization
> > > which causes, with the kernel's -Werror=..., the following:
> > > ```
> > > /var/tmp/portage/sys-kernel/gentoo-kernel-6.6.41/work/linux-6.6/fs/btrfs/print-tree.c:29:49: error: initializer-string for array of ‘char’ is too long [-Werror=unterminated-string-initialization]
> > >     29 |         { BTRFS_BLOCK_GROUP_TREE_OBJECTID,      "BLOCK_GROUP_TREE"      },
> > >        |
> > >        ^~~~~~~~~~~~~~~~~~
> > > ```
> 
> Great new GCC feature!

Thanks!!  It's a pleasure to see it working.  :-)

> And it's indeed too long, not only for the block group tree, but also
> for the future RAID_STRIPE_TREE too.

Yep, there are two entries too long.

> I believe we can just enlarge the string to 32 bytes for now.
> I'll send out a fix soon.
> 
> 
> On the other hand, it's better to do build time verification on those
> string length.
> 
> Any good advice to craft some build time macro/functions to find the max
> string length of a const array?

I don't think you can write any magic macro for that.  But you can
specify

	CFLAGS += -Werror=unterminated-string-initialization

That should do what you want.

> Or at least check against the array size.
> 
> Thank you guys!
> Qu

Have a lovely day!
Alex

-- 
<https://www.alejandro-colomar.es/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Build failure with trunk GCC 15 in fs/btrfs/print-tree.c (-Wunterminated-string-initialization)

[Alejandro Colomar]

[-- Attachment #1: Type: text/plain, Size: 1588 bytes --]

On Fri, Jul 19, 2024 at 10:39:06AM GMT, Alejandro Colomar wrote:
> Hi Qu,
> 
> On Fri, Jul 19, 2024 at 10:49:09AM GMT, Qu Wenruo wrote:
> > 
> > 
> > 在 2024/7/19 07:56, Alejandro Colomar 写道:
> > > Hi Sam!
> > > 
> > > On Thu, Jul 18, 2024 at 10:54:40PM GMT, Sam James wrote:
> > > > GCC 15 introduces a new warning -Wunterminated-string-initialization
> > > > which causes, with the kernel's -Werror=..., the following:
> > > > ```
> > > > /var/tmp/portage/sys-kernel/gentoo-kernel-6.6.41/work/linux-6.6/fs/btrfs/print-tree.c:29:49: error: initializer-string for array of ‘char’ is too long [-Werror=unterminated-string-initialization]
> > > >     29 |         { BTRFS_BLOCK_GROUP_TREE_OBJECTID,      "BLOCK_GROUP_TREE"      },
> > > >        |
> > > >        ^~~~~~~~~~~~~~~~~~
> > > > ```
> > 
> > Great new GCC feature!
> 
> Thanks!!  It's a pleasure to see it working.  :-)
> 
> > And it's indeed too long, not only for the block group tree, but also
> > for the future RAID_STRIPE_TREE too.
> 
> Yep, there are two entries too long.
> 
> > I believe we can just enlarge the string to 32 bytes for now.
> > I'll send out a fix soon.

Please add:

Fixes: 9c54e80ddc6b ("btrfs: add code to support the block group root")
Reported-by: Sam James <sam@gentoo.org>
Reported-by: Alejandro Colomar <alx@kernel.org>
Suggested-by: Alejandro Colomar <alx@kernel.org>

And this will be useful:

$ git describe --contains 9c54e80ddc6bd89596a4046d451908700476fd14
v5.18-rc1~172^2~64

Cheers,
Alex


-- 
<https://www.alejandro-colomar.es/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Build failure with trunk GCC 15 in fs/btrfs/print-tree.c (-Wunterminated-string-initialization)

[Alejandro Colomar]

[-- Attachment #1: Type: text/plain, Size: 1896 bytes --]

On Fri, Jul 19, 2024 at 10:47:52AM GMT, Alejandro Colomar wrote:
> On Fri, Jul 19, 2024 at 10:39:06AM GMT, Alejandro Colomar wrote:
> > Hi Qu,
> > 
> > On Fri, Jul 19, 2024 at 10:49:09AM GMT, Qu Wenruo wrote:
> > > 
> > > 
> > > 在 2024/7/19 07:56, Alejandro Colomar 写道:
> > > > Hi Sam!
> > > > 
> > > > On Thu, Jul 18, 2024 at 10:54:40PM GMT, Sam James wrote:
> > > > > GCC 15 introduces a new warning -Wunterminated-string-initialization
> > > > > which causes, with the kernel's -Werror=..., the following:
> > > > > ```
> > > > > /var/tmp/portage/sys-kernel/gentoo-kernel-6.6.41/work/linux-6.6/fs/btrfs/print-tree.c:29:49: error: initializer-string for array of ‘char’ is too long [-Werror=unterminated-string-initialization]
> > > > >     29 |         { BTRFS_BLOCK_GROUP_TREE_OBJECTID,      "BLOCK_GROUP_TREE"      },
> > > > >        |
> > > > >        ^~~~~~~~~~~~~~~~~~
> > > > > ```
> > > 
> > > Great new GCC feature!
> > 
> > Thanks!!  It's a pleasure to see it working.  :-)
> > 
> > > And it's indeed too long, not only for the block group tree, but also
> > > for the future RAID_STRIPE_TREE too.
> > 
> > Yep, there are two entries too long.
> > 
> > > I believe we can just enlarge the string to 32 bytes for now.
> > > I'll send out a fix soon.
> 
> Please add:
> 
> Fixes: 9c54e80ddc6b ("btrfs: add code to support the block group root")

And also (I forgot)

Fixes: edde81f1abf29 ("btrfs: add raid stripe tree pretty printer")

> Reported-by: Sam James <sam@gentoo.org>
> Reported-by: Alejandro Colomar <alx@kernel.org>
> Suggested-by: Alejandro Colomar <alx@kernel.org>
> 
> And this will be useful:
> 
> $ git describe --contains 9c54e80ddc6bd89596a4046d451908700476fd14
> v5.18-rc1~172^2~64
> 
> Cheers,
> Alex
> 
> 
> -- 
> <https://www.alejandro-colomar.es/>



-- 
<https://www.alejandro-colomar.es/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Build failure with trunk GCC 15 in fs/btrfs/print-tree.c (-Wunterminated-string-initialization)

[Sam James]

[-- Attachment #1: Type: text/plain, Size: 1707 bytes --]

Alejandro Colomar <alx@kernel.org> writes:

> Hi Sam!
>
> On Thu, Jul 18, 2024 at 10:54:40PM GMT, Sam James wrote:
>> GCC 15 introduces a new warning -Wunterminated-string-initialization
>> which causes, with the kernel's -Werror=..., the following:
>> ```
>> /var/tmp/portage/sys-kernel/gentoo-kernel-6.6.41/work/linux-6.6/fs/btrfs/print-tree.c:29:49: error: initializer-string for array of ‘char’ is too long [-Werror=unterminated-string-initialization]
>>    29 |         { BTRFS_BLOCK_GROUP_TREE_OBJECTID,      "BLOCK_GROUP_TREE"      },
>>       |
>>       ^~~~~~~~~~~~~~~~~~
>> ```
>> 
>> It was introduced in https://gcc.gnu.org/PR115185. I don't have time
>> today to check the case to see what the best fix is, but CCing Alex who
>> wrote the warning implementation in case he has a chance.
>
> Thanks for forwarding the report.  It looks like a legit diagnostic.  It
> seems like a bug.

Thank you for analysing it so quickly! I normally try to for bug reports
but I'd already hit an unrelated kernel issue I was debugging so I
didn't want to worry about it for now.

> [...]

> The fix would be to add at least one byte to that array size.  Possibly
> make it 32 for alignment.  But I don't know if that array size is fixed
> by any ABI, so the maintainer will be better placed to find the suitable
> fix.
>
> The only alternatives I see are
>
> -  Use a larger number of elements for that array (1 would be enough).
> -  Use a shorter string so that it fits the 16 bytes.
>
> Have a lovely night!

You too :)

It might be worth making a list of "real bugs the warning found" as
stuff gets ported over the next few months.

> Alex

thanks,
sam

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 377 bytes --]

Re: Build failure with trunk GCC 15 in fs/btrfs/print-tree.c (-Wunterminated-string-initialization)

[Alejandro Colomar]

[-- Attachment #1: Type: text/plain, Size: 2077 bytes --]

Hi Sam,

On Fri, Jul 19, 2024 at 07:08:55AM GMT, Sam James wrote:
> Alejandro Colomar <alx@kernel.org> writes:
> 
> > Hi Sam!
> >
> > On Thu, Jul 18, 2024 at 10:54:40PM GMT, Sam James wrote:
> >> GCC 15 introduces a new warning -Wunterminated-string-initialization
> >> which causes, with the kernel's -Werror=..., the following:
> >> ```
> >> /var/tmp/portage/sys-kernel/gentoo-kernel-6.6.41/work/linux-6.6/fs/btrfs/print-tree.c:29:49: error: initializer-string for array of ‘char’ is too long [-Werror=unterminated-string-initialization]
> >>    29 |         { BTRFS_BLOCK_GROUP_TREE_OBJECTID,      "BLOCK_GROUP_TREE"      },
> >>       |
> >>       ^~~~~~~~~~~~~~~~~~
> >> ```
> >> 
> >> It was introduced in https://gcc.gnu.org/PR115185. I don't have time
> >> today to check the case to see what the best fix is, but CCing Alex who
> >> wrote the warning implementation in case he has a chance.
> >
> > Thanks for forwarding the report.  It looks like a legit diagnostic.  It
> > seems like a bug.
> 
> Thank you for analysing it so quickly!

grepc(1) makes it easy.  :-)

> I normally try to for bug reports
> but I'd already hit an unrelated kernel issue I was debugging so I
> didn't want to worry about it for now.
> 
> > [...]
> 
> > The fix would be to add at least one byte to that array size.  Possibly
> > make it 32 for alignment.  But I don't know if that array size is fixed
> > by any ABI, so the maintainer will be better placed to find the suitable
> > fix.
> >
> > The only alternatives I see are
> >
> > -  Use a larger number of elements for that array (1 would be enough).
> > -  Use a shorter string so that it fits the 16 bytes.
> >
> > Have a lovely night!
> 
> You too :)

Thanks :)

> It might be worth making a list of "real bugs the warning found" as
> stuff gets ported over the next few months.

Hmmm, sounds good.  I'll try to build a list of related links and
commits.

Have a lovely day!
Alex

> 
> > Alex
> 
> thanks,
> sam



-- 
<https://www.alejandro-colomar.es/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]