From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-1.mimecast.com ([207.211.31.81]:47245 "EHLO us-smtp-delivery-1.mimecast.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1725924AbgAVOmq (ORCPT ); Wed, 22 Jan 2020 09:42:46 -0500 Received: by mail-wr1-f71.google.com with SMTP id w6so3154760wrm.16 for ; Wed, 22 Jan 2020 06:42:43 -0800 (PST) Subject: Re: strict aliasing in kvm-unit-tests (was: Re: [kvm-unit-tests PATCH v8 6/6] s390x: SCLP unit test) References: <20200120184256.188698-1-imbrenda@linux.ibm.com> <20200120184256.188698-7-imbrenda@linux.ibm.com> <35e59971-c09e-2808-1be6-f2ccd555c4f6@redhat.com> <42c5b040-733d-4e5b-0276-5b94315336bb@redhat.com> <997a62b7-7ab7-6119-4948-e8779e639101@redhat.com> <4d09b567-c2ae-ec9d-59d0-bd259a86b14d@redhat.com> <946e1194-4607-c928-6d66-9e306dc1216a@redhat.com> From: Paolo Bonzini Message-ID: Date: Wed, 22 Jan 2020 15:42:38 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-s390-owner@vger.kernel.org List-ID: To: Thomas Huth , David Hildenbrand , Claudio Imbrenda , kvm@vger.kernel.org, Andrew Jones , Laurent Vivier Cc: linux-s390@vger.kernel.org, borntraeger@de.ibm.com, frankja@linux.ibm.com On 22/01/20 15:15, Thomas Huth wrote: > On 22/01/2020 13.16, Thomas Huth wrote: >> On 22/01/2020 11.40, David Hildenbrand wrote: >>> On 22.01.20 11:39, Thomas Huth wrote: >>>> On 22/01/2020 11.32, David Hildenbrand wrote: >>>>> On 22.01.20 11:31, Thomas Huth wrote: >>>>>> On 22/01/2020 11.22, David Hildenbrand wrote: >>>>>>> On 22.01.20 11:10, David Hildenbrand wrote: >> [...] >>>>>>>> Doing a fresh ./configure + make on RHEL7 gives me >>>>>>>> >>>>>>>> [linux1@rhkvm01 kvm-unit-tests]$ make >>>>>>>> gcc -std=gnu99 -ffreestanding -I /home/linux1/git/kvm-unit-tests/lib -I /home/linux1/git/kvm-unit-tests/lib/s390x -I lib -O2 -march=zEC12 -fno-delete-null-pointer-checks -g -MMD -MF s390x/.sclp.d -Wall -Wwrite-strings -Wempty-body -Wuninitialized -Wignored-qualifiers -Werror -fomit-frame-pointer -Wno-frame-address -fno-pic -Wclobbered -Wunused-but-set-parameter -Wmissing-parameter-type -Wold-style-declaration -Woverride-init -Wmissing-prototypes -Wstrict-prototypes -c -o s390x/sclp.o s390x/sclp.c >>>>>>>> s390x/sclp.c: In function 'test_one_simple': >>>>>>>> s390x/sclp.c:121:2: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing] >>>>>>>> ((SCCBHeader *)sccb_template)->length = sccb_len; >>>>>>>> ^ >>>>>>>> s390x/sclp.c: At top level: >>>>>>>> cc1: error: unrecognized command line option "-Wno-frame-address" [-Werror] >>>>>>>> cc1: all warnings being treated as errors >>>>>>>> make: *** [s390x/sclp.o] Error 1 >>>>>>> >>>>>>> The following makes it work: >>>>>>> >>>>>>> >>>>>>> diff --git a/s390x/sclp.c b/s390x/sclp.c >>>>>>> index c13fa60..0b8117a 100644 >>>>>>> --- a/s390x/sclp.c >>>>>>> +++ b/s390x/sclp.c >>>>>>> @@ -117,8 +117,10 @@ static bool test_one_ro(uint32_t cmd, uint8_t *addr, uint64_t exp_pgm, uint16_t >>>>>>> static bool test_one_simple(uint32_t cmd, uint8_t *addr, uint16_t sccb_len, >>>>>>> uint16_t buf_len, uint64_t exp_pgm, uint16_t exp_rc) >>>>>>> { >>>>>>> + SCCBHeader *header = (void *)sccb_template; >>>>>>> + >>>>>>> memset(sccb_template, 0, sizeof(sccb_template)); >>>>>>> - ((SCCBHeader *)sccb_template)->length = sccb_len; >>>>>>> + header->length = sccb_len; >>>>>> >>>>>> While that might silence the compiler warning, we still might get >>>>>> aliasing problems here, I think. >>>>>> The right way to solve this problem is to turn sccb_template into a >>>>>> union of the various structs/arrays that you want to use and then access >>>>>> the fields through the union instead ("type-punning through union"). >>>>> >>>>> We do have the exact same thing in lib/s390x/sclp.c already, no? >>>> >>>> Maybe we should carefully check that code, too... >>>> >>>>> Especially, new compilers don't seem to care? >>>> >>>> I've seen horrible bugs due to these aliasing problems in the past - >>>> without compiler warnings showing up! Certain versions of GCC assume >>>> that they can re-order code with pointers that point to types of >>>> different sizes, i.e. in the above example, I think they could assume >>>> that they could re-order the memset() and the header->length = ... line. >>>> I'd feel better if we play safe and use a union here. >>> >>> Should we simply allow type-punning? >> >> Maybe yes. The kernel also compiles with "-fno-strict-aliasing", and >> since kvm-unit-tests is mainly a "playground" for people who do kernel >> development, too, we should maybe also compile the unit tests with >> "-fno-strict-aliasing". >> >> Paolo, Andrew, Laurent, what do you think? I think enabling -fno-strict-aliasing is a good idea. Paolo