diff mbox

[edk2,RFC] MdePkg: add ARG_UNUSED notation to Base.h

Message ID 1457969921-29870-1-git-send-email-leif.lindholm@linaro.org
State Superseded
Headers show

Commit Message

Leif Lindholm March 14, 2016, 3:38 p.m. UTC 
To permit many existing platforms to build with -Wunused-parameter, on
GCC and CLANG, the unused parameters need to be annotated as such.
Existing regexp code already uses ARG_UNUSED for this, but it is really
needed across the codebase - so add a version in Base.h.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>

---

This is really the result of a friendly toolchain engineer informing me
CLANG has the -Weverything flag, to actually enable all possible
warnings.

One problem trying to pick out the real bugs from the just not entirely
clear code is that basically a lot of *LibNull implementations, and
some libraries that should be usable, trip build failures due to unused
parameters.

 MdePkg/Include/Base.h | 9 +++++++++
 1 file changed, 9 insertions(+)

-- 
2.1.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Laszlo Ersek March 16, 2016, 1:54 p.m. UTC | #1 
On 03/14/16 16:38, Leif Lindholm wrote:
> To permit many existing platforms to build with -Wunused-parameter, on

> GCC and CLANG, the unused parameters need to be annotated as such.

> Existing regexp code already uses ARG_UNUSED for this, but it is really

> needed across the codebase - so add a version in Base.h.

> 

> Contributed-under: TianoCore Contribution Agreement 1.0

> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---

> 

> This is really the result of a friendly toolchain engineer informing me

> CLANG has the -Weverything flag, to actually enable all possible

> warnings.

> 

> One problem trying to pick out the real bugs from the just not entirely

> clear code is that basically a lot of *LibNull implementations, and

> some libraries that should be usable, trip build failures due to unused

> parameters.

> 

>  MdePkg/Include/Base.h | 9 +++++++++

>  1 file changed, 9 insertions(+)

> 

> diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h

> index 89b2aed..f789094 100644

> --- a/MdePkg/Include/Base.h

> +++ b/MdePkg/Include/Base.h

> @@ -189,6 +189,15 @@ struct _LIST_ENTRY {

>  ///

>  #define OPTIONAL

>  

> +///

> +/// Function argument intentionally unused in function.

> +///

> +#if defined(__GNUC__) || defined(__clang__)

> +  #define ARG_UNUSED  __attribute__ ((unused))

> +#else

> +  #define ARG_UNUSED

> +#endif

> +

>  //

>  //  UEFI specification claims 1 and 0. We are concerned about the

>  //  complier portability so we did it this way.

> 


Support for this seems to go back to gcc-4.4:

https://gcc.gnu.org/onlinedocs/gcc-4.4.7/gcc/Variable-Attributes.html

So it looks safe. (And I agree with the idea.)

Acked-by: Laszlo Ersek <lersek@redhat.com>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek March 16, 2016, 5:06 p.m. UTC | #2 
On 03/16/16 17:50, Andrew Fish wrote:
> 

>> On Mar 16, 2016, at 6:54 AM, Laszlo Ersek <lersek@redhat.com> wrote:

>>

>> On 03/14/16 16:38, Leif Lindholm wrote:

>>> To permit many existing platforms to build with -Wunused-parameter, on

>>> GCC and CLANG, the unused parameters need to be annotated as such.

>>> Existing regexp code already uses ARG_UNUSED for this, but it is really

>>> needed across the codebase - so add a version in Base.h.

>>>

>>> Contributed-under: TianoCore Contribution Agreement 1.0

>>> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>

>>> ---

>>>

>>> This is really the result of a friendly toolchain engineer informing me

>>> CLANG has the -Weverything flag, to actually enable all possible

>>> warnings.

>>>

>>> One problem trying to pick out the real bugs from the just not entirely

>>> clear code is that basically a lot of *LibNull implementations, and

>>> some libraries that should be usable, trip build failures due to unused

>>> parameters.

>>>

>>> MdePkg/Include/Base.h | 9 +++++++++

>>> 1 file changed, 9 insertions(+)

>>>

>>> diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h

>>> index 89b2aed..f789094 100644

>>> --- a/MdePkg/Include/Base.h

>>> +++ b/MdePkg/Include/Base.h

>>> @@ -189,6 +189,15 @@ struct _LIST_ENTRY {

>>> ///

>>> #define OPTIONAL

>>>

>>> +///

>>> +/// Function argument intentionally unused in function.

>>> +///

>>> +#if defined(__GNUC__) || defined(__clang__)

>>> +  #define ARG_UNUSED  __attribute__ ((unused))

>>> +#else

>>> +  #define ARG_UNUSED

>>> +#endif

>>> +

>>> //

>>> //  UEFI specification claims 1 and 0. We are concerned about the

>>> //  complier portability so we did it this way.

>>>

>>

>> Support for this seems to go back to gcc-4.4:

>>

>> https://gcc.gnu.org/onlinedocs/gcc-4.4.7/gcc/Variable-Attributes.html

>>

>> So it looks safe. (And I agree with the idea.)

>>

> 

> I'm confused by the usage, or really when the compiler detects the

> warning?

> 

> For example how is this going to work on public interfaces?

> Protocols, PPIs,  or library class definitions? There is a single

> definition of the function, but multiple implementations. Some

> implementations may not use some arguments. But what arguments get

> used seems to be an implementation choice and not part of the API?

> Thus it seems like this macro is not going to enable changing

> compiler flags?


I think this attribute would be used in library instances and protocol
implementations. It would not be used in library class headers nor
protocol definitions.

Example:

    /* included from library class hdr of protocol def hdr */
    int f(int x);

    /* code in library instance or protocol impl */
    int f(int x __attribute__ ((unused)))
    {
      return 0;
    }

    int main(void)
    {
      return f(-2);
    }

Compiles silently with

$ gcc -Wall -Wextra -ansi -pedantic -Werror -o x x.c

If I remove __attribute__ ((unused)), I get:

x.c:5:15: error: unused parameter 'x' [-Werror=unused-parameter]
     int f(int x)
               ^
cc1: all warnings being treated as errors

Thanks
Laszlo

> 

> Or am I just confused?

> 

> Thanks,

> 

> Andrew Fish

>  

> 

>> Acked-by: Laszlo Ersek <lersek@redhat.com>

>> _______________________________________________

>> edk2-devel mailing list

>> edk2-devel@lists.01.org

>> https://lists.01.org/mailman/listinfo/edk2-devel

> 


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Leif Lindholm March 17, 2016, 10:11 a.m. UTC | #3 
On Wed, Mar 16, 2016 at 07:06:12PM -0700, Andrew Fish wrote:
> 

> > On Mar 16, 2016, at 7:02 PM, Gao, Liming <liming.gao@intel.com> wrote:

> > 

> > Andrew:

> >   If we enable -Wunused-parameter option for GCC and CLANG, it will bring the big change to edk2 project. This patch is just to add ARG_UNUSED notation to Base.h. It has no impact. So, I think this patch is fine. 

> >  

> 

> Liming,

> 

> I don't really have an issue adding it for source compatibility with other projects.

> 

> The comments from Leif imply an across the codebase change? I was

> trying to point out what a large undertaking that would be.


Well, it doesn't have to be an all in one go type thing.
As long as the modifier exists, it can be added where people come
across problems when building with lots of warnings enabled.

But a follow-up question (for everyone) - is ARG_UNUSED the keyword to
use, or should it just be UNUSED?

Regards,

Leif

> > >>> On 03/14/16 16:38, Leif Lindholm wrote:

> > >>>> To permit many existing platforms to build with -Wunused-parameter, on

> > >>>> GCC and CLANG, the unused parameters need to be annotated as such.

> > >>>> Existing regexp code already uses ARG_UNUSED for this, but it is really

> > >>>> needed across the codebase - so add a version in Base.h.

> 

> 

> Thanks,

> 

> Andrew Fish

> 

> 

> > Thanks

> > Liming <>

> >  <>From: afish@apple.com <mailto:afish@apple.com> [mailto:afish@apple.com <mailto:afish@apple.com>] 

> > Sent: Thursday, March 17, 2016 1:12 AM

> > To: Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>

> > Cc: Kinney, Michael D <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>>; edk2-devel@ml01.01.org <mailto:edk2-devel@ml01.01.org>; Gao, Liming <liming.gao@intel.com <mailto:liming.gao@intel.com>>; Leif Lindholm <leif.lindholm@linaro.org <mailto:leif.lindholm@linaro.org>>

> > Subject: Re: [edk2] [RFC] MdePkg: add ARG_UNUSED notation to Base.h

> >  

> > 

> > > On Mar 16, 2016, at 10:06 AM, Laszlo Ersek wrote:

> > > 

> > > On 03/16/16 17:50, Andrew Fish wrote:

> > >> 

> > >>> On Mar 16, 2016, at 6:54 AM, Laszlo Ersek wrote:

> > >>> 

> > >>> On 03/14/16 16:38, Leif Lindholm wrote:

> > >>>> To permit many existing platforms to build with -Wunused-parameter, on

> > >>>> GCC and CLANG, the unused parameters need to be annotated as such.

> > >>>> Existing regexp code already uses ARG_UNUSED for this, but it is really

> > >>>> needed across the codebase - so add a version in Base.h.

> > >>>> 

> > >>>> Contributed-under: TianoCore Contribution Agreement 1.0

> > >>>> Signed-off-by: Leif Lindholm 

> > >>>> ---

> > >>>> 

> > >>>> This is really the result of a friendly toolchain engineer informing me

> > >>>> CLANG has the -Weverything flag, to actually enable all possible

> > >>>> warnings.

> > >>>> 

> > >>>> One problem trying to pick out the real bugs from the just not entirely

> > >>>> clear code is that basically a lot of *LibNull implementations, and

> > >>>> some libraries that should be usable, trip build failures due to unused

> > >>>> parameters.

> > >>>> 

> > >>>> MdePkg/Include/Base.h | 9 +++++++++

> > >>>> 1 file changed, 9 insertions(+)

> > >>>> 

> > >>>> diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h

> > >>>> index 89b2aed..f789094 100644

> > >>>> --- a/MdePkg/Include/Base.h

> > >>>> +++ b/MdePkg/Include/Base.h

> > >>>> @@ -189,6 +189,15 @@ struct _LIST_ENTRY {

> > >>>> ///

> > >>>> #define OPTIONAL

> > >>>> 

> > >>>> +///

> > >>>> +/// Function argument intentionally unused in function.

> > >>>> +///

> > >>>> +#if defined(__GNUC__) || defined(__clang__)

> > >>>> + #define ARG_UNUSED __attribute__ ((unused))

> > >>>> +#else

> > >>>> + #define ARG_UNUSED

> > >>>> +#endif

> > >>>> +

> > >>>> //

> > >>>> // UEFI specification claims 1 and 0. We are concerned about the

> > >>>> // complier portability so we did it this way.

> > >>>> 

> > >>> 

> > >>> Support for this seems to go back to gcc-4.4:

> > >>> 

> > >>> https://gcc.gnu.org/onlinedocs/gcc-4.4.7/gcc/Variable-Attributes.html <https://gcc.gnu.org/onlinedocs/gcc-4.4.7/gcc/Variable-Attributes.html>

> > >>> 

> > >>> So it looks safe. (And I agree with the idea.)

> > >>> 

> > >> 

> > >> I'm confused by the usage, or really when the compiler detects the

> > >> warning?

> > >> 

> > >> For example how is this going to work on public interfaces?

> > >> Protocols, PPIs, or library class definitions? There is a single

> > >> definition of the function, but multiple implementations. Some

> > >> implementations may not use some arguments. But what arguments get

> > >> used seems to be an implementation choice and not part of the API?

> > >> Thus it seems like this macro is not going to enable changing

> > >> compiler flags?

> > > 

> > > I think this attribute would be used in library instances and protocol

> > > implementations. It would not be used in library class headers nor

> > > protocol definitions.

> > > 

> > > Example:

> > > 

> > > /* included from library class hdr of protocol def hdr */

> > > int f(int x);

> > > 

> > > /* code in library instance or protocol impl */

> > > int f(int x __attribute__ ((unused)))

> > > {

> > > return 0;

> > > }

> > > 

> > > int main(void)

> > > {

> > > return f(-2);

> > > }

> > > 

> > > Compiles silently with

> > > 

> > > $ gcc -Wall -Wextra -ansi -pedantic -Werror -o x x.c

> > > 

> > > If I remove __attribute__ ((unused)), I get:

> > > 

> > > x.c:5:15: error: unused parameter 'x' [-Werror=unused-parameter]

> > > int f(int x)

> > > ^

> > > cc1: all warnings being treated as errors

> > > 

> > 

> > OK that makes sense. That is feels like it is going to be a very very large change to the codebase given almost every protocol and PPI member pass the "This" pointer, but may not use it in code.

> > 

> > Do we need something for VC++?

> > 

> > Thanks,

> > 

> > Andrew Fish

> > 

> > > Thanks

> > > Laszlo

> > > 

> > >> 

> > >> Or am I just confused?

> > >> 

> > >> Thanks,

> > >> 

> > >> Andrew Fish

> > >> 

> > >> 

> > >>> Acked-by: Laszlo Ersek 

> > >>> _______________________________________________

> > >>> edk2-devel mailing list

> > >>> edk2-devel@lists.01.org <mailto:edk2-devel@lists.01.org>

> > >>> https://lists.01.org/mailman/listinfo/edk2-devel <https://lists.01.org/mailman/listinfo/edk2-devel>

> > >> 

> > > 

> > > _______________________________________________

> > > edk2-devel mailing list

> > > edk2-devel@lists.01.org <mailto:edk2-devel@lists.01.org>

> > > https://lists.01.org/mailman/listinfo/edk2-devel <https://lists.01.org/mailman/listinfo/edk2-devel>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek March 17, 2016, 10:37 a.m. UTC | #4 
On 03/17/16 11:11, Leif Lindholm wrote:
> On Wed, Mar 16, 2016 at 07:06:12PM -0700, Andrew Fish wrote:

>>

>>> On Mar 16, 2016, at 7:02 PM, Gao, Liming <liming.gao@intel.com> wrote:

>>>

>>> Andrew:

>>>   If we enable -Wunused-parameter option for GCC and CLANG, it will bring the big change to edk2 project. This patch is just to add ARG_UNUSED notation to Base.h. It has no impact. So, I think this patch is fine. 

>>>  

>>

>> Liming,

>>

>> I don't really have an issue adding it for source compatibility with other projects.

>>

>> The comments from Leif imply an across the codebase change? I was

>> trying to point out what a large undertaking that would be.

> 

> Well, it doesn't have to be an all in one go type thing.

> As long as the modifier exists, it can be added where people come

> across problems when building with lots of warnings enabled.

> 

> But a follow-up question (for everyone) - is ARG_UNUSED the keyword to

> use, or should it just be UNUSED?


Well, "ARG" does disturb me a bit. In this case, "parameter" is a better
match I think.

From C99, 3.3p1:

  *argument*
  actual argument
  actual parameter (deprecated)

  expression in the comma-separated list bounded by the parentheses in
  a function call expression, or a sequence of preprocessing tokens in
  the comma-separated list bounded by the parentheses in a
  function-like macro invocation

3.15p1:

  *parameter*
  formal parameter
  formal argument (deprecated)

  object declared as part of a function declaration or definition that
  acquires a value on entry to the function, or an identifier from the
  comma-separated list bounded by the parentheses immediately following
  the macro name in a function-like macro definition

Where we would employ this macro is function definitions (not function
calls), so I would propose PARAM_UNUSED or just UNUSED, over ARG_UNUSED.

Thanks
Laszlo

> 

> Regards,

> 

> Leif

> 

>>>>>> On 03/14/16 16:38, Leif Lindholm wrote:

>>>>>>> To permit many existing platforms to build with -Wunused-parameter, on

>>>>>>> GCC and CLANG, the unused parameters need to be annotated as such.

>>>>>>> Existing regexp code already uses ARG_UNUSED for this, but it is really

>>>>>>> needed across the codebase - so add a version in Base.h.

>>

>>

>> Thanks,

>>

>> Andrew Fish

>>

>>

>>> Thanks

>>> Liming <>

>>>  <>From: afish@apple.com <mailto:afish@apple.com> [mailto:afish@apple.com <mailto:afish@apple.com>] 

>>> Sent: Thursday, March 17, 2016 1:12 AM

>>> To: Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>

>>> Cc: Kinney, Michael D <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>>; edk2-devel@ml01.01.org <mailto:edk2-devel@ml01.01.org>; Gao, Liming <liming.gao@intel.com <mailto:liming.gao@intel.com>>; Leif Lindholm <leif.lindholm@linaro.org <mailto:leif.lindholm@linaro.org>>

>>> Subject: Re: [edk2] [RFC] MdePkg: add ARG_UNUSED notation to Base.h

>>>  

>>>

>>>> On Mar 16, 2016, at 10:06 AM, Laszlo Ersek wrote:

>>>>

>>>> On 03/16/16 17:50, Andrew Fish wrote:

>>>>>

>>>>>> On Mar 16, 2016, at 6:54 AM, Laszlo Ersek wrote:

>>>>>>

>>>>>> On 03/14/16 16:38, Leif Lindholm wrote:

>>>>>>> To permit many existing platforms to build with -Wunused-parameter, on

>>>>>>> GCC and CLANG, the unused parameters need to be annotated as such.

>>>>>>> Existing regexp code already uses ARG_UNUSED for this, but it is really

>>>>>>> needed across the codebase - so add a version in Base.h.

>>>>>>>

>>>>>>> Contributed-under: TianoCore Contribution Agreement 1.0

>>>>>>> Signed-off-by: Leif Lindholm 

>>>>>>> ---

>>>>>>>

>>>>>>> This is really the result of a friendly toolchain engineer informing me

>>>>>>> CLANG has the -Weverything flag, to actually enable all possible

>>>>>>> warnings.

>>>>>>>

>>>>>>> One problem trying to pick out the real bugs from the just not entirely

>>>>>>> clear code is that basically a lot of *LibNull implementations, and

>>>>>>> some libraries that should be usable, trip build failures due to unused

>>>>>>> parameters.

>>>>>>>

>>>>>>> MdePkg/Include/Base.h | 9 +++++++++

>>>>>>> 1 file changed, 9 insertions(+)

>>>>>>>

>>>>>>> diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h

>>>>>>> index 89b2aed..f789094 100644

>>>>>>> --- a/MdePkg/Include/Base.h

>>>>>>> +++ b/MdePkg/Include/Base.h

>>>>>>> @@ -189,6 +189,15 @@ struct _LIST_ENTRY {

>>>>>>> ///

>>>>>>> #define OPTIONAL

>>>>>>>

>>>>>>> +///

>>>>>>> +/// Function argument intentionally unused in function.

>>>>>>> +///

>>>>>>> +#if defined(__GNUC__) || defined(__clang__)

>>>>>>> + #define ARG_UNUSED __attribute__ ((unused))

>>>>>>> +#else

>>>>>>> + #define ARG_UNUSED

>>>>>>> +#endif

>>>>>>> +

>>>>>>> //

>>>>>>> // UEFI specification claims 1 and 0. We are concerned about the

>>>>>>> // complier portability so we did it this way.

>>>>>>>

>>>>>>

>>>>>> Support for this seems to go back to gcc-4.4:

>>>>>>

>>>>>> https://gcc.gnu.org/onlinedocs/gcc-4.4.7/gcc/Variable-Attributes.html <https://gcc.gnu.org/onlinedocs/gcc-4.4.7/gcc/Variable-Attributes.html>

>>>>>>

>>>>>> So it looks safe. (And I agree with the idea.)

>>>>>>

>>>>>

>>>>> I'm confused by the usage, or really when the compiler detects the

>>>>> warning?

>>>>>

>>>>> For example how is this going to work on public interfaces?

>>>>> Protocols, PPIs, or library class definitions? There is a single

>>>>> definition of the function, but multiple implementations. Some

>>>>> implementations may not use some arguments. But what arguments get

>>>>> used seems to be an implementation choice and not part of the API?

>>>>> Thus it seems like this macro is not going to enable changing

>>>>> compiler flags?

>>>>

>>>> I think this attribute would be used in library instances and protocol

>>>> implementations. It would not be used in library class headers nor

>>>> protocol definitions.

>>>>

>>>> Example:

>>>>

>>>> /* included from library class hdr of protocol def hdr */

>>>> int f(int x);

>>>>

>>>> /* code in library instance or protocol impl */

>>>> int f(int x __attribute__ ((unused)))

>>>> {

>>>> return 0;

>>>> }

>>>>

>>>> int main(void)

>>>> {

>>>> return f(-2);

>>>> }

>>>>

>>>> Compiles silently with

>>>>

>>>> $ gcc -Wall -Wextra -ansi -pedantic -Werror -o x x.c

>>>>

>>>> If I remove __attribute__ ((unused)), I get:

>>>>

>>>> x.c:5:15: error: unused parameter 'x' [-Werror=unused-parameter]

>>>> int f(int x)

>>>> ^

>>>> cc1: all warnings being treated as errors

>>>>

>>>

>>> OK that makes sense. That is feels like it is going to be a very very large change to the codebase given almost every protocol and PPI member pass the "This" pointer, but may not use it in code.

>>>

>>> Do we need something for VC++?

>>>

>>> Thanks,

>>>

>>> Andrew Fish

>>>

>>>> Thanks

>>>> Laszlo

>>>>

>>>>>

>>>>> Or am I just confused?

>>>>>

>>>>> Thanks,

>>>>>

>>>>> Andrew Fish

>>>>>

>>>>>

>>>>>> Acked-by: Laszlo Ersek 

>>>>>> _______________________________________________

>>>>>> edk2-devel mailing list

>>>>>> edk2-devel@lists.01.org <mailto:edk2-devel@lists.01.org>

>>>>>> https://lists.01.org/mailman/listinfo/edk2-devel <https://lists.01.org/mailman/listinfo/edk2-devel>

>>>>>

>>>>

>>>> _______________________________________________

>>>> edk2-devel mailing list

>>>> edk2-devel@lists.01.org <mailto:edk2-devel@lists.01.org>

>>>> https://lists.01.org/mailman/listinfo/edk2-devel <https://lists.01.org/mailman/listinfo/edk2-devel>


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
diff mbox

Patch

diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h
index 89b2aed..f789094 100644
--- a/MdePkg/Include/Base.h
+++ b/MdePkg/Include/Base.h
@@ -189,6 +189,15 @@  struct _LIST_ENTRY {
 ///
 #define OPTIONAL
 
+///
+/// Function argument intentionally unused in function.
+///
+#if defined(__GNUC__) || defined(__clang__)
+  #define ARG_UNUSED  __attribute__ ((unused))
+#else
+  #define ARG_UNUSED
+#endif
+
 //
 //  UEFI specification claims 1 and 0. We are concerned about the
 //  complier portability so we did it this way.