Re: [Qemu-devel] [PATCH] memory: Set notdirty_mem_ops validator

[Eric Blake <eblake@redhat.com>]

On 9/6/19 8:24 AM, Philippe Mathieu-Daudé wrote:

>>>>  static const MemoryRegionOps notdirty_mem_ops = {
>>>>      .write = notdirty_mem_write,
>>>> -    .valid.accepts = notdirty_mem_accepts,
>>>>      .endianness = DEVICE_NATIVE_ENDIAN,
>>>>      .valid = {
>>>>          .min_access_size = 1,
>>>>          .max_access_size = 8,
>>>>          .unaligned = false,
>>>> +        .accepts = notdirty_mem_accepts,
>>>
>>> I'm surprised the compiler doesn't emit any warning...
>>
>> Same here.
>>
>> But reading
>> https://en.cppreference.com/w/c/language/struct_initialization, this is
>> compliant behavior:
>>
>> "However, when an initializer begins with a left open brace, its current
>> object is fully re-initialized and any prior explicit initializers for
>> any of its subobjects are ignored:"
>>
>> so it is worth filing a gcc bug asking for a QoI improvement in adding a
>> warning (since the code does not violate the C standard, but does cause
>> surprises in the reinitialization of omitted members in the later {} to
>> go back to 0 in spite of the earlier initialization by nested name).
> 
> Just remembered another case of (correct) reinitialization in
> hw/arm/palm.c:101:

We use nested reinitialization in multiple places. A gcc warning would
have to be discriminating enough to NOT warn merely when something is
listed twice...:

> 
> static struct {
>     int row;
>     int column;
> } palmte_keymap[0x80] = {
>     [0 ... 0x7f] = { -1, -1 },
>     [0x3b] = { 0, 0 },	/* F1	-> Calendar */

Here, [0x3b].row and [0x3b].column are listed twice, but both of the
second listings are explicit.

Similarly, in qobject/json-lexer.c:

static const uint8_t json_lexer[][256] =  {
    /* Relies on default initialization to IN_ERROR! */
...

    /*
     * Two start states:
     * - IN_START recognizes JSON tokens with our string extensions
     * - IN_START_INTERP additionally recognizes interpolation.
     */
    [IN_START ... IN_START_INTERP] = {
        ['"'] = IN_DQ_STRING,
...
        ['\n'] = IN_START,
    },
    [IN_START_INTERP]['%'] = IN_INTERP,
};

Done that way on purpose (I actually remember scratching my head on the
best way to compress things while reviewing Markus' patch that ended up
as 2cbd15aa6f; it took me a couple of tries off-list to end up at that
override).

...rather, the gcc warning that I envision should ONLY be when a later
partial ={} causes a zero-initialization override of an earlier explicit
.member, and NOT when a later complete ={} or explicit member overrides
an earlier init (whether the earlier one was explicit due to .member or
implicit due to partial ={}).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org