| Message ID | 20190809020137.27334-2-marmarek@invisiblethingslab.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] python: fix -Wsign-compare warnings | expand |
On Fri, Aug 09, 2019 at 04:01:37AM +0200, Marek Marczykowski-Górecki wrote: > GCC9 complains about "%.12s" format with an argument having exactly 12 > bytes and no terminating null character. This is intentional > construct, so disable the warning. IIRC this is deemed as a gcc bug, albeit I'm not sure how we are supposed to workaround it: https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg01712.html Disabling the check wholesale seems like a big hammer. Thanks, Roger.
On 09.08.2019 09:51, Roger Pau Monné wrote: > On Fri, Aug 09, 2019 at 04:01:37AM +0200, Marek Marczykowski-Górecki wrote: >> GCC9 complains about "%.12s" format with an argument having exactly 12 >> bytes and no terminating null character. This is intentional >> construct, so disable the warning. > > IIRC this is deemed as a gcc bug, albeit I'm not sure how we are > supposed to workaround it: > > https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg01712.html > > Disabling the check wholesale seems like a big hammer. Indeed. I had tried to observe this with a simple small example source already, and failed. I can't tell whether that's because I've tried with an almost plain upstream 9.1.0 (and others have some extra patches in it), or whether that's because there's more to it than just the array size and format specifier interaction. Therefore it would help if someone who can actually see the issue would be able to strip down the full test source here to a simple reproducer. Depending on whether _that_ then also fails with plain upstream 9.1.0 the bug would want to be reported there or to the respective distro(s). Jan
Marek Marczykowski-Górecki writes ("[PATCH 2/2] tools/tests/cpu-policy: disable -Wformat-overflow"):
> GCC9 complains about "%.12s" format with an argument having exactly 12
> bytes and no terminating null character. This is intentional
> construct, so disable the warning.
Is there some good reason to have things done this way ?
ISTM that a nicer fix would be to change 12 to 13, earlier.
That way we wouldn't lose justified -Wformat-overflow output.
I appreciate this is only a test program so I'm bikeshedding rather.
As an aside, I hope this code is compiled with -fno-strict-aliasing,
because otherwise it's definitely type-punning in a UB way.
Thanks,
Ian.
On 09.08.2019 12:41, Ian Jackson wrote: > Marek Marczykowski-Górecki writes ("[PATCH 2/2] tools/tests/cpu-policy: disable -Wformat-overflow"): >> GCC9 complains about "%.12s" format with an argument having exactly 12 >> bytes and no terminating null character. This is intentional >> construct, so disable the warning. > > Is there some good reason to have things done this way ? > ISTM that a nicer fix would be to change 12 to 13, earlier. > That way we wouldn't lose justified -Wformat-overflow output. Would you mind clarifying which 12 you mean to change to 13? The one in "%.12s" would, if changed and afaict, then legitimately trigger the warning. And we've already objected to the array to get grown. Jan
Jan Beulich writes ("Re: [Xen-devel] [PATCH 2/2] tools/tests/cpu-policy: disable -Wformat-overflow"):
> Would you mind clarifying which 12 you mean to change to 13?
> The one in "%.12s" would, if changed and afaict, then
> legitimately trigger the warning. And we've already objected
> to the array to get grown.
I meant the array. I missed that objection. I just went and read the
thread
tests/cpu-policy: fix format-overflow warning by null terminating strings
and it did conclude that the compiler was wrong to complain.
But for me it doesn't follow that the original code is necessarily the
best way of doing things, and I didn't see anyone giving an argument
why simply increasing the array was a bad idea.
C "prefers" null-terminated strings in that they work somewhat better
with a variety of primitives.
Ian.
On 09.08.2019 13:05, Ian Jackson wrote: > Jan Beulich writes ("Re: [Xen-devel] [PATCH 2/2] tools/tests/cpu-policy: disable -Wformat-overflow"): >> Would you mind clarifying which 12 you mean to change to 13? >> The one in "%.12s" would, if changed and afaict, then >> legitimately trigger the warning. And we've already objected >> to the array to get grown. > > I meant the array. I missed that objection. I just went and read the > thread > tests/cpu-policy: fix format-overflow warning by null terminating strings > and it did conclude that the compiler was wrong to complain. > > But for me it doesn't follow that the original code is necessarily the > best way of doing things, and I didn't see anyone giving an argument > why simply increasing the array was a bad idea. > > C "prefers" null-terminated strings in that they work somewhat better > with a variety of primitives. Right, but the %.<num>s specification exits precisely to allow to deal with potentially not nul-terminated strings. ACPI code in the hypervisor makes quite a bit of use of this, for example, without triggering any compiler warnings with 9.1.0. Jan
On 09/08/2019 12:34, Jan Beulich wrote: > On 09.08.2019 13:05, Ian Jackson wrote: >> Jan Beulich writes ("Re: [Xen-devel] [PATCH 2/2] >> tools/tests/cpu-policy: disable -Wformat-overflow"): >>> Would you mind clarifying which 12 you mean to change to 13? >>> The one in "%.12s" would, if changed and afaict, then >>> legitimately trigger the warning. And we've already objected >>> to the array to get grown. >> >> I meant the array. I missed that objection. I just went and read the >> thread >> tests/cpu-policy: fix format-overflow warning by null terminating >> strings >> and it did conclude that the compiler was wrong to complain. >> >> But for me it doesn't follow that the original code is necessarily the >> best way of doing things, and I didn't see anyone giving an argument >> why simply increasing the array was a bad idea. >> >> C "prefers" null-terminated strings in that they work somewhat better >> with a variety of primitives. > > Right, but the %.<num>s specification exits precisely to allow > to deal with potentially not nul-terminated strings. ACPI code > in the hypervisor makes quite a bit of use of this, for example, > without triggering any compiler warnings with 9.1.0. I also wonder whether https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html#index-nonstring-variable-attribute might be a way of fixing this, seeing as it exists specifically for the purpose. ~Andrew
diff --git a/tools/tests/cpu-policy/Makefile b/tools/tests/cpu-policy/Makefile index 07dd58f5c2..7d17a4074d 100644 --- a/tools/tests/cpu-policy/Makefile +++ b/tools/tests/cpu-policy/Makefile @@ -30,7 +30,7 @@ install: all .PHONY: uninstall -CFLAGS += -Werror $(CFLAGS_xeninclude) -D__XEN_TOOLS__ -O3 +CFLAGS += -Werror -Wno-format-overflow $(CFLAGS_xeninclude) -D__XEN_TOOLS__ -O3 CFLAGS += $(APPEND_CFLAGS) vpath %.c ../../../xen/lib/x86
GCC9 complains about "%.12s" format with an argument having exactly 12 bytes and no terminating null character. This is intentional construct, so disable the warning. Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- tools/tests/cpu-policy/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)