diff mbox series

[2/2] tools/tests/cpu-policy: disable -Wformat-overflow

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

Commit Message

Marek Marczykowski-Górecki Aug. 9, 2019, 2:01 a.m. UTC
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(-)

Comments

Roger Pau Monné Aug. 9, 2019, 7:51 a.m. UTC | #1
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.
Jan Beulich Aug. 9, 2019, 8:26 a.m. UTC | #2
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
Ian Jackson Aug. 9, 2019, 10:41 a.m. UTC | #3
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.
Jan Beulich Aug. 9, 2019, 10:50 a.m. UTC | #4
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
Ian Jackson Aug. 9, 2019, 11:05 a.m. UTC | #5
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.
Jan Beulich Aug. 9, 2019, 11:34 a.m. UTC | #6
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
Andrew Cooper Aug. 9, 2019, 12:46 p.m. UTC | #7
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 mbox series

Patch

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