[if gte mso 9]>

Re: [edk2][what type is an enum, can UEFI really specify one?] Re: XhciSched.c:916: warning: ‘AsyncUrb’ may be used uninitialized

Subject: Re: [edk2][what type is an enum, can UEFI really specify one?] Re: XhciSched.c:916: warning: ‘AsyncUrb’ may be used uninitialized

From: "Sun, Rui" <rui.sun@intel.com>

To: "edk2-devel@lists.sourceforge.net" <edk2-devel@lists.sourceforge.net>

Date: 2012-04-19 12:03:21

For the statement GCC and clang respond to the range of enumerators;  we use the smallest underlying type (but no smaller than int) that can store all the enumerators, with a preference for unsigned types., I found that GCC supports an option fshort-enums which tells the compiler to allocate to an "enum" type only as many bytes as it needs for the declared range of possible values. Experiments show that sizeof  (EFI_MEMORY_TYPE) is 1 for IA32 and X64 arch with the option specified for GCC44. Fortunately, disassembly shows GCC44 promotes the size of an enum type to int for function parameter even if the option is specified.

 

I also found that building the DXE Core using GCC44 with -Wtype-limits failed.

 

Based on discussions we had before, we have 2 choices:

1.       Use type cast for force an enum type to be unsigned int. we have to fix all occurrences of this issue in EDKII.

2.       Turn off Wextra for clang (Is it possible?)

 

Folks, what do you think?

 

From: Andrew Fish [mailto:afish@apple.com]
Sent: Wednesday, April 18, 2012 2:19 PM
To: edk2-devel@lists.sourceforge.net
Subject: Re: [edk2] [what type is an enum, can UEFI really specify one?] Re: XhciSched.c:916: warning: AsyncUrb may be used uninitialized

 

Homework complete on enums, as I talked with some of our compiler experts...

 

It looks like the basic mechanics of passing data as used in the UEFI spec will work. Different compilers can get different semantics from the code, and this is what we have seen with the unsigned int vs. signed int warnings we are seeing. 

 

The following is what our most common compilers do:

MSVC always uses int as the underlying type of an enum.  It does this

even if the enum contains enumerators which obviously do not fit into
the range of int!

 

GCC and clang respond to the range of enumerators;  we use the
smallest underlying type (but no smaller than int) that can store all the
enumerators, with a preference for unsigned types.

 

So it basically looks like GCC and clang will get warnings as they will default to unsigned types, as this is what we have seen. 

 

Andrew Fish

 

 

 

On Apr 13, 2012, at 6:01 PM, Andrew Fish wrote:



Daryl,

 

I agree with you 100%, and your post is very helpful. The concept of strongly typed enums was added to C++11 and Objective-C, but does not exist in C.  

 

So we have a problem with the UEFI spec making the following statement, "<Enumerated Type>

Element of a standard ANSI C enum type declaration. Type INT32."

 

I think if we replace <Enumerated Type> with <Enumerated Constants> then the UEFI spec would make sense.  Then the spec is just defining int to be 32-bits. So it is OK to use the enum values defined in EFI as constants, but it is not OK to place an enum type in a structure. Take for example SERIAL_IO_MODE it has UINT32 elements and only uses the enum  values as constant. So the promotion rules make things work. A public API like gBS->SetTimer() is a little scarier, but I think it might just work. The reason is the caller is passing an int constant value to will get properly converted to what ever type the callee compiler thinks the specific enum value is. Since the size of the enumerations is known on both side, I don't think the arbitrary choices of the compiler will mater since the size of the constant is known. The only place the choices made by the complier come into play is code in the callee that error checks the enum value (this is outside the scope of the UEFI spec, and really in the scope of this mailing list). And that is how we started this entire conversation. 

 

So It looks like UEFI is doing the right thing, but the text about enum is some what confusing and it needs to be clarified. The edk2 code needs to follow the C language specification and cast properly prior to bounds checking. 

 

I will double check this with our language experts to make sure these assertions are true in all cases. 

 

Andrew Fish

typedef struct {

UINT32 ControlMask;

// current Attributes

UINT32 Timeout;

UINT64 BaudRate;

UINT32 ReceiveFifoDepth; 

UINT32 DataBits;

UINT32 Parity;

UINT32 StopBits; 

} SERIAL_IO_MODE;


