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

[Peter Maydell <peter.maydell@linaro.org>]

On 20 January 2013 15:54, Blue Swirl <blauwirbel@gmail.com> 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.

-- PMM