[edk2] MdePkg: add UNUSED notation to Base.h

Message ID 1458229994-13192-1-git-send-email-leif.lindholm@linaro.org
State New
Headers show

Commit Message

Leif Lindholm March 17, 2016, 3:53 p.m.
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 called UNUSED in Base.h.

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

---

Change since RFC: rename ARG_UNUSED -> UNUSED.

 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

Leif Lindholm March 17, 2016, 4:53 p.m. | #1
Hi Michael,

On Thu, Mar 17, 2016 at 04:29:15PM +0000, Kinney, Michael D wrote:
> I am curious what value enabling the warning for an unused argument

> adds.  Can someone provide some examples where enabling this warning

> and adding the UNUSED tag addresses real issues?  I am concerned

> that we are enabling a warning that would impose a large number of

> source changes with little or no value.  I can be convinced

> otherwise, but would like to see some supporting data.


I am in no way suggesting that we turn this option on by default, even
for DEBUG builds. But it _is_ valuable to be able to build with all
possible warnings for the purpose of spotting undefined behaviour.

The trigger for this whole exercise was a presentation at Linaro
Connect last week by the toolchain guys, going into quite some details
on what undefined behaviour has caused real issues in other projects
(like the Linux kernel) after GCC started becoming more aggressive in
its optimisations around 4.8 timeframe.

If you want to develop difficulties sleeping, have a look at
http://connect.linaro.org/resource/bkk16/bkk16-503/

> Also, what are the requirements for maintenance of code once if this

> new UNUSED macro is used?  We have function implementations where an

> argument may not be used today, but a bug fix/feature enhancement

> later may use the UNUSED argument.  Do we get a different warning

> because it was tagged as UNUSED, but it is actually used now? Or is

> that silent?


All the UNUSED argument does is instruct the compiler to not give off
the warning when encountering it whilst building with the warning
enabled.

> I think there are a few categories of functions to consider here:

> 

> 1) Protocol/PPI functions.  The parameters for functions are defined

>    for a wide usage sometimes with optional behavior.  Specific

>    implementations may choose to not implement some optional

>    behavior, and hence may not require some of the parameters.  I do

>    not think a warning or an error should be generated by a

>    conformant implementation that does not use a parameter.

> 

> 2) Library Class functions.  The parameters for functions are also

>    defined for a wide usage sometimes with optional behavior.

>    Specific implementations may choose to not implement optional

>    behavior, and hence may not require some of the parameters.  I do

>    not think a warning or an error should be generated by a specific

>    library instance that does not use a parameter in a public

>    library class API.

> 

> 3) Internal functions.  If an internal function does not use a

>    parameter, then remove the parameter from the function call.

> 

> Can we enable /Wall and disable this specific warning?  Or is there

> a concern that category (3) cannot be detected without enabling this

> warning?  Maybe it would be better if we could find a way to disable

> this warning for all APIs that are decorated with EFIAPI?


Again, the intent is not to have this warning (or in my case
-Weverything) enabled in tools_def.template. At least not for a very
long time, until we've reached a place where doing so would not be
expected to trigger build failures.

Basically, -Wall is too week and catches too few errors, but the GCC
folks have conceded that if they extend its scope, much existing
software stops building. They did sound keen to add a -Weverything
option to GCC as well, though.

And working through and attempting to build with -Weverything _has_
picked up a fair amount of real bugs, so I think this is worthwhile.

Regards,

Leif

> 

> Thanks,

> 

> Mike

> 

> > -----Original Message-----

> > From: Leif Lindholm [mailto:leif.lindholm@linaro.org]

> > Sent: Thursday, March 17, 2016 8:53 AM

> > To: edk2-devel@lists.01.org

> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming

> > <liming.gao@intel.com>

> > Subject: [PATCH] MdePkg: add UNUSED notation to Base.h

> > 

> > 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 called UNUSED in Base.h.

> > 

> > Contributed-under: TianoCore Contribution Agreement 1.0

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

> > ---

> > 

> > Change since RFC: rename ARG_UNUSED -> UNUSED.

> > 

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

> >  1 file changed, 9 insertions(+)

> > 

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

> > index 89b2aed..a68b209 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 UNUSED  __attribute__ ((unused))

> > +#else

> > +  #define UNUSED

> > +#endif

> > +

> >  //

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

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

> > --

> > 2.1.4

> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Leif Lindholm March 17, 2016, 6:05 p.m. | #2
On Thu, Mar 17, 2016 at 10:31:21AM -0700, Andrew Fish wrote:
> > On Mar 17, 2016, at 9:53 AM, Leif Lindholm <leif.lindholm@linaro.org> wrote:

> > On Thu, Mar 17, 2016 at 04:29:15PM +0000, Kinney, Michael D wrote:

> >> I am curious what value enabling the warning for an unused argument

> >> adds.  Can someone provide some examples where enabling this warning

> >> and adding the UNUSED tag addresses real issues?  I am concerned

> >> that we are enabling a warning that would impose a large number of

> >> source changes with little or no value.  I can be convinced

> >> otherwise, but would like to see some supporting data.

> > 

> > I am in no way suggesting that we turn this option on by default, even

> > for DEBUG builds. But it _is_ valuable to be able to build with all

> > possible warnings for the purpose of spotting undefined behaviour.

> > 

> > The trigger for this whole exercise was a presentation at Linaro

> > Connect last week by the toolchain guys, going into quite some details

> > on what undefined behaviour has caused real issues in other projects

> > (like the Linux kernel) after GCC started becoming more aggressive in

> > its optimisations around 4.8 timeframe.

> > 

> > If you want to develop difficulties sleeping, have a look at

> > http://connect.linaro.org/resource/bkk16/bkk16-503/

> 

> Leif,

> 

> I did not think not using an argument in C was undefined behavior?


I am not saying it is. But it gets enabled along with a guaranteed way of
enabling all of the diagnostics detecting this (and then some).

I must confess to no small amount of surprise that optionally adding
the ability to tag an unused argument as unused is controversial.

If people genuinely do not see any benefit in enabling this, I guess I
can just carry it myself - although it will make it less likely that I
will set up this exercise in an automated fashion.

> LLVM/clang has been really aggressive in optimizing away undefined

> behavior and that is probably what started the push...

> http://blog.llvm.org/2011/05/what-every-c-programmer-should-know.html

> 

> Xcode clang emits traps for undefined behavior and optimizes away

> the undefined behavior, and all the code after it! Notice there is

> no stack cleanup or even a return from the C function in the example

> code.

> 

> For the edk2 XCODE5 (works on Xcode 5.0 and newer)  tools target we

> tell the compiler to emit a function call in place of the trap via,

> -ftrap-function=undefined_behavior_has_been_optimized_away_by_clang. This

> way we get a link failure if the compiler optimizes away undefined

> behavior. You might want to look to see if we can enable this flag

> on other compilers or get your toolchain team to add this

> flag/behavior.


That does seem like a very useful thing, and I appreciate the pointer
- I will definitely look into doing something like that.

But this mechanism is trapping only the undefined behaviours and not
the other useful warnings above and beyond -Wall, so it still does not
achieve what I'm after.

Perhaps I shall revive this thread after I've posted some of the bugs
I've found through this exercise, to demonstrate why I think it is a
useful thing to be able to do on a recurring basis.

Regards,

Leif

> Here is a simple example:

> ~/work/Compiler>cat undefined.c

> 

> int

> main ()

> {

>   int *Yikes = 0;

> 

>   *Yikes = 1;

>   return 0;

> }

> 

> ~/work/Compiler>clang -Os -S undefined.c

> ~/work/Compiler>cat undefined.S

> 	.section	__TEXT,__text,regular,pure_instructions

> 	.macosx_version_min 10, 11

> 	.globl	_main

> _main:                                  ## @main

> 	pushq	%rbp

> 	movq	%rsp, %rbp

> 	ud2

> 	.cfi_endproc

> 

> 

> .subsections_via_symbols

> 

> ~/work/Compiler>clang -Os undefined.c -ftrap-function=undefined_behavior_has_been_optimized_away_by_clang

> Undefined symbols for architecture x86_64:

>   "_undefined_behavior_has_been_optimized_away_by_clang", referenced from:

>       _main in undefined-350f38.o

> ld: symbol(s) not found for architecture x86_64

> clang: error: linker command failed with exit code 1 (use -v to see invocation)

> 

> We hit a bug in the compiler that caused the trap to not be emitted

> and I can't tell you how confused we got looking at the stack back

> traces and the code just kept running.


:)

Regards,

Leif

> 

> Thanks,

> 

> Andrew Fish

> 

> 

> >> Also, what are the requirements for maintenance of code once if this

> >> new UNUSED macro is used?  We have function implementations where an

> >> argument may not be used today, but a bug fix/feature enhancement

> >> later may use the UNUSED argument.  Do we get a different warning

> >> because it was tagged as UNUSED, but it is actually used now? Or is

> >> that silent?

> > 

> > All the UNUSED argument does is instruct the compiler to not give off

> > the warning when encountering it whilst building with the warning

> > enabled.

> > 

> >> I think there are a few categories of functions to consider here:

> >> 

> >> 1) Protocol/PPI functions.  The parameters for functions are defined

> >>   for a wide usage sometimes with optional behavior.  Specific

> >>   implementations may choose to not implement some optional

> >>   behavior, and hence may not require some of the parameters.  I do

> >>   not think a warning or an error should be generated by a

> >>   conformant implementation that does not use a parameter.

> >> 

> >> 2) Library Class functions.  The parameters for functions are also

> >>   defined for a wide usage sometimes with optional behavior.

> >>   Specific implementations may choose to not implement optional

> >>   behavior, and hence may not require some of the parameters.  I do

> >>   not think a warning or an error should be generated by a specific

> >>   library instance that does not use a parameter in a public

> >>   library class API.

> >> 

> >> 3) Internal functions.  If an internal function does not use a

> >>   parameter, then remove the parameter from the function call.

> >> 

> >> Can we enable /Wall and disable this specific warning?  Or is there

> >> a concern that category (3) cannot be detected without enabling this

> >> warning?  Maybe it would be better if we could find a way to disable

> >> this warning for all APIs that are decorated with EFIAPI?

> > 

> > Again, the intent is not to have this warning (or in my case

> > -Weverything) enabled in tools_def.template. At least not for a very

> > long time, until we've reached a place where doing so would not be

> > expected to trigger build failures.

> > 

> > Basically, -Wall is too week and catches too few errors, but the GCC

> > folks have conceded that if they extend its scope, much existing

> > software stops building. They did sound keen to add a -Weverything

> > option to GCC as well, though.

> > 

> > And working through and attempting to build with -Weverything _has_

> > picked up a fair amount of real bugs, so I think this is worthwhile.

> > 

> > Regards,

> > 

> > Leif

> > 

> >> 

> >> Thanks,

> >> 

> >> Mike

> >> 

> >>> -----Original Message-----

> >>> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]

> >>> Sent: Thursday, March 17, 2016 8:53 AM

> >>> To: edk2-devel@lists.01.org

> >>> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming

> >>> <liming.gao@intel.com>

> >>> Subject: [PATCH] MdePkg: add UNUSED notation to Base.h

> >>> 

> >>> 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 called UNUSED in Base.h.

> >>> 

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

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

> >>> ---

> >>> 

> >>> Change since RFC: rename ARG_UNUSED -> UNUSED.

> >>> 

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

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

> >>> 

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

> >>> index 89b2aed..a68b209 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 UNUSED  __attribute__ ((unused))

> >>> +#else

> >>> +  #define UNUSED

> >>> +#endif

> >>> +

> >>> //

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

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

> >>> --

> >>> 2.1.4

> >> 

> > _______________________________________________

> > 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
Laszlo Ersek March 17, 2016, 7:51 p.m. | #3
(adding Ard and Shumin because the below will tie in with another thread)

On 03/17/16 19:05, Leif Lindholm wrote:

> I must confess to no small amount of surprise that optionally adding

> the ability to tag an unused argument as unused is controversial.


I'm also surprised, but by a different thing. :)

(1) This is a side point, but I think it is a bit relevant.

See this sub-thread:

http://thread.gmane.org/gmane.comp.bios.edk2.devel/8848/focus=9254

Shumin did build his patch in question with GCC46, but the warning was
not reported to him. (Side side point: I will assume that Shumin is a
male name. I googled it and found nothing conclusive; apologies!) Ard
and myself stumbled upon it because we built the code for aarch64 as
well.

So the difference is ("obviously" in retrospect) in the different build
flags. Compare, in "BaseTools/Conf/tools_def.template":

DEFINE GCC46_IA32_CC_FLAGS           = DEF(GCC45_IA32_CC_FLAGS) -Wno-address -Wno-unused-but-set-variable
DEFINE GCC46_X64_CC_FLAGS            = DEF(GCC45_X64_CC_FLAGS) -Wno-address -Wno-unused-but-set-variable

vs.

  DEBUG_GCC48_AARCH64_CC_FLAGS   = DEF(GCC48_AARCH64_CC_FLAGS) -O0
RELEASE_GCC48_AARCH64_CC_FLAGS   = DEF(GCC48_AARCH64_CC_FLAGS) -Wno-unused-but-set-variable

Only the DEBUG target for AARCH64 does not include
"-Wno-unused-but-set-variable"; that's why Ard and I got that warning,
and Shumin didn't.

I think it might be feasible to remove -Wno-unused-but-set-variable. I
removed it and built OvmfPkg and ArmVirtPkg with a number of settings
(for the DEBUG target, with gcc-4.8). The warnings that surfaced were
only:

> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c:1114:29: warning: variable 'BusEnd' set but not used [-Wunused-but-set-variable]

> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c:1114:9: warning: variable 'BusEnd' set but not used [-Wunused-but-set-variable]

> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c:631:10: warning: variable 'AddrLen' set but not used [-Wunused-but-set-variable]

> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c:631:41: warning: variable 'AddrLen' set but not used [-Wunused-but-set-variable]

> UefiCpuPkg/Library/MtrrLib/MtrrLib.c:462:11: warning: variable 'TempQword' set but not used [-Wunused-but-set-variable]

> UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c:1314:25: warning: variable 'SwSmiCpuIndex' set but not used [-Wunused-but-set-variable]

> UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c:1314:9: warning: variable 'SwSmiCpuIndex' set but not used [-Wunused-but-set-variable]

> UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c:345:14: warning: variable 'Status' set but not used [-Wunused-but-set-variable]

> UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c:345:36: warning: variable 'Status' set but not used [-Wunused-but-set-variable]

> UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c:779:14: warning: variable 'Status' set but not used [-Wunused-but-set-variable]

> UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c:779:22: warning: variable 'Status' set but not used [-Wunused-but-set-variable]


Side point ends.

(2) Beyond removing "-Wno-unused-but-set-variable", I then added
"-Wunused-parameter -Wunused-but-set-parameter".

Oh boy. :) First, a large number of "AutoGen.c" files (maybe all of
them?) seem to hit it. I simply filtered those out.

Second, the remaining set of warnings is also huge: almost 4000
instances. The list of locations is too large to attach or paste (and I
don't think the list will allow compressed attachments), so I'm
uploading it here:
<http://people.redhat.com/lersek/unused-parameter.txt>.

If I understand correctly, if we wanted to enable "-Wunused-parameter
-Wunused-but-set-parameter" even just occasionally, these ~4000
instances would have to be audited, and each should be either fixed
(i.e., internal functions should drop the parameters) or marked UNUSED
(i.e., library instances and PPI/protocol implementations should
annotate their definitions of public functions).

Thus, this is what surprises me. It looks daunting.

Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo Ersek March 17, 2016, 7:57 p.m. | #4
On 03/17/16 20:51, Laszlo Ersek wrote:
> (adding Ard and Shumin because the below will tie in with another thread)

> 

> On 03/17/16 19:05, Leif Lindholm wrote:

> 

>> I must confess to no small amount of surprise that optionally adding

>> the ability to tag an unused argument as unused is controversial.


> If I understand correctly, if we wanted to enable "-Wunused-parameter

> -Wunused-but-set-parameter" even just occasionally, these ~4000

> instances would have to be audited, and each should be either fixed

> (i.e., internal functions should drop the parameters) or marked UNUSED

> (i.e., library instances and PPI/protocol implementations should

> annotate their definitions of public functions).

> 

> Thus, this is what surprises me. It looks daunting.


Small clarification: if you'd like to selectively add
"-Wunused-parameter -Wunused-but-set-parameter" to the [BuildOptions] of
a number of (non-core?) modules (in their INF files), and employ UNUSED
in connection with that, I certainly think that's a valid use case.

To me it does justify this patch. Namely, perhaps marking parameters as
UNUSED will not be enforced across the entire tree, but if a module
owner would like to enable those warnings on his/her turf, then he/she
should be able to annotate the unused parameters with a macro that is
centrally defined. The macro definition should be universal, even though
its application might not be.

Thanks
Laszlo

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

Patch

diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h
index 89b2aed..a68b209 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 UNUSED  __attribute__ ((unused))
+#else
+  #define UNUSED
+#endif
+
 //
 //  UEFI specification claims 1 and 0. We are concerned about the
 //  complier portability so we did it this way.