strict aliasing in kvm-unit-tests (was: Re: [kvm-unit-tests PATCH v8 6/6] s390x: SCLP unit test)

[Thomas Huth <thuth@redhat.com>]

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?

By the way, when compiling the x86 code with -O2 instead of -O1, I also get:

lib/x86/intel-iommu.c: In function ‘vtd_setup_msi’:
lib/x86/intel-iommu.c:324:4: error: dereferencing type-punned pointer
will break strict-aliasing rules [-Werror=strict-aliasing]
   *(uint64_t *)&msi_addr, *(uint32_t *)&msi_data);
    ^~~~~~~~~~~~~~~~~~~~~
lib/x86/intel-iommu.c:324:28: error: dereferencing type-punned pointer
will break strict-aliasing rules [-Werror=strict-aliasing]
   *(uint64_t *)&msi_addr, *(uint32_t *)&msi_data);
                            ^~~~~~~~~~~~~~~~~~~~~
lib/x86/intel-iommu.c:326:29: error: dereferencing type-punned pointer
will break strict-aliasing rules [-Werror=strict-aliasing]
  return pci_setup_msi(dev, *(uint64_t *)&msi_addr,
                             ^~~~~~~~~~~~~~~~~~~~~
lib/x86/intel-iommu.c:327:10: error: dereferencing type-punned pointer
will break strict-aliasing rules [-Werror=strict-aliasing]
         *(uint32_t *)&msi_data);
          ^~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

... so maybe -fno-strict-aliasing would be a good idea for that, too?

 Thomas