RE: [PATCH] ACPICA: fix -Wfallthrough

From: Moore, Robert
Date: Thu Nov 12 2020 - 16:48:07 EST

Next message: Tushar Sugandhi: "Re: [PATCH v5 2/7] IMA: update process_buffer_measurement to measure buffer hash"
Previous message: Tushar Sugandhi: "Re: [PATCH v5 2/7] IMA: update process_buffer_measurement to measure buffer hash"
In reply to: Joe Perches: "Re: [PATCH] ACPICA: fix -Wfallthrough"
Next in thread: Nick Desaulniers: "Re: [PATCH] ACPICA: fix -Wfallthrough"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

-----Original Message-----
From: Nick Desaulniers <ndesaulniers@xxxxxxxxxx>
Sent: Thursday, November 12, 2020 11:31 AM
To: Moore, Robert <robert.moore@xxxxxxxxx>
Cc: Kaneda, Erik <erik.kaneda@xxxxxxxxx>; Wysocki, Rafael J <rafael.j.wysocki@xxxxxxxxx>; Gustavo A . R . Silva <gustavoars@xxxxxxxxxx>; clang-built-linux@xxxxxxxxxxxxxxxx; Len Brown <lenb@xxxxxxxxxx>; linux-acpi@xxxxxxxxxxxxxxx; devel@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
Subject: Re: [PATCH] ACPICA: fix -Wfallthrough

On Thu, Nov 12, 2020 at 7:13 AM Moore, Robert <robert.moore@xxxxxxxxx> wrote:
>
>
>
> -----Original Message-----
> From: Nick Desaulniers <ndesaulniers@xxxxxxxxxx>
> Sent: Wednesday, November 11, 2020 10:48 AM
> To: Moore, Robert <robert.moore@xxxxxxxxx>
> Cc: Kaneda, Erik <erik.kaneda@xxxxxxxxx>; Wysocki, Rafael J
> <rafael.j.wysocki@xxxxxxxxx>; Gustavo A . R . Silva
> <gustavoars@xxxxxxxxxx>; clang-built-linux@xxxxxxxxxxxxxxxx; Len Brown
> <lenb@xxxxxxxxxx>; linux-acpi@xxxxxxxxxxxxxxx; devel@xxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH] ACPICA: fix -Wfallthrough
>
> On Wed, Nov 11, 2020 at 7:15 AM Moore, Robert <robert.moore@xxxxxxxxx> wrote:
> >
> > Yes, but: isn't the "fallthrough" keyword compiler-specific? That is the problem for us.
>
> It's not a keyword.
>
> It's a preprocessor macro that expands to
> __attribute__((__fallthrough__)) for compilers that support it. For compilers that do not, it expands to nothing. Both GCC 7+ and Clang support this attribute. Which other compilers that support -Wimplicit-fallthrough do you care to support?
>
> We need to support MSVC 2017 -- which apparently does not support this.

In which case, the macro is not expanded to a compiler attribute the compiler doesn't support. Please see also its definition in include/linux/compiler_attributes.h.

From what I can tell, MSVC does not warn on implicit fallthrough, so there's no corresponding attribute (or comment) to disable the warning in MSVC.

That doesn't mean this code is not portable to MSVC; a macro that expands to nothing should not be a problem.

Based on
https://docs.microsoft.com/en-us/cpp/code-quality/c26819?view=msvc-160
https://developercommunity.visualstudio.com/idea/423975/issue-compiler-warning-when-using-implicit-fallthr.html
it sounds like MSVC 2019 will be able to warn, for C++ mode, which will rely on the C++ style attribute to annotate intentional fallthrough.

Can you confirm how this does not work for MSVC 2017?

1>c:\acpica\source\components\utilities\utdelete.c(270): warning C4013: '__attribute__' undefined; assuming extern returning int
1>c:\acpica\source\components\utilities\utdelete.c(270): error C2065: '__fallthrough__': undeclared identifier
1>c:\acpica\source\components\utilities\utdelete.c(272): error C2143: syntax error: missing ';' before 'case'

> > Bob
> >
> >
> > -----Original Message-----
> > From: ndesaulniers via sendgmr
> > <ndesaulniers@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx> On Behalf Of Nick
> > Desaulniers
> > Sent: Tuesday, November 10, 2020 6:12 PM
> > To: Moore, Robert <robert.moore@xxxxxxxxx>; Kaneda, Erik
> > <erik.kaneda@xxxxxxxxx>; Wysocki, Rafael J
> > <rafael.j.wysocki@xxxxxxxxx>; Gustavo A . R . Silva
> > <gustavoars@xxxxxxxxxx>
> > Cc: clang-built-linux@xxxxxxxxxxxxxxxx; Nick Desaulniers
> > <ndesaulniers@xxxxxxxxxx>; Len Brown <lenb@xxxxxxxxxx>;
> > linux-acpi@xxxxxxxxxxxxxxx; devel@xxxxxxxxxx;
> > linux-kernel@xxxxxxxxxxxxxxx
> > Subject: [PATCH] ACPICA: fix -Wfallthrough
> >
> > The "fallthrough" pseudo-keyword was added as a portable way to denote intentional fallthrough. This code seemed to be using a mix of fallthrough comments that GCC recognizes, and some kind of lint marker.
> > I'm guessing that linter hasn't been run in a while from the mixed use of the marker vs comments.
> >
> > Signed-off-by: Nick Desaulniers <ndesaulniers@xxxxxxxxxx>
> > ---
> > drivers/acpi/acpica/dscontrol.c | 3 +--
> > drivers/acpi/acpica/dswexec.c | 4 +---
> > drivers/acpi/acpica/dswload.c | 3 +--
> > drivers/acpi/acpica/dswload2.c | 3 +--
> > drivers/acpi/acpica/exfldio.c | 3 +--
> > drivers/acpi/acpica/exresop.c | 5 ++---
> > drivers/acpi/acpica/exstore.c | 6 ++----
> > drivers/acpi/acpica/hwgpe.c | 3 +--
> > drivers/acpi/acpica/utdelete.c | 3 +--
> > drivers/acpi/acpica/utprint.c | 2 +-
> > 10 files changed, 12 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/acpi/acpica/dscontrol.c
> > b/drivers/acpi/acpica/dscontrol.c index 4b5b6e859f62..1e75e5fbfd19
> > 100644
> > --- a/drivers/acpi/acpica/dscontrol.c
> > +++ b/drivers/acpi/acpica/dscontrol.c
> > @@ -61,8 +61,7 @@ acpi_ds_exec_begin_control_op(struct acpi_walk_state *walk_state,
> > break;
> > }
> > }
> > -
> > - /*lint -fallthrough */
> > + fallthrough;
> >
> > case AML_IF_OP:
> > /*
> > diff --git a/drivers/acpi/acpica/dswexec.c
> > b/drivers/acpi/acpica/dswexec.c index 1d4f8c81028c..e8c32d4fe55f
> > 100644
> > --- a/drivers/acpi/acpica/dswexec.c
> > +++ b/drivers/acpi/acpica/dswexec.c
> > @@ -597,9 +597,7 @@ acpi_status acpi_ds_exec_end_op(struct acpi_walk_state *walk_state)
> > if (ACPI_FAILURE(status)) {
> > break;
> > }
> > -
> > - /* Fall through */
> > - /*lint -fallthrough */
> > + fallthrough;
> >
> > case AML_INT_EVAL_SUBTREE_OP:
> >
> > diff --git a/drivers/acpi/acpica/dswload.c
> > b/drivers/acpi/acpica/dswload.c index 27069325b6de..afc663c3742d
> > 100644
> > --- a/drivers/acpi/acpica/dswload.c
> > +++ b/drivers/acpi/acpica/dswload.c
> > @@ -223,8 +223,7 @@ acpi_ds_load1_begin_op(struct acpi_walk_state *walk_state,
> > parse_flags & ACPI_PARSE_MODULE_LEVEL)) {
> > break;
> > }
> > -
> > - /*lint -fallthrough */
> > + fallthrough;
> >
> > default:
> >
> > diff --git a/drivers/acpi/acpica/dswload2.c
> > b/drivers/acpi/acpica/dswload2.c index edadbe146506..1b794b6ba072
> > 100644
> > --- a/drivers/acpi/acpica/dswload2.c
> > +++ b/drivers/acpi/acpica/dswload2.c
> > @@ -213,8 +213,7 @@ acpi_ds_load2_begin_op(struct acpi_walk_state *walk_state,
> > parse_flags & ACPI_PARSE_MODULE_LEVEL)) {
> > break;
> > }
> > -
> > - /*lint -fallthrough */
> > + fallthrough;
> >
> > default:
> >
> > diff --git a/drivers/acpi/acpica/exfldio.c
> > b/drivers/acpi/acpica/exfldio.c index ade35ff1c7ba..9d1cabe0fed9
> > 100644
> > --- a/drivers/acpi/acpica/exfldio.c
> > +++ b/drivers/acpi/acpica/exfldio.c
> > @@ -433,8 +433,7 @@ acpi_ex_field_datum_io(union acpi_operand_object *obj_desc,
> > * Now that the Bank has been selected, fall through to the
> > * region_field case and write the datum to the Operation Region
> > */
> > -
> > - /*lint -fallthrough */
> > + fallthrough;
> >
> > case ACPI_TYPE_LOCAL_REGION_FIELD:
> > /*
> > diff --git a/drivers/acpi/acpica/exresop.c
> > b/drivers/acpi/acpica/exresop.c index 4d1b22971d58..df48faa9a551
> > 100644
> > --- a/drivers/acpi/acpica/exresop.c
> > +++ b/drivers/acpi/acpica/exresop.c
> > @@ -197,8 +197,7 @@ acpi_ex_resolve_operands(u16 opcode,
> > case ACPI_REFCLASS_DEBUG:
> >
> > target_op = AML_DEBUG_OP;
> > -
> > - /*lint -fallthrough */
> > + fallthrough;
> >
> > case ACPI_REFCLASS_ARG:
> > case ACPI_REFCLASS_LOCAL:
> > @@ -264,7 +263,7 @@ acpi_ex_resolve_operands(u16 opcode,
> > * Else not a string - fall through to the normal Reference
> > * case below
> > */
> > - /*lint -fallthrough */
> > + fallthrough;
> >
> > case ARGI_REFERENCE: /* References: */
> > case ARGI_INTEGER_REF:
> > diff --git a/drivers/acpi/acpica/exstore.c
> > b/drivers/acpi/acpica/exstore.c index 3adc0a29d890..2067baa7c120
> > 100644
> > --- a/drivers/acpi/acpica/exstore.c
> > +++ b/drivers/acpi/acpica/exstore.c
> > @@ -95,8 +95,7 @@ acpi_ex_store(union acpi_operand_object *source_desc,
> > if (dest_desc->common.flags & AOPOBJ_AML_CONSTANT) {
> > return_ACPI_STATUS(AE_OK);
> > }
> > -
> > - /*lint -fallthrough */
> > + fallthrough;
> >
> > default:
> >
> > @@ -421,8 +420,7 @@ acpi_ex_store_object_to_node(union acpi_operand_object *source_desc,
> > }
> > break;
> > }
> > -
> > - /* Fallthrough */
> > + fallthrough;
> >
> > case ACPI_TYPE_DEVICE:
> > case ACPI_TYPE_EVENT:
> > diff --git a/drivers/acpi/acpica/hwgpe.c
> > b/drivers/acpi/acpica/hwgpe.c index b13a4ed5bc63..fbfad80c8a53
> > 100644
> > --- a/drivers/acpi/acpica/hwgpe.c
> > +++ b/drivers/acpi/acpica/hwgpe.c
> > @@ -166,8 +166,7 @@ acpi_hw_low_set_gpe(struct acpi_gpe_event_info *gpe_event_info, u32 action)
> > if (!(register_bit & gpe_register_info->enable_mask)) {
> > return (AE_BAD_PARAMETER);
> > }
> > -
> > - /*lint -fallthrough */
> > + fallthrough;
> >
> > case ACPI_GPE_ENABLE:
> >
> > diff --git a/drivers/acpi/acpica/utdelete.c
> > b/drivers/acpi/acpica/utdelete.c index 4c0d4e434196..8076e7947585
> > 100644
> > --- a/drivers/acpi/acpica/utdelete.c
> > +++ b/drivers/acpi/acpica/utdelete.c
> > @@ -111,8 +111,7 @@ static void acpi_ut_delete_internal_obj(union acpi_operand_object *object)
> > (void)acpi_ev_delete_gpe_block(object->device.
> > gpe_block);
> > }
> > -
> > - /*lint -fallthrough */
> > + fallthrough;
> >
> > case ACPI_TYPE_PROCESSOR:
> > case ACPI_TYPE_THERMAL:
> > diff --git a/drivers/acpi/acpica/utprint.c
> > b/drivers/acpi/acpica/utprint.c index 681c11f4af4e..f7e43baf5ff2
> > 100644
> > --- a/drivers/acpi/acpica/utprint.c
> > +++ b/drivers/acpi/acpica/utprint.c
> > @@ -475,7 +475,7 @@ int vsnprintf(char *string, acpi_size size, const char *format, va_list args)
> > case 'X':
> >
> > type |= ACPI_FORMAT_UPPER;
> > - /* FALLTHROUGH */
> > + fallthrough;
> >
> > case 'x':
> >
> > --
> > 2.29.2.222.g5d2a92d10f8-goog
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers

--
Thanks,
~Nick Desaulniers

Next message: Tushar Sugandhi: "Re: [PATCH v5 2/7] IMA: update process_buffer_measurement to measure buffer hash"
Previous message: Tushar Sugandhi: "Re: [PATCH v5 2/7] IMA: update process_buffer_measurement to measure buffer hash"
In reply to: Joe Perches: "Re: [PATCH] ACPICA: fix -Wfallthrough"
Next in thread: Nick Desaulniers: "Re: [PATCH] ACPICA: fix -Wfallthrough"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]