Re: [Qemu-devel] [PATCH] Annotate questionable fallthroughs

[Blue Swirl]

On Sun, Jan 20, 2013 at 4:56 PM, Peter Maydell <address@hidden> wrote:
> On 20 January 2013 15:54, Blue Swirl <address@hidden> wrote:
>
> This patch is a bit big to usefully review. A few comments on bits
> I happened to notice:
>
>> diff --git a/hw/arm_sysctl.c b/hw/arm_sysctl.c
>> index a196fcc..2066ef3 100644
>> --- a/hw/arm_sysctl.c
>> +++ b/hw/arm_sysctl.c
>> @@ -199,6 +199,7 @@ static void arm_sysctl_write(void *opaque, hwaddr offset,
>>      switch (offset) {
>>      case 0x08: /* LED */
>>          s->leds = val;
>> +        /* XXX: questionable fallthrough */
>
> Should have its own 'break' but it's safe currently as the following
> case is just 'break' anyway.
>
>>      case 0x0c: /* OSC0 */
>>      case 0x10: /* OSC1 */
>>      case 0x14: /* OSC2 */
>> @@ -295,6 +296,7 @@ static void arm_sysctl_write(void *opaque, hwaddr offset,
>>              /* On VExpress this register is unimplemented and will RAZ/WI */
>>              break;
>>          }
>> +        /* XXX: questionable fallthrough */
>
> Ditto.
>
>>      case 0x54: /* CLCDSER */
>>      case 0x64: /* DMAPSR0 */
>>      case 0x68: /* DMAPSR1 */
>
>> --- a/hw/es1370.c
>> +++ b/hw/es1370.c
>> @@ -537,8 +537,10 @@ IO_WRITE_PROTO (es1370_writew)
>>
>>      case ES1370_REG_ADC_SCOUNT:
>>          d++;
>> +        /* XXX: questionable fallthrough */
>>      case ES1370_REG_DAC2_SCOUNT:
>>          d++;
>> +        /* XXX: questionable fallthrough */
>>      case ES1370_REG_DAC1_SCOUNT:
>>          d->scount = (d->scount & ~0xffff) | (val & 0xffff);
>>          break;
>
> These fallthroughs are clearly intentional (similar cases
> elsewhere in your patch).
>
>> --- a/hw/stellaris.c
>> +++ b/hw/stellaris.c
>> @@ -182,8 +182,10 @@ static uint64_t gptm_read(void *opaque, hwaddr offset,
>>      case 0x48: /* TAR */
>>          if (s->control == 1)
>>              return s->rtc;
>> +        /* XXX: questionable fallthrough */
>>      case 0x4c: /* TBR */
>>          hw_error("TODO: Timer value read\n");
>> +        /* XXX: questionable fallthrough */
>
> This isn't a fallthrough at all, hw_error() never returns.
>
>>      default:
>>          hw_error("gptm_read: Bad offset 0x%x\n", (int)offset);
>>          return 0;
>
> (...so this return 0 is unreachable, but hey.)
>
> I don't think there's much point adding tons of "XXX" comments
> when a bunch of these aren't actually wrong code. If you want to fix
> this I think a better approach would be more focused patches aimed
> at adding 'break;' or "/* fallthrough */" based on actual human
> examination of the surrounding code.

The problem is that while some cases may be easy to decide, others are
not so clear.

My initial thought about the work flow was that this patch should be
succeeded by other patches which replace the comment with correct
action. These could be squashed to the original patch or committed
later. If no decision can be made for some comment, it could stay as
XXX.

Alternatively, I could split this patch per maintainer, architecture
or file even. Each maintainer could tune the patches as they see fit
and commit whatever they want later. Probably some areas would be
never fixed.

>
> -- PMM