Parity: If applicable, this is the EFI_PARITY_TYPE that is computed or checked as each character is transmitted or received. If the device does not support parity the value is the default parity value.

 

typedef enum {

      DefaultParity,

      NoParity,

      EvenParity,

      OddParity,

      MarkParity,

      SpaceParity

  } EFI_PARITY_TYPE;

 

 

On Apr 13, 2012, at 5:23 PM, Mcdaniel, Daryl wrote:



An "unsigned enum" is not a valid declaration.

Essentially, the "effective type" of an enumeration is determined by the set
of values of all enumerations for a given enumeration type.  In all cases,
an enumeration type will be an integer type.

If one or more of the enumeration constants defining an enumeration type
is negative, that particular enumeration type will be a signed integer type
sufficient to hold the values of all member enumeration constants.

An enumeration constant may be assigned to any object of integer type.  The
conversion (and any potential warnings) will proceed the same as for
assignment between regular integer objects of the same effective type.

Relevant sections of the C Language Specification (which is honored by
both the MSFT and GCC compilers) are quoted below.



From the ISO/IEC 9899 C Language Specification:

-----------------------------------------------
<integer types>
 * It is implementation-defined whether the specifier int designates the same
   type as signed int or the same type as unsigned int.
 * There are five standard signed integer types, designated as signed char,
   short int, int, long int, and long long int.
 * A "plain" int object has the natural size suggested by the architecture
   of the execution environment.
 * For each of the signed integer types, there is a corresponding (but
   different) unsigned integer type (designated with the keyword unsigned)
   that uses the same amount of storage (including sign information) and has
   the same alignment requirements.
 * The standard signed integer types and standard unsigned integer types are
   collectively called the standard integer types.

NOTE: Some ambiguity is introduced because the first two rules, quoted above,
     appear to contradict each other.

<enumerated types>
 * The type char, the signed and unsigned integer types, and the enumerated
   types are collectively called integer types.
 * Each distinct enumeration constitutes a different enumerated type.
 * Each enumerated type shall be compatible with char, a signed integer type,
   or an unsigned integer type. The choice of type is implementation-defined,
   but shall be capable of representing the values of all the members of the
   enumeration.  An implementation may delay the choice of which integer type
   until all enumeration constants have been seen.
<enumeration & enumeration constants>
 * An enumeration comprises a set of named integer constant values. Each
   distinct enumeration constitutes a different enumerated type.
 * An identifier declared as an enumeration constant has type int.
 * The expression that defines the value of an enumeration constant shall be
   an integer constant expression that has a value representable as an int.

You will notice that it is "implementation defined" whether unqualified
integer types are signed or unsigned.  This leaves types "char", "short",
"int", "long", and "long long" somewhat ambiguous.

For normal C programming, one should always use the signed or unsigned
qualifiers and never use the unadorned standard integer types.  

Daryl McDaniel
SSG/SSD/PTAC/Platform Software Infrastructure
+1 503-712-4670

Software is like entropy.
It is difficult to grasp, weighs nothing, and obeys the second law of thermodynamics;
i.e. it always increases.
-- Norman R. Augustine


-----Original Message-----
From: Sun, Rui [mailto:rui.sun@intel.com]
Sent: Thursday, April 12, 2012 8:41 PM
To: edk2-devel@lists.sourceforge.net
Subject: Re: [edk2] [what type is an enum, can UEFI really specify one?] Re: XhciSched.c:916: warning: AsyncUrb may be used uninitialized

Andrew,

Theoretically, it is a binary interoperability issue. For example, if the compiler for the caller treats an enumerated type as CHAR and the compiler for the caller treats the enumerated type as Integer, then there would be a problem. I agree with you that practically it is not a so bad problem.

I verified that type cast works with GCC44. Let's see the input from your compiler team:)


-----Original Message-----
From: Andrew Fish [mailto:afish@apple.com]
Sent: Thursday, April 12, 2012 11:17 PM
To: edk2-devel@lists.sourceforge.net
Subject: Re: [edk2] [what type is an enum, can UEFI really specify one?] Re: XhciSched.c:916: warning: AsyncUrb may be used uninitialized

On Apr 12, 2012, at 1:57 AM, Sun, Rui wrote:


Andrew,

 

You are correct. I got warnings when compiling the PCI bus driver using GCC44 with -Wtype-limits.

 

Enum type is used as parameters for some UEFI services and protocols. Do you think this is a binary interoperability issue as the choice of type is implementation-defined?

 


I don't think the problems are too bad. When I suppressed the warning and outputted assembly from clang the code seemed to be the same even when optimization is turned on. I want to check with the compiler team to make sure this is not a bad assumption.  It seems the statement in the UEFI spec is not possible to implement with an ANSI C compiler, but the enums in UEFI are unsigned so the binary values are the same for a signed and unsigned type.  It seems the only issue is in the code that is error checking the enum range.

I noticed I can cast the warning away in clang.

((INT32)UnsignedEnum)

Does this work in gcc? At some point in the past this did not work for gcc and clang, but it looks like clang fixed the bug.

The cast suppresses the warning, but also should ensure the compiler code generation is correct. So I think the cast is probably the right thing to do in the code.

Thanks,

Andrew



-----Original Message-----

From: Andrew Fish [mailto:afish@apple.com]

Sent: Thursday, April 12, 2012 1:17 AM

To: edk2-devel@lists.sourceforge.net

Subject: [edk2] [what type is an enum, can UEFI really specify one?] Re: XhciSched.c:916: warning: AsyncUrb may be used uninitialized

 

 

On Apr 11, 2012, at 1:48 AM, Sun, Rui wrote:

 

Hi,

 

The UEFI spec defines <Enumerated Type> as type of INT32. Refer to the section 2.3.1 Data Types in the UEFI 2.3.1 Errata A spec.

 

 

It looks to me like the only way to make an enum type INT32 in ANSI C is to add a negative element to the enum.

 

It is observed that most compilers treat Enum as INT32 except XCode. Does XCode support a command line option to specify Enum as INT32?

 

 

I think your observation is in error. It is my understanding that both gcc and clang pick the sign of the enum value based on a "best match" for the enumerated values, per ANSI C. You are not seeing warnings on gcc since they are not turned on. Try adding -Wextra to gcc and I think you will start seeing the same errors as Xcode/clang. I would not be surprised to find that Visual Studio does that same thing, but choses not to warn to support legacy code frameworks and include files.

 

This discusion, http://arstechnica.com/civis/viewtopic.php?f=20&t=1130118 , about gcc seems to track with what I've seen with Xcode/clang.

 

While gcc turns on the warning with -Wextra, clang has it on by default. See the following example (maybe some one could test on gcc): This example shows that the sign of enum is chosen by the compiler base on the range of values as per ANSI C. I think if you try the current gcc and turn on -Wextra you will get similar results.

 

~/work/Compiler>cat enum.c

 

typedef enum {

ENUM_a,

ENUM_b,

ENUM_Max

} ENUM_TEST;

 

typedef enum {

SENUM_a,

SENUM_b,

SENUM_Max,

SENUM_FORCE_SIGNED = -1

} ENUM_TEST_SIGNED;

 

int

TestSignedEnumCode (

ENUM_TEST_SIGNED  SignedEnum,

)

{

if ((SignedEnum < 0) || (SignedEnum >=ENUM_Max)) {

  return -1;

}

return 0;

}

 

int

TestUnsingedEnumCode (

ENUM_TEST         UnsignedEnum

)

{

if ((UnsignedEnum < 0) || (UnsignedEnum >=SENUM_Max)) {

  return -1;

}

return 0;

~/work/Compiler>clang -S enum.c

enum.c:31:21: warning: comparison of unsigned enum expression < 0 is always false

    [-Wtautological-compare]

if ((UnsignedEnum < 0) || (UnsignedEnum >=SENUM_Max)) {

     ~~~~~~~~~~~~ ^ ~

1 warning generated.

 

~/work/Compiler>clang -S enum.c -arch i386 -Oz -Wno-tautological-compare

~/work/Compiler>cat enum.s

            .section            __TEXT,__text,regular,pure_instructions

            .globl   _TestSignedEnumCode

_TestSignedEnumCode:                    ## @TestSignedEnumCode

## BB#0:

            pushl    %ebp

            movl    %esp, %ebp

            movl    $-1, %ecx

            cmpl    $1, 8(%ebp)

            movl    $0, %eax

            cmoval %ecx, %eax

            popl     %ebp

            ret

 

            .globl   _TestUnsingedEnumCode

_TestUnsingedEnumCode:                  ## @TestUnsingedEnumCode

## BB#0:

            pushl    %ebp

            movl    %esp, %ebp

            movl    $-1, %ecx

            cmpl    $1, 8(%ebp)

            movl    $0, %eax

            cmoval %ecx, %eax

            popl     %ebp

            ret

 

 

.subsections_via_symbols

 

 

-----Original Message-----

From: Andrew Fish [mailto:afish@apple.com]

Sent: Monday, April 02, 2012 5:49 AM

To: edk2-devel@lists.sourceforge.net

Subject: Re: [edk2] XhciSched.c:916: warning: AsyncUrb may be used uninitialized

 

 

On Apr 1, 2012, at 2:45 PM, Jordan Justen wrote:

 

On Sun, Apr 1, 2012 at 09:29, Andrew Fish <afish@apple.com> wrote:

I was going to test with clang, Xcode 4.3 compiler, but I hit some errors.

 

Per ANSI C spec:

Each enumerated type shall be compatible with char, a signed integer type, or an unsigned integer type. The choice of type is implementation-defined,110) but shall be capable of representing the values of all the members of the enumeration. The enumerated type is incomplete until after the } that terminates the list of enumerator declarations.

 

/Users/fish/work/edk2TOT/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c:88:13: error: comparison of unsigned enum expression < 0 is always false [-Werror,-Wtautological-compare]

if (Width < 0 || Width >= EfiPciIoWidthMaximum) {

 

Would something like this work?

if ((UINTN) Width >= EfiPciIoWidthMaximum) {

 

 

Yes that works with clang, and is correct per ANSI C.

 

-Jordan

 

  ~~~~~ ^ ~

/Users/fish/work/edk2TOT/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c:149:13: error: comparison of unsigned enum expression < 0 is always false [-Werror,-Wtautological-compare]

if (Width < 0 || Width >= EfiPciIoWidthMaximum) {

  ~~~~~ ^ ~

/Users/fish/work/edk2TOT/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c:218:13: error: comparison of unsigned enum expression < 0 is always false [-Werror,-Wtautological-compare]

if (Width < 0 || Width >= EfiPciIoWidthMaximum) {

  ~~~~~ ^ ~

/Users/fish/work/edk2TOT/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c:325:13: error: comparison of unsigned enum expression < 0 is always false [-Werror,-Wtautological-compare]

if (Width < 0 || Width > EfiPciIoWidthUint64) {

  ~~~~~ ^ ~

/Users/fish/work/edk2TOT/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c:424:13: error: comparison of unsigned enum expression < 0 is always false [-Werror,-Wtautological-compare]

if (Width < 0 || Width >= EfiPciIoWidthMaximum) {

  ~~~~~ ^ ~

/Users/fish/work/edk2TOT/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c:503:13: error: comparison of unsigned enum expression < 0 is always false [-Werror,-Wtautological-compare]

if (Width < 0 || Width >= EfiPciIoWidthMaximum) {

  ~~~~~ ^ ~

/Users/fish/work/edk2TOT/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c:581:13: error: comparison of unsigned enum expression < 0 is always false [-Werror,-Wtautological-compare]

if (Width < 0 || Width >= EfiPciIoWidthMaximum) {

  ~~~~~ ^ ~

/Users/fish/work/edk2TOT/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c:659:13: error: comparison of unsigned enum expression < 0 is always false [-Werror,-Wtautological-compare]

if (Width < 0 || Width >= EfiPciIoWidthMaximum) {

  ~~~~~ ^ ~

/Users/fish/work/edk2TOT/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c:884:13: error: comparison of unsigned enum expression < 0 is always false [-Werror,-Wtautological-compare]

if (Width < 0 || Width >= EfiPciIoWidthMaximum) {

  ~~~~~ ^ ~

/Users/fish/work/edk2TOT/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c:973:17: error: comparison of unsigned enum expression < 0 is always false [-Werror,-Wtautological-compare]

if (Operation < 0 || Operation >= EfiPciIoOperationMaximum) {

  ~~~~~~~~~ ^ ~

10 errors generated.

 

Andrew Fish

 

 

 

 

 

On Mar 31, 2012, at 7:18 PM, Tian, Feng wrote:

 

Which tool chain and build option are you using?

 

Our build system says it's passed.

 

and I checked code, it's impossible for this case.

 

-----Original Message-----

From: Sergey Isakov [mailto:isakov-sl@bk.ru]

Sent: Saturday, March 31, 2012 23:35

To: edk2-devel@lists.sourceforge.net

Subject: [edk2] XhciSched.c:916: warning: AsyncUrb may be used uninitialized

 

Dear sirs,

Is it possible to exclude warnings before commit into trunk?

Sergey

 

 

------------------------------------------------------------------------------

This SF email is sponsosred by:

Try Windows Azure free for 90 days Click Here http://p.sf.net/sfu/sfd2d-msazure _______________________________________________

edk2-devel mailing list

edk2-devel@lists.sourceforge.net

https://lists.sourceforge.net/lists/listinfo/edk2-devel

 

------------------------------------------------------------------------------

This SF email is sponsosred by:

Try Windows Azure free for 90 days Click Here

http://p.sf.net/sfu/sfd2d-msazure

_______________________________________________

edk2-devel mailing list

edk2-devel@lists.sourceforge.net

https://lists.sourceforge.net/lists/listinfo/edk2-devel

 

 

------------------------------------------------------------------------------

This SF email is sponsosred by:

Try Windows Azure free for 90 days Click Here

http://p.sf.net/sfu/sfd2d-msazure

_______________________________________________

edk2-devel mailing list

edk2-devel@lists.sourceforge.net

https://lists.sourceforge.net/lists/listinfo/edk2-devel

 

------------------------------------------------------------------------------

This SF email is sponsosred by:

Try Windows Azure free for 90 days Click Here

http://p.sf.net/sfu/sfd2d-msazure

_______________________________________________

edk2-devel mailing list

edk2-devel@lists.sourceforge.net

https://lists.sourceforge.net/lists/listinfo/edk2-devel

 

 

------------------------------------------------------------------------------

This SF email is sponsosred by:

Try Windows Azure free for 90 days Click Here

http://p.sf.net/sfu/sfd2d-msazure

_______________________________________________

edk2-devel mailing list

edk2-devel@lists.sourceforge.net

https://lists.sourceforge.net/lists/listinfo/edk2-devel

 

------------------------------------------------------------------------------

Better than sec? Nothing is better than sec when it comes to

monitoring Big Data applications. Try Boundary one-second

resolution app monitoring today. Free.

http://p.sf.net/sfu/Boundary-dev2dev

_______________________________________________

edk2-devel mailing list

edk2-devel@lists.sourceforge.net

https://lists.sourceforge.net/lists/listinfo/edk2-devel

 

 

------------------------------------------------------------------------------

Better than sec? Nothing is better than sec when it comes to

monitoring Big Data applications. Try Boundary one-second

resolution app monitoring today. Free.

http://p.sf.net/sfu/Boundary-dev2dev

_______________________________________________

edk2-devel mailing list

edk2-devel@lists.sourceforge.net

https://lists.sourceforge.net/lists/listinfo/edk2-devel

 

------------------------------------------------------------------------------

For Developers, A Lot Can Happen In A Second.

Boundary is the first to Know...and Tell You.

Monitor Your Applications in Ultra-Fine Resolution. Try it FREE!

http://p.sf.net/sfu/Boundary-d2dvs2

_______________________________________________

edk2-devel mailing list

edk2-devel@lists.sourceforge.net

https://lists.sourceforge.net/lists/listinfo/edk2-devel



------------------------------------------------------------------------------
For Developers, A Lot Can Happen In A Second.
Boundary is the first to Know...and Tell You.
Monitor Your Applications in Ultra-Fine Resolution. Try it FREE!
http://p.sf.net/sfu/Boundary-d2dvs2
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel

------------------------------------------------------------------------------
For Developers, A Lot Can Happen In A Second.
Boundary is the first to Know...and Tell You.
Monitor Your Applications in Ultra-Fine Resolution. Try it FREE!
http://p.sf.net/sfu/Boundary-d2dvs2
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel

------------------------------------------------------------------------------
For Developers, A Lot Can Happen In A Second.
Boundary is the first to Know...and Tell You.
Monitor Your Applications in Ultra-Fine Resolution. Try it FREE!
http://p.sf.net/sfu/Boundary-d2dvs2
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel

 

------------------------------------------------------------------------------
For Developers, A Lot Can Happen In A Second.
Boundary is the first to Know...and Tell You.
Monitor Your Applications in Ultra-Fine Resolution. Try it FREE!
http://p.sf.net/sfu/Boundary-d2dvs2_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel