Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise operations.
Hey Mike,
Thanks for your reply.
Under these circumstances I will not submit a V2. I hope you find a decent set
of tests to be performed.
Regards,
Marvin.
> -Original Message-
> From: Kinney, Michael D <michael.d.kin...@intel.com>
> Sent: Thursday, March 1, 2018 6:19 PM
> To: Marvin Häuser <marvin.haeu...@outlook.com>; edk2-
> de...@lists.01.org; Kinney, Michael D <michael.d.kin...@intel.com>
> Cc: ler...@redhat.com; Gao, Liming <liming@intel.com>
> Subject: RE: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise
> operations.
>
> Marvin,
>
> Thanks. I agree that there may be some compiler behavior assumptions.
>
> I would prefer we add to Base.h tests for the expected behavior
> assumptions and break the build if the compiler does not adhere to those
> assumptions. We have already added these to verify the size of base types
> and the size of enums.
>
> /**
> Verifies the storage size of a given data type.
>
> This macro generates a divide by zero error or a zero size array
> declaration in
> the preprocessor if the size is incorrect. These are declared as "extern"
> so
> the space for these arrays will not be in the modules.
>
> @param TYPE The date type to determine the size of.
> @param Size The expected size for the TYPE.
>
> **/
> #define VERIFY_SIZE_OF(TYPE, Size) extern UINT8
> _VerifySizeof##TYPE[(sizeof(TYPE) == (Size)) / (sizeof(TYPE) == (Size))]
>
> //
> // Verify that ProcessorBind.h produced UEFI Data Types that are compliant
> with // Section 2.3.1 of the UEFI 2.3 Specification.
> //
> VERIFY_SIZE_OF (BOOLEAN, 1);
> VERIFY_SIZE_OF (INT8, 1);
> VERIFY_SIZE_OF (UINT8, 1);
> VERIFY_SIZE_OF (INT16, 2);
> VERIFY_SIZE_OF (UINT16, 2);
> VERIFY_SIZE_OF (INT32, 4);
> VERIFY_SIZE_OF (UINT32, 4);
> VERIFY_SIZE_OF (INT64, 8);
> VERIFY_SIZE_OF (UINT64, 8);
> VERIFY_SIZE_OF (CHAR8, 1);
> VERIFY_SIZE_OF (CHAR16, 2);
>
> //
> // The following three enum types are used to verify that the compiler //
> configuration for enum types is compliant with Section 2.3.1 of the // UEFI
> 2.3
> Specification. These enum types and enum values are not // intended to be
> used. A prefix of '__' is used avoid conflicts with // other types.
> //
> typedef enum {
> __VerifyUint8EnumValue = 0xff
> } __VERIFY_UINT8_ENUM_SIZE;
>
> typedef enum {
> __VerifyUint16EnumValue = 0x
> } __VERIFY_UINT16_ENUM_SIZE;
>
> typedef enum {
> __VerifyUint32EnumValue = 0x
> } __VERIFY_UINT32_ENUM_SIZE;
>
> VERIFY_SIZE_OF (__VERIFY_UINT8_ENUM_SIZE, 4); VERIFY_SIZE_OF
> (__VERIFY_UINT16_ENUM_SIZE, 4); VERIFY_SIZE_OF
> (__VERIFY_UINT32_ENUM_SIZE, 4);
>
>
> Mike
>
>
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-
> > boun...@lists.01.org] On Behalf Of Marvin Häuser
> > Sent: Thursday, March 1, 2018 3:11 AM
> > To: edk2-devel@lists.01.org; Kinney, Michael D
> > <michael.d.kin...@intel.com>
> > Cc: ler...@redhat.com; Gao, Liming
> > <liming@intel.com>
> > Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise
> > operations.
> >
> >
> > > -----Original Message-
> > > From: Kinney, Michael D <michael.d.kin...@intel.com>
> > > Sent: Thursday, March 1, 2018 2:42 AM
> > > To: Marvin Häuser <marvin.haeu...@outlook.com>; edk2-
> > > de...@lists.01.org; Kinney, Michael D
> > <michael.d.kin...@intel.com>
> > > Cc: ler...@redhat.com; Gao, Liming
> > <liming@intel.com>
> > > Subject: RE: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure
> > safe bitwise
> > > operations.
> > >
> > > Hi Marvin,
> > >
> > > Yes. I have been reading the thread.
> > >
> > > Lots of really good analysis.
> > >
> > > However, it does not make sense to me to add 'u' to
> > the smaller BITxx,
> > > BASExx, and SIZExx macros if we have constants in
> > other places that do not
> > > add the 'u'. I think this patch may increase the
> > inconsistency of the whole
> > > tree.
> >
> > Basically applying this to the BIT definitions first was to see
> > whether the concept is percepted as a whole.
> > Of course this should be applied to all definitions that are at some
> > point used as a mask, which I continued to do locally.
> >
> > >
> > > If we don’t make this change, what types of issues do
> > we need to fix and
> > > what would the fix entail?
> >
> > To be honest, actual issues are very unlikely to happen as all
&
Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise operations.
Marvin,
Thanks. I agree that there may be some compiler behavior assumptions.
I would prefer we add to Base.h tests for the expected behavior
assumptions and break the build if the compiler does not adhere
to those assumptions. We have already added these to verify the
size of base types and the size of enums.
/**
Verifies the storage size of a given data type.
This macro generates a divide by zero error or a zero size array declaration
in
the preprocessor if the size is incorrect. These are declared as "extern" so
the space for these arrays will not be in the modules.
@param TYPE The date type to determine the size of.
@param Size The expected size for the TYPE.
**/
#define VERIFY_SIZE_OF(TYPE, Size) extern UINT8
_VerifySizeof##TYPE[(sizeof(TYPE) == (Size)) / (sizeof(TYPE) == (Size))]
//
// Verify that ProcessorBind.h produced UEFI Data Types that are compliant with
// Section 2.3.1 of the UEFI 2.3 Specification.
//
VERIFY_SIZE_OF (BOOLEAN, 1);
VERIFY_SIZE_OF (INT8, 1);
VERIFY_SIZE_OF (UINT8, 1);
VERIFY_SIZE_OF (INT16, 2);
VERIFY_SIZE_OF (UINT16, 2);
VERIFY_SIZE_OF (INT32, 4);
VERIFY_SIZE_OF (UINT32, 4);
VERIFY_SIZE_OF (INT64, 8);
VERIFY_SIZE_OF (UINT64, 8);
VERIFY_SIZE_OF (CHAR8, 1);
VERIFY_SIZE_OF (CHAR16, 2);
//
// The following three enum types are used to verify that the compiler
// configuration for enum types is compliant with Section 2.3.1 of the
// UEFI 2.3 Specification. These enum types and enum values are not
// intended to be used. A prefix of '__' is used avoid conflicts with
// other types.
//
typedef enum {
__VerifyUint8EnumValue = 0xff
} __VERIFY_UINT8_ENUM_SIZE;
typedef enum {
__VerifyUint16EnumValue = 0x
} __VERIFY_UINT16_ENUM_SIZE;
typedef enum {
__VerifyUint32EnumValue = 0x
} __VERIFY_UINT32_ENUM_SIZE;
VERIFY_SIZE_OF (__VERIFY_UINT8_ENUM_SIZE, 4);
VERIFY_SIZE_OF (__VERIFY_UINT16_ENUM_SIZE, 4);
VERIFY_SIZE_OF (__VERIFY_UINT32_ENUM_SIZE, 4);
Mike
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-
> boun...@lists.01.org] On Behalf Of Marvin Häuser
> Sent: Thursday, March 1, 2018 3:11 AM
> To: edk2-devel@lists.01.org; Kinney, Michael D
> <michael.d.kin...@intel.com>
> Cc: ler...@redhat.com; Gao, Liming
> <liming....@intel.com>
> Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure
> safe bitwise operations.
>
>
> > -Original Message-
> > From: Kinney, Michael D <michael.d.kin...@intel.com>
> > Sent: Thursday, March 1, 2018 2:42 AM
> > To: Marvin Häuser <marvin.haeu...@outlook.com>; edk2-
> > de...@lists.01.org; Kinney, Michael D
> <michael.d.kin...@intel.com>
> > Cc: ler...@redhat.com; Gao, Liming
> <liming@intel.com>
> > Subject: RE: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure
> safe bitwise
> > operations.
> >
> > Hi Marvin,
> >
> > Yes. I have been reading the thread.
> >
> > Lots of really good analysis.
> >
> > However, it does not make sense to me to add 'u' to
> the smaller BITxx,
> > BASExx, and SIZExx macros if we have constants in
> other places that do not
> > add the 'u'. I think this patch may increase the
> inconsistency of the whole
> > tree.
>
> Basically applying this to the BIT definitions first
> was to see whether the concept is percepted as a whole.
> Of course this should be applied to all definitions
> that are at some point used as a mask, which I
> continued to do locally.
>
> >
> > If we don’t make this change, what types of issues do
> we need to fix and
> > what would the fix entail?
>
> To be honest, actual issues are very unlikely to happen
> as all architectures supported by the specification use
> two-complements for negative values.
> Furthermore, all currently supported compilers
> implement bitwise operations for signed integers
> seemingly the same way as for unsigned types.
> However, if either will change in the future, code will
> silently break as in many mask operations will return
> values not intended by the code.
>
> If you are not interested in the solution concepts
> previously discussed, I propose as least a Unit Test to
> verify the operations used in praxis work out fine.
>
> Thanks,
> Marvin.
>
> >
> > Mike
> >
> > > -Original Message-----
> > > From: Marvin Häuser
> [mailto:marvin.haeu...@outlook.com]
> > > Sent: Wednesday, February 28, 2018 10:52 AM
> > > To: edk2-devel@lists.01.org; Kinney, Michael D
> > > <michael.d.kin...@intel.com>
> > > Cc: ler...@redhat.com; Gao, Liming
> > > <liming@intel.com>
> > > Subject: RE: [edk2] [PATCH 1/2] MdePkg/Base.h:
> Ensure safe bitwise
> > > operations.
> > >
>
Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise operations.
> -Original Message- > From: Laszlo Ersek <ler...@redhat.com> > Sent: Thursday, March 1, 2018 11:40 AM > To: Marvin Häuser <marvin.haeu...@outlook.com>; edk2- > de...@lists.01.org > Cc: michael.d.kin...@intel.com; liming....@intel.com > Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise > operations. > > On 02/28/18 22:07, Marvin Häuser wrote: > > One comment is inline. > > > > Thank you in advance, > > Marvin. > > > >> -Original Message- > >> From: edk2-devel <edk2-devel-boun...@lists.01.org> On Behalf Of > >> Marvin Häuser > >> Sent: Wednesday, February 28, 2018 7:46 PM > >> To: edk2-devel@lists.01.org; Laszlo Ersek <ler...@redhat.com> > >> Cc: michael.d.kin...@intel.com; liming@intel.com > >> Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise > >> operations. > >> > >> I have just locally updated all BIT defines to use the ULL prefix and > >> added casts to defines using them. > >> I did that to ensure that 1) inversions always produce the correct > >> value and 2) assignments never result in implicit casts to a smaller > >> int, which would raise a warning. > >> > >> After I was done doing it for MdePkg, a build showed that (N)ASM > >> files consumed these definitions. > >> > >> I only see a bunch of possible solutions to that: > >> * Prohibit the usage of such defines in assembly code (which I would > >> strongly dislike). > >> * Introduce a "DEFINE_BIT" macro which produces one definition for C > >> code and one for assembly. > > > > I only just realized that including C headers was not a NASM feature, but it > is actually edk2 invoking the PP. > > Might the best solution just be to introduce a casting macro, which casts > when it's invoked for a C compiler and doesn't when it's invoked for an > assembler? > > Basically would require nothing else than adding a "- > D__EDK2_ASSEMBLER__" or something alike to the PP flags when applicable. > > > > Any opinion on that? > > Sigh, I don't know what to answer. On one hand (if we can get it to work > without regressions) I like the idea of making all BITx macros ULL. On the > other hand, defining the same macro with different replacement text, > dependent on whether the including source code is assembly or C, looks > dirty. I can't really put my finger on it, but I feel such dual definitions > could > cause issues or confusion. If BaseTools people are OK with the dual > definition, I guess I could live with it. Indeed it is dirty, however I don't think there is any choice but the smallest devil. Leaving them signed might become dangerous, relying on suffixes is not a proper solution considering the new 128-bit type and casting results in the sharing issue between C and NASM. Actually I would abandon the "two definitions" concept as of the idea of introducing __EDK2_ASSEMBLER__. The solution I think would be the best to ensure a safe and forward-compatible is: 1) Cast all generic defines that might be used as masks to the highest available integer type (macro), including BITx. 2) Introduce a casting macro which would roughly look like this and apply it to all "named bit" definitions: #ifdef __EDK2_ASSEMLER__ #define PP_CAST(Value, Type) (Value) #else #define PP_CAST(Value, Type) ((Type)(Value)) #endif This way: * Bit operations on all types of unsigned integers are safe and well-defined. * One can intuitively use inverses for both generic and "named" masks. * One can continue to intuitively assign "named bits" to variables of their type (except for when integer promotion happens as part of an OP, of course, but this is unrelated). * Code not casting correctly will raise compile-time errors. The only alternative worth arguing I see is scrapping it all and introducing Unit Tests. However, should a Unit Test ever fail a specific compiler, we would be back here again. Regards, Marvin. > > Thanks, > Laszlo > > > > >> * Rely on 'ULL' always producing the biggest possible value > >> (including the 128- bit range new to the spec) or documenting an > >> exception for it, and insist on the caller casting (which I would find > >> quite > ugly). > >> * Scrap the patch and continue to rely on > >> compiler-/architecture-specific behavior, which could cause issues > seemingly randomly. > >> > >> Thanks, > >> Marvin. > >> > >>> -Original Message- > >>> From: edk2-devel <edk2-devel-boun...@lists.01.org> On Behalf Of > >&g
Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise operations.
> -Original Message- > From: Kinney, Michael D <michael.d.kin...@intel.com> > Sent: Thursday, March 1, 2018 2:42 AM > To: Marvin Häuser <marvin.haeu...@outlook.com>; edk2- > de...@lists.01.org; Kinney, Michael D <michael.d.kin...@intel.com> > Cc: ler...@redhat.com; Gao, Liming <liming@intel.com> > Subject: RE: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise > operations. > > Hi Marvin, > > Yes. I have been reading the thread. > > Lots of really good analysis. > > However, it does not make sense to me to add 'u' to the smaller BITxx, > BASExx, and SIZExx macros if we have constants in other places that do not > add the 'u'. I think this patch may increase the inconsistency of the whole > tree. Basically applying this to the BIT definitions first was to see whether the concept is percepted as a whole. Of course this should be applied to all definitions that are at some point used as a mask, which I continued to do locally. > > If we don’t make this change, what types of issues do we need to fix and > what would the fix entail? To be honest, actual issues are very unlikely to happen as all architectures supported by the specification use two-complements for negative values. Furthermore, all currently supported compilers implement bitwise operations for signed integers seemingly the same way as for unsigned types. However, if either will change in the future, code will silently break as in many mask operations will return values not intended by the code. If you are not interested in the solution concepts previously discussed, I propose as least a Unit Test to verify the operations used in praxis work out fine. Thanks, Marvin. > > Mike > > > -Original Message- > > From: Marvin Häuser [mailto:marvin.haeu...@outlook.com] > > Sent: Wednesday, February 28, 2018 10:52 AM > > To: edk2-devel@lists.01.org; Kinney, Michael D > > <michael.d.kin...@intel.com> > > Cc: ler...@redhat.com; Gao, Liming > > <liming@intel.com> > > Subject: RE: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise > > operations. > > > > Hey Mike, > > > > You are right, the patch was premature because I did not consider any > > 'incorrect' or 'clever' usages of these definitions. > > The problem is not primarily undefined behavior, but > > implementation-defined behavior. > > Any bitwise operation to a signed integer results in > > implementation-defined behavior, which compilers usually do not warn > > about, while well-defined behavior is desirable. > > > > Have you read Laszlo's comments? They are quite good at showing up > > what logics might be and are relied on, which however are not > > guaranteed to be the case for > > non-x86 architectures, > > or even for x86 in case a development team decides to change this > > behavior some day or a new toolchain not having adopted them in the > > first place should be added. > > > > Furthermore, I don't think inconsistency between the definitions > > generally is desirable. > > > > Thanks, > > Marvin. > > > > > -Original Message- > > > From: Kinney, Michael D <michael.d.kin...@intel.com> > > > Sent: Wednesday, February 28, 2018 7:37 PM > > > To: Marvin Häuser <marvin.haeu...@outlook.com>; edk2- > > > de...@lists.01.org; Laszlo Ersek <ler...@redhat.com>; > > Kinney, Michael D > > > <michael.d.kin...@intel.com> > > > Cc: Gao, Liming <liming@intel.com> > > > Subject: RE: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure > > safe bitwise > > > operations. > > > > > > Hi Marvin, > > > > > > I do not think add 'u' to the BITxx defines does not > > seem to be a complete > > > solution. Code can use integer constants in lots of > > places including other > > > #defines or inline in expressions. > > > > > > If we follow your suggestion wouldn’t we need to add > > 'u' to every constant > > > that does not start with a '-' > > > and might potentially be used with a bit operation? > > > > > > Compilers are doing a good job of finding undefined > > behavior. Isn’t that > > > sufficient to fix the issues identified? > > > > > > Mike > > > > > > > -Original Message- > > > > From: Marvin Häuser > > [mailto:marvin.haeu...@outlook.com] > > > > Sent: Wednesday, February 28, 2018 6:21 AM > > > > To: edk2-devel@lists.01.org; Laszlo Ersek > > <ler...@redhat.com> > > > > Cc: K
Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise operations.
On 02/28/18 22:07, Marvin Häuser wrote: > One comment is inline. > > Thank you in advance, > Marvin. > >> -Original Message- >> From: edk2-devel <edk2-devel-boun...@lists.01.org> On Behalf Of Marvin >> Häuser >> Sent: Wednesday, February 28, 2018 7:46 PM >> To: edk2-devel@lists.01.org; Laszlo Ersek <ler...@redhat.com> >> Cc: michael.d.kin...@intel.com; liming....@intel.com >> Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise >> operations. >> >> I have just locally updated all BIT defines to use the ULL prefix and added >> casts to defines using them. >> I did that to ensure that 1) inversions always produce the correct value and >> 2) >> assignments never result in implicit casts to a smaller int, which would >> raise a >> warning. >> >> After I was done doing it for MdePkg, a build showed that (N)ASM files >> consumed these definitions. >> >> I only see a bunch of possible solutions to that: >> * Prohibit the usage of such defines in assembly code (which I would strongly >> dislike). >> * Introduce a "DEFINE_BIT" macro which produces one definition for C code >> and one for assembly. > > I only just realized that including C headers was not a NASM feature, but it > is actually edk2 invoking the PP. > Might the best solution just be to introduce a casting macro, which casts > when it's invoked for a C compiler and doesn't when it's invoked for an > assembler? > Basically would require nothing else than adding a "-D__EDK2_ASSEMBLER__" or > something alike to the PP flags when applicable. > > Any opinion on that? Sigh, I don't know what to answer. On one hand (if we can get it to work without regressions) I like the idea of making all BITx macros ULL. On the other hand, defining the same macro with different replacement text, dependent on whether the including source code is assembly or C, looks dirty. I can't really put my finger on it, but I feel such dual definitions could cause issues or confusion. If BaseTools people are OK with the dual definition, I guess I could live with it. Thanks, Laszlo > >> * Rely on 'ULL' always producing the biggest possible value (including the >> 128- >> bit range new to the spec) or documenting an exception for it, and insist on >> the caller casting (which I would find quite ugly). >> * Scrap the patch and continue to rely on compiler-/architecture-specific >> behavior, which could cause issues seemingly randomly. >> >> Thanks, >> Marvin. >> >>> -----Original Message- >>> From: edk2-devel <edk2-devel-boun...@lists.01.org> On Behalf Of Marvin >>> Häuser >>> Sent: Wednesday, February 28, 2018 3:21 PM >>> To: edk2-devel@lists.01.org; Laszlo Ersek <ler...@redhat.com> >>> Cc: michael.d.kin...@intel.com; liming@intel.com >>> Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise >>> operations. >>> >>> Hey Laszlo, >>> >>> I cut your rant because it is not strictly related to this patch. >>> However, thank you for composing it nevertheless because it was an >> interesting read! >>> Comments are inline. >>> >>> Michael, Liming, >>> Do you have any comments regarding the discussion? Thanks in advance. >>> >>> Best regards, >>> Marvin. >>> >>>> -Original Message- >>>> From: Laszlo Ersek <ler...@redhat.com> >>>> Sent: Wednesday, February 28, 2018 2:57 PM >>>> To: Marvin Häuser <marvin.haeu...@outlook.com>; edk2- >>>> de...@lists.01.org >>>> Cc: michael.d.kin...@intel.com; liming@intel.com >>>> Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise >>>> operations. >>>> >>>> On 02/28/18 12:43, Marvin Häuser wrote: >>> [...] >>>>> as edk2 does not support vendor extensions such as __int128 anyway. >>>> >>>> Not *yet*, I guess :) UEFI 2.7 does list UINT128 / INT128, in table >>>> 5, "Common UEFI Data Types". I believe those typedefs may have been >>> added for RISC-V. >>> >>> Oh yikes, I have not noticed that before. Besides that I wonder how >>> that will be implemented by edk2 for non-RISC-V platforms, maybe that >>> should be considered? >>> As ridiculous as it sounds, maybe some kind of UINT_MAX type (now >>> UINT64, later UINT128) should be introduced and any BIT or bitmask >>> definition being explicitly casted to that? >>> A
Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise operations.
Hi Marvin, Yes. I have been reading the thread. Lots of really good analysis. However, it does not make sense to me to add 'u' to the smaller BITxx, BASExx, and SIZExx macros if we have constants in other places that do not add the 'u'. I think this patch may increase the inconsistency of the whole tree. If we don’t make this change, what types of issues do we need to fix and what would the fix entail? Mike > -Original Message- > From: Marvin Häuser [mailto:marvin.haeu...@outlook.com] > Sent: Wednesday, February 28, 2018 10:52 AM > To: edk2-devel@lists.01.org; Kinney, Michael D > <michael.d.kin...@intel.com> > Cc: ler...@redhat.com; Gao, Liming > <liming....@intel.com> > Subject: RE: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure > safe bitwise operations. > > Hey Mike, > > You are right, the patch was premature because I did > not consider any 'incorrect' or 'clever' usages of > these definitions. > The problem is not primarily undefined behavior, but > implementation-defined behavior. > Any bitwise operation to a signed integer results in > implementation-defined behavior, which compilers > usually do not warn about, while well-defined behavior > is desirable. > > Have you read Laszlo's comments? They are quite good at > showing up what logics might be and are relied on, > which however are not guaranteed to be the case for > non-x86 architectures, > or even for x86 in case a development team decides to > change this behavior some day or a new toolchain not > having adopted them in the first place should be added. > > Furthermore, I don't think inconsistency between the > definitions generally is desirable. > > Thanks, > Marvin. > > > -Original Message- > > From: Kinney, Michael D <michael.d.kin...@intel.com> > > Sent: Wednesday, February 28, 2018 7:37 PM > > To: Marvin Häuser <marvin.haeu...@outlook.com>; edk2- > > de...@lists.01.org; Laszlo Ersek <ler...@redhat.com>; > Kinney, Michael D > > <michael.d.kin...@intel.com> > > Cc: Gao, Liming <liming@intel.com> > > Subject: RE: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure > safe bitwise > > operations. > > > > Hi Marvin, > > > > I do not think add 'u' to the BITxx defines does not > seem to be a complete > > solution. Code can use integer constants in lots of > places including other > > #defines or inline in expressions. > > > > If we follow your suggestion wouldn’t we need to add > 'u' to every constant > > that does not start with a '-' > > and might potentially be used with a bit operation? > > > > Compilers are doing a good job of finding undefined > behavior. Isn’t that > > sufficient to fix the issues identified? > > > > Mike > > > > > -----Original Message- > > > From: Marvin Häuser > [mailto:marvin.haeu...@outlook.com] > > > Sent: Wednesday, February 28, 2018 6:21 AM > > > To: edk2-devel@lists.01.org; Laszlo Ersek > <ler...@redhat.com> > > > Cc: Kinney, Michael D <michael.d.kin...@intel.com>; > Gao, Liming > > > <liming@intel.com> > > > Subject: RE: [edk2] [PATCH 1/2] MdePkg/Base.h: > Ensure safe bitwise > > > operations. > > > > > > Hey Laszlo, > > > > > > I cut your rant because it is not strictly related > to this patch. > > > However, thank you for composing it nevertheless > because it was an > > > interesting read! > > > Comments are inline. > > > > > > Michael, Liming, > > > Do you have any comments regarding the discussion? > > > Thanks in advance. > > > > > > Best regards, > > > Marvin. > > > > > > > -Original Message- > > > > From: Laszlo Ersek <ler...@redhat.com> > > > > Sent: Wednesday, February 28, 2018 2:57 PM > > > > To: Marvin Häuser <marvin.haeu...@outlook.com>; > edk2- > > > > de...@lists.01.org > > > > Cc: michael.d.kin...@intel.com; > liming@intel.com > > > > Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: > Ensure > > > safe bitwise > > > > operations. > > > > > > > > On 02/28/18 12:43, Marvin Häuser wrote: > > > [...] > > > > > as edk2 does not support vendor extensions such > as > > > __int128 anyway. > > > > > > > > Not *yet*, I guess :) UEFI 2.7 does list UINT128 > / > > > INT128, in table 5, "Common > > > > UEFI Data Types". I believe those typedefs may > have > > >
Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise operations.
One comment is inline. Thank you in advance, Marvin. > -Original Message- > From: edk2-devel <edk2-devel-boun...@lists.01.org> On Behalf Of Marvin > Häuser > Sent: Wednesday, February 28, 2018 7:46 PM > To: edk2-devel@lists.01.org; Laszlo Ersek <ler...@redhat.com> > Cc: michael.d.kin...@intel.com; liming....@intel.com > Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise > operations. > > I have just locally updated all BIT defines to use the ULL prefix and added > casts to defines using them. > I did that to ensure that 1) inversions always produce the correct value and > 2) > assignments never result in implicit casts to a smaller int, which would > raise a > warning. > > After I was done doing it for MdePkg, a build showed that (N)ASM files > consumed these definitions. > > I only see a bunch of possible solutions to that: > * Prohibit the usage of such defines in assembly code (which I would strongly > dislike). > * Introduce a "DEFINE_BIT" macro which produces one definition for C code > and one for assembly. I only just realized that including C headers was not a NASM feature, but it is actually edk2 invoking the PP. Might the best solution just be to introduce a casting macro, which casts when it's invoked for a C compiler and doesn't when it's invoked for an assembler? Basically would require nothing else than adding a "-D__EDK2_ASSEMBLER__" or something alike to the PP flags when applicable. Any opinion on that? > * Rely on 'ULL' always producing the biggest possible value (including the > 128- > bit range new to the spec) or documenting an exception for it, and insist on > the caller casting (which I would find quite ugly). > * Scrap the patch and continue to rely on compiler-/architecture-specific > behavior, which could cause issues seemingly randomly. > > Thanks, > Marvin. > > > -Original Message- > > From: edk2-devel <edk2-devel-boun...@lists.01.org> On Behalf Of Marvin > > Häuser > > Sent: Wednesday, February 28, 2018 3:21 PM > > To: edk2-devel@lists.01.org; Laszlo Ersek <ler...@redhat.com> > > Cc: michael.d.kin...@intel.com; liming@intel.com > > Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise > > operations. > > > > Hey Laszlo, > > > > I cut your rant because it is not strictly related to this patch. > > However, thank you for composing it nevertheless because it was an > interesting read! > > Comments are inline. > > > > Michael, Liming, > > Do you have any comments regarding the discussion? Thanks in advance. > > > > Best regards, > > Marvin. > > > > > -----Original Message----- > > > From: Laszlo Ersek <ler...@redhat.com> > > > Sent: Wednesday, February 28, 2018 2:57 PM > > > To: Marvin Häuser <marvin.haeu...@outlook.com>; edk2- > > > de...@lists.01.org > > > Cc: michael.d.kin...@intel.com; liming@intel.com > > > Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise > > > operations. > > > > > > On 02/28/18 12:43, Marvin Häuser wrote: > > [...] > > > > as edk2 does not support vendor extensions such as __int128 anyway. > > > > > > Not *yet*, I guess :) UEFI 2.7 does list UINT128 / INT128, in table > > > 5, "Common UEFI Data Types". I believe those typedefs may have been > > added for RISC-V. > > > > Oh yikes, I have not noticed that before. Besides that I wonder how > > that will be implemented by edk2 for non-RISC-V platforms, maybe that > > should be considered? > > As ridiculous as it sounds, maybe some kind of UINT_MAX type (now > > UINT64, later UINT128) should be introduced and any BIT or bitmask > > definition being explicitly casted to that? > > Are BIT definitions or masks occasionally used in preprocessor operations? > > That might break after all. > > Anyway, if that idea would be approved, there really would have to be > > a note regarding this design in some of the EDK2 specifications, > > probably C Code Style. > > > > [...] > > > > > > > -1) The 'truncating constant value' warning would probably need to > > > > be disabled globally, however I don't understand how an explicit > > > > cast is a problem anyway. > > > > > > > > Did I overlook anything contra regarding that? > > > > > > Hmmm... Do you think it could have a performance impact on 32-bit > > > platforms? (I don't think so, at least not in optimized / RELEASE > > > builds.) > > > > I don't think a
Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise operations.
Hey Mike, You are right, the patch was premature because I did not consider any 'incorrect' or 'clever' usages of these definitions. The problem is not primarily undefined behavior, but implementation-defined behavior. Any bitwise operation to a signed integer results in implementation-defined behavior, which compilers usually do not warn about, while well-defined behavior is desirable. Have you read Laszlo's comments? They are quite good at showing up what logics might be and are relied on, which however are not guaranteed to be the case for non-x86 architectures, or even for x86 in case a development team decides to change this behavior some day or a new toolchain not having adopted them in the first place should be added. Furthermore, I don't think inconsistency between the definitions generally is desirable. Thanks, Marvin. > -Original Message- > From: Kinney, Michael D <michael.d.kin...@intel.com> > Sent: Wednesday, February 28, 2018 7:37 PM > To: Marvin Häuser <marvin.haeu...@outlook.com>; edk2- > de...@lists.01.org; Laszlo Ersek <ler...@redhat.com>; Kinney, Michael D > <michael.d.kin...@intel.com> > Cc: Gao, Liming <liming....@intel.com> > Subject: RE: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise > operations. > > Hi Marvin, > > I do not think add 'u' to the BITxx defines does not seem to be a complete > solution. Code can use integer constants in lots of places including other > #defines or inline in expressions. > > If we follow your suggestion wouldn’t we need to add 'u' to every constant > that does not start with a '-' > and might potentially be used with a bit operation? > > Compilers are doing a good job of finding undefined behavior. Isn’t that > sufficient to fix the issues identified? > > Mike > > > -Original Message- > > From: Marvin Häuser [mailto:marvin.haeu...@outlook.com] > > Sent: Wednesday, February 28, 2018 6:21 AM > > To: edk2-devel@lists.01.org; Laszlo Ersek <ler...@redhat.com> > > Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Gao, Liming > > <liming@intel.com> > > Subject: RE: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise > > operations. > > > > Hey Laszlo, > > > > I cut your rant because it is not strictly related to this patch. > > However, thank you for composing it nevertheless because it was an > > interesting read! > > Comments are inline. > > > > Michael, Liming, > > Do you have any comments regarding the discussion? > > Thanks in advance. > > > > Best regards, > > Marvin. > > > > > -Original Message- > > > From: Laszlo Ersek <ler...@redhat.com> > > > Sent: Wednesday, February 28, 2018 2:57 PM > > > To: Marvin Häuser <marvin.haeu...@outlook.com>; edk2- > > > de...@lists.01.org > > > Cc: michael.d.kin...@intel.com; liming@intel.com > > > Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure > > safe bitwise > > > operations. > > > > > > On 02/28/18 12:43, Marvin Häuser wrote: > > [...] > > > > as edk2 does not support vendor extensions such as > > __int128 anyway. > > > > > > Not *yet*, I guess :) UEFI 2.7 does list UINT128 / > > INT128, in table 5, "Common > > > UEFI Data Types". I believe those typedefs may have > > been added for RISC-V. > > > > Oh yikes, I have not noticed that before. Besides that I wonder how > > that will be implemented by edk2 for non- RISC-V platforms, maybe that > > should be considered? > > As ridiculous as it sounds, maybe some kind of UINT_MAX type (now > > UINT64, later UINT128) should be introduced and any BIT or bitmask > > definition being explicitly casted to that? > > Are BIT definitions or masks occasionally used in preprocessor > > operations? That might break after all. > > Anyway, if that idea would be approved, there really would have to be > > a note regarding this design in some of the EDK2 specifications, > > probably C Code Style. > > > > [...] > > > > > > > -1) The 'truncating constant value' warning would > > probably need to be > > > > disabled globally, however I don't understand how > > an explicit cast is > > > > a problem anyway. > > > > > > > > Did I overlook anything contra regarding that? > > > > > > Hmmm... Do you think it could have a performance > > impact on 32-bit > > > platforms? (I don't think so, at least not in > > optimized / RELEASE > > > builds.) > > > > I don't think any proper optimizer would not optimize this. After all, > > it can not only evaluate the value directly and notice that the value > > does not reach into the 'long long range', but also consider the type > > of the other operand. > > > > [...] ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise operations.
I have just locally updated all BIT defines to use the ULL prefix and added casts to defines using them. I did that to ensure that 1) inversions always produce the correct value and 2) assignments never result in implicit casts to a smaller int, which would raise a warning. After I was done doing it for MdePkg, a build showed that (N)ASM files consumed these definitions. I only see a bunch of possible solutions to that: * Prohibit the usage of such defines in assembly code (which I would strongly dislike). * Introduce a "DEFINE_BIT" macro which produces one definition for C code and one for assembly. * Rely on 'ULL' always producing the biggest possible value (including the 128-bit range new to the spec) or documenting an exception for it, and insist on the caller casting (which I would find quite ugly). * Scrap the patch and continue to rely on compiler-/architecture-specific behavior, which could cause issues seemingly randomly. Thanks, Marvin. > -Original Message- > From: edk2-devel <edk2-devel-boun...@lists.01.org> On Behalf Of Marvin > Häuser > Sent: Wednesday, February 28, 2018 3:21 PM > To: edk2-devel@lists.01.org; Laszlo Ersek <ler...@redhat.com> > Cc: michael.d.kin...@intel.com; liming....@intel.com > Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise > operations. > > Hey Laszlo, > > I cut your rant because it is not strictly related to this patch. However, > thank > you for composing it nevertheless because it was an interesting read! > Comments are inline. > > Michael, Liming, > Do you have any comments regarding the discussion? Thanks in advance. > > Best regards, > Marvin. > > > -Original Message- > > From: Laszlo Ersek <ler...@redhat.com> > > Sent: Wednesday, February 28, 2018 2:57 PM > > To: Marvin Häuser <marvin.haeu...@outlook.com>; edk2- > > de...@lists.01.org > > Cc: michael.d.kin...@intel.com; liming@intel.com > > Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise > > operations. > > > > On 02/28/18 12:43, Marvin Häuser wrote: > [...] > > > as edk2 does not support vendor extensions such as __int128 anyway. > > > > Not *yet*, I guess :) UEFI 2.7 does list UINT128 / INT128, in table 5, > > "Common UEFI Data Types". I believe those typedefs may have been > added for RISC-V. > > Oh yikes, I have not noticed that before. Besides that I wonder how that will > be implemented by edk2 for non-RISC-V platforms, maybe that should be > considered? > As ridiculous as it sounds, maybe some kind of UINT_MAX type (now > UINT64, later UINT128) should be introduced and any BIT or bitmask > definition being explicitly casted to that? > Are BIT definitions or masks occasionally used in preprocessor operations? > That might break after all. > Anyway, if that idea would be approved, there really would have to be a > note regarding this design in some of the EDK2 specifications, probably C > Code Style. > > [...] > > > > > -1) The 'truncating constant value' warning would probably need to > > > be disabled globally, however I don't understand how an explicit > > > cast is a problem anyway. > > > > > > Did I overlook anything contra regarding that? > > > > Hmmm... Do you think it could have a performance impact on 32-bit > > platforms? (I don't think so, at least not in optimized / RELEASE > > builds.) > > I don't think any proper optimizer would not optimize this. After all, it can > not > only evaluate the value directly and notice that the value does not reach into > the 'long long range', but also consider the type of the other operand. > > [...] > > ___ > 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
Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise operations.
Hi Marvin, I do not think add 'u' to the BITxx defines does not seem to be a complete solution. Code can use integer constants in lots of places including other #defines or inline in expressions. If we follow your suggestion wouldn’t we need to add 'u' to every constant that does not start with a '-' and might potentially be used with a bit operation? Compilers are doing a good job of finding undefined behavior. Isn’t that sufficient to fix the issues identified? Mike > -Original Message- > From: Marvin Häuser [mailto:marvin.haeu...@outlook.com] > Sent: Wednesday, February 28, 2018 6:21 AM > To: edk2-devel@lists.01.org; Laszlo Ersek > <ler...@redhat.com> > Cc: Kinney, Michael D <michael.d.kin...@intel.com>; > Gao, Liming <liming....@intel.com> > Subject: RE: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure > safe bitwise operations. > > Hey Laszlo, > > I cut your rant because it is not strictly related to > this patch. However, thank you for composing it > nevertheless because it was an interesting read! > Comments are inline. > > Michael, Liming, > Do you have any comments regarding the discussion? > Thanks in advance. > > Best regards, > Marvin. > > > -Original Message- > > From: Laszlo Ersek <ler...@redhat.com> > > Sent: Wednesday, February 28, 2018 2:57 PM > > To: Marvin Häuser <marvin.haeu...@outlook.com>; edk2- > > de...@lists.01.org > > Cc: michael.d.kin...@intel.com; liming@intel.com > > Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure > safe bitwise > > operations. > > > > On 02/28/18 12:43, Marvin Häuser wrote: > [...] > > > as edk2 does not support vendor extensions such as > __int128 anyway. > > > > Not *yet*, I guess :) UEFI 2.7 does list UINT128 / > INT128, in table 5, "Common > > UEFI Data Types". I believe those typedefs may have > been added for RISC-V. > > Oh yikes, I have not noticed that before. Besides that > I wonder how that will be implemented by edk2 for non- > RISC-V platforms, maybe that should be considered? > As ridiculous as it sounds, maybe some kind of UINT_MAX > type (now UINT64, later UINT128) should be introduced > and any BIT or bitmask definition being explicitly > casted to that? > Are BIT definitions or masks occasionally used in > preprocessor operations? That might break after all. > Anyway, if that idea would be approved, there really > would have to be a note regarding this design in some > of the EDK2 specifications, probably C Code Style. > > [...] > > > > > -1) The 'truncating constant value' warning would > probably need to be > > > disabled globally, however I don't understand how > an explicit cast is > > > a problem anyway. > > > > > > Did I overlook anything contra regarding that? > > > > Hmmm... Do you think it could have a performance > impact on 32-bit > > platforms? (I don't think so, at least not in > optimized / RELEASE > > builds.) > > I don't think any proper optimizer would not optimize > this. After all, it can not only evaluate the value > directly and notice that the value does not reach into > the 'long long range', but also consider the type of > the other operand. > > [...] ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise operations.
Hey Laszlo, I cut your rant because it is not strictly related to this patch. However, thank you for composing it nevertheless because it was an interesting read! Comments are inline. Michael, Liming, Do you have any comments regarding the discussion? Thanks in advance. Best regards, Marvin. > -Original Message- > From: Laszlo Ersek <ler...@redhat.com> > Sent: Wednesday, February 28, 2018 2:57 PM > To: Marvin Häuser <marvin.haeu...@outlook.com>; edk2- > de...@lists.01.org > Cc: michael.d.kin...@intel.com; liming....@intel.com > Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise > operations. > > On 02/28/18 12:43, Marvin Häuser wrote: [...] > > as edk2 does not support vendor extensions such as __int128 anyway. > > Not *yet*, I guess :) UEFI 2.7 does list UINT128 / INT128, in table 5, "Common > UEFI Data Types". I believe those typedefs may have been added for RISC-V. Oh yikes, I have not noticed that before. Besides that I wonder how that will be implemented by edk2 for non-RISC-V platforms, maybe that should be considered? As ridiculous as it sounds, maybe some kind of UINT_MAX type (now UINT64, later UINT128) should be introduced and any BIT or bitmask definition being explicitly casted to that? Are BIT definitions or masks occasionally used in preprocessor operations? That might break after all. Anyway, if that idea would be approved, there really would have to be a note regarding this design in some of the EDK2 specifications, probably C Code Style. [...] > > > -1) The 'truncating constant value' warning would probably need to be > > disabled globally, however I don't understand how an explicit cast is > > a problem anyway. > > > > Did I overlook anything contra regarding that? > > Hmmm... Do you think it could have a performance impact on 32-bit > platforms? (I don't think so, at least not in optimized / RELEASE > builds.) I don't think any proper optimizer would not optimize this. After all, it can not only evaluate the value directly and notice that the value does not reach into the 'long long range', but also consider the type of the other operand. [...] ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise operations.
On 02/28/18 14:57, Laszlo Ersek wrote: > On 02/28/18 12:43, Marvin Häuser wrote: >> +2) Binary operations (AND, OR, ...) should not raise any problems at >> all (except for our fellow using VS2005x86 :) ) > > Haha :) In earnest though, you are right. Hm... It should not be a frequent pattern, but: (BIT5 | BIT3) << 2 might no longer build; we might have to use LShiftU64() instead. Laszlo ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise operations.
On 02/28/18 12:43, Marvin Häuser wrote:
> Actually, your explanations and the rest of the bit defines made me
> wonder, whether marking all BIT defines and bit masks of any kind,
> anywhere, as ULL, might be the best solution.
For a new project just being started, that could be one of the safest /
wisest choices. :)
For edk2, about the only counter-argument I'd have right now is that
many assignments and function arguments could suddenly require
down-casts.
> +1) Inversion can be used for any integer size uncasted,
Right, you could just use ~BITn, and truncate it to whatever unsigned
type is needed.
> as edk2 does not support vendor extensions such as __int128 anyway.
Not *yet*, I guess :) UEFI 2.7 does list UINT128 / INT128, in table 5,
"Common UEFI Data Types". I believe those typedefs may have been added
for RISC-V.
> +2) Binary operations (AND, OR, ...) should not raise any problems at
> all (except for our fellow using VS2005x86 :) )
Haha :) In earnest though, you are right.
> +3) Assignments and passing-by-arguments will result in compiler-time
> errors when not done properly in the sense of this patch, which
> eliminates the need to audit all usages manually, but just compile the
> entire codebase and fix the errors one-by-one (which is of course
> still not a quick task, but if the package authors agree with the
> proposal, I might attempt it).
Good point; it didn't occur to me that a large number of the "client
sites" would be flagged by the compiler at once.
... I've just remembered another possible problem: I think some EBC
compiler cannot use 64-bit integers as case labels in switch statements.
See commit ada385843b94 ("MdeModulePkg/PciBus: Change switch-case to
if-else to fix EBC build", 2018-01-10). Some case labels currently built
of BIT[0..31] macros could break in EBC builds. OTOH, the (EBC) compiler
would catch those as well; no silent breakage.
> -1) The 'truncating constant value' warning would probably need to be
> disabled globally, however I don't understand how an explicit cast is
> a problem anyway.
>
> Did I overlook anything contra regarding that?
Hmmm... Do you think it could have a performance impact on 32-bit
platforms? (I don't think so, at least not in optimized / RELEASE
builds.)
>> (3) The clever code: is aware that BIT macros are (mostly) signed,
>> and exploits that fact for various ends.
>> If we don't think (3) is a real risk, I'm fine with the approach of
>> these patches. (I don't think I'll be able to send a real R-b for
>> them, because that would require me to evaluate all uses, and that's
>> a Herculean task I just can't take on; apologies.)
>
> I hope that the 'all ULL' proposal uncovers them all. Would there be
> cases, where no error would be raised?
Dunno :) I quite like the idea!
> -#define ENCODE_ERROR(StatusCode) ((RETURN_STATUS)(MAX_BIT |
(StatusCode)))
> +#define ENCODE_ERROR(StatusCode) ((RETURN_STATUS)(MAX_BIT
>> |
(StatusCode##ULL)))
>> Given the current definition, if StatusCode is a signed integer, then
>> one way or another, it will be converted to (not reinterpreted as) an
>> unsigned integer type. This is due to the rules on the bitwise OR
>> operator and the outermost cast (thank you for reminding me of that).
>> Such conversions are fully defined in the C standard, they are not
>> implementation-defined.
>
> Thanks for noting. Would you still append the U-prefix for readability
> / making the intention clear?
I think I'd document the requirement that StatusCode have an unsigned
integer type, and then I'd update the ENCODE_ERROR() macro invocations
to conform.
>> Is your point that we shouldn't even assume INT32=="int" and
>> UINT32=="unsigned int"?
>
> Yes, pretty much. I don't think it does hurt either. While it is
> certainly a very ugly definition, that's my preferred use for macros
> anyway - hide ugly code behind a lovely name.
>
>>
>> I think that would be a good point for a highly portable C codebase
>> that was written from the ground up without type assumptions. But I
>> don't think it applies to edk2. If UINT32 mapped to "unsigned short"
>> and INT64 mapped to "int", edk2 would break in a thousand places
>> immediately.
>
> Of course it would, however I imagined a header file that is included
> in every file for every architecture to be as close to flawless as
> possible.
> While of course I would also fix such assumptions in executable code
> if I found, and hope the maintainers care enough to review and merge,
> I consider header files a greater danger as that 'flawed' (in an
> idealistic sense) macro is expanded into external code, which might
> cause trouble while debugging, as the consumer assumes the mistake is
> within his code.
> Headers >>> library code > module code, in my opinion, especially for
> the most central one.
I'll defer to the MdePkg maintainers on this.
>> Personally I think that's a laudable goal, and I agree that all
>> portable C code should be written
Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise operations.
Thanks. Comments are inline.
Best regards,
Marvin.
> -Original Message-
> From: Laszlo Ersek <ler...@redhat.com>
> Sent: Wednesday, February 28, 2018 12:00 PM
> To: Marvin Häuser <marvin.haeu...@outlook.com>; edk2-
> de...@lists.01.org
> Cc: michael.d.kin...@intel.com; liming....@intel.com
> Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise
> operations.
>
> On 02/27/18 21:31, Marvin Häuser wrote:
> >> -Original Message-
> >> From: Laszlo Ersek <ler...@redhat.com>
> >> Sent: Tuesday, February 27, 2018 8:54 PM
> >> To: Marvin Häuser <marvin.haeu...@outlook.com>; edk2-
> >> de...@lists.01.org
> >> Cc: michael.d.kin...@intel.com; liming@intel.com
> >> Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise
> >> operations.
> >>
[...]
>
> This is exactly how I feel, yes. My concern is that making the change now
> runs the risk of tricky regressions that we might not even prevent with a
> detailed audit.
Actually, your explanations and the rest of the bit defines made me wonder,
whether marking all BIT defines and bit masks of any kind, anywhere, as ULL,
might be the best solution.
+1) Inversion can be used for any integer size uncasted, as edk2 does not
support vendor extensions such as __int128 anyway.
+2) Binary operations (AND, OR, ...) should not raise any problems at all
(except for our fellow using VS2005x86 :) )
+3) Assignments and passing-by-arguments will result in compiler-time errors
when not done properly in the sense of this patch, which eliminates the need to
audit all usages manually, but just compile the entire codebase and fix the
errors one-by-one (which is of course still not a quick task, but if the
package authors agree with the proposal, I might attempt it).
-1) The 'truncating constant value' warning would probably need to be disabled
globally, however I don't understand how an explicit cast is a problem anyway.
Did I overlook anything contra regarding that?
>
> Anyway, I don't want to spread FUD about this -- I think the goal of these
> changes is good. It's up to the MdePkg maintainers to evaluate the risks, I
> just wanted us to be aware of them. Once we reach an end state where all
> the BIT, SIZE and BASE macros are unsigned, and no regressions are found (or
> none remain), that's for the best!
Nah, your comments are great, thanks!
[...]
> I think I agree with your assessment, considering the usual application of
> these macros in source code. I distinguish three kinds of uses:
>
[...]
>
> (3) The clever code: is aware that BIT macros are (mostly) signed, and
> exploits that fact for various ends.
>
> I agree that most of the edk2 codebase should fall under (1).
>
> As you may expect, I personally write (2), and code like (2) should not worry
> about BIT / SIZE / BASE becoming unsigned.
>
> My concern is (3). There have been examples in core edk2 modules that
> explicitly relied on undefined behavior, such as left shifts of negative
> integers
> and such. We've only slowly fixed them up after compilers / analyzers started
> flagging them.
>
> If we don't think (3) is a real risk, I'm fine with the approach of these
> patches.
> (I don't think I'll be able to send a real R-b for them, because that would
> require me to evaluate all uses, and that's a Herculean task I just can't take
> on; apologies.)
I hope that the 'all ULL' proposal uncovers them all. Would there be cases,
where no error would be raised?
[...]
>
> >>> -#define ENCODE_ERROR(StatusCode) ((RETURN_STATUS)(MAX_BIT |
> >> (StatusCode)))
> >>> +#define ENCODE_ERROR(StatusCode) ((RETURN_STATUS)(MAX_BIT
> |
> >> (StatusCode##ULL)))
> >>>
> >>
> >> MAX_BIT always has type UINTN (UINT64 aka "unsigned long long" on
> >> 64-bit platforms, and UINT32 aka "unsigned int" on 32-bit platforms).
> >> This means that ENCODE_ERROR results in a UINTN, right now.
> >
> > Good point, I didn't keep that in mind. It probably should be solely
> > 'U'.
>
> In fact there's no need even for that: because MAX_BIT has type UINTN
> ("unsigned long long" or "unsigned int"), the StatusCode macro argument,
> which is expected to be a small nonnegative "int", will be converted to
> UINTN, for the bitwise OR operator.
Oh, right, thanks. I would still prefer it to be explicit, but I'll wait for a
maintainer's comment.
[...]
>
> What is implementation-defined in the current definition of
> ENCODE_ERROR()?
That was a result of the misunderstanding above, scrap that.
>
> Given the current definition, if StatusCode is a s
Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise operations.
On 02/27/18 21:31, Marvin Häuser wrote:
>> -Original Message-
>> From: Laszlo Ersek <ler...@redhat.com>
>> Sent: Tuesday, February 27, 2018 8:54 PM
>> To: Marvin Häuser <marvin.haeu...@outlook.com>; edk2-
>> de...@lists.01.org
>> Cc: michael.d.kin...@intel.com; liming....@intel.com
>> Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise
>> operations.
>>
>> On 02/27/18 17:47, Marvin Häuser wrote:
[...]
>> I think this change is a good idea *in theory* (arguably the macros
>> should have been introduced like this in the first place). However,
>> at this point, quite a bit of code could silently see its meaning
>> changed.
>
> You are right that likely at least some vendor has code that relies on
> the defines being signed. However, you seem to agree that bit
> definitions are naturally assumed to be unsigned and the same as this
> change could break code, not making the change could lead to
> non-functioning code that looks valid at first sight.
This is exactly how I feel, yes. My concern is that making the change
now runs the risk of tricky regressions that we might not even prevent
with a detailed audit.
Anyway, I don't want to spread FUD about this -- I think the goal of
these changes is good. It's up to the MdePkg maintainers to evaluate the
risks, I just wanted us to be aware of them. Once we reach an end state
where all the BIT, SIZE and BASE macros are unsigned, and no regressions
are found (or none remain), that's for the best!
> Regarding your comment on the 'truncating patches', saying the code
> relies on the definitions being signed: I honestly doubt the code was
> intentionally written with that in mind, but rather intuitively and it
> happened to work. I might be wrong of course.
I think I agree with your assessment, considering the usual application
of these macros in source code. I distinguish three kinds of uses:
(1) The trusting / maybe "naive" client code: silently expects the BIT
macros to be unsigned, resultant code happens to work; when it
doesn't, it is fixed up with individual patches.
(2) The paranoid code: is aware that BIT macros are not (all) unsigned,
believes that they should be unsigned, casts BIT macro applications
to unsigned types locally, all further manipulation is done with
unsigned types.
(3) The clever code: is aware that BIT macros are (mostly) signed, and
exploits that fact for various ends.
I agree that most of the edk2 codebase should fall under (1).
As you may expect, I personally write (2), and code like (2) should not
worry about BIT / SIZE / BASE becoming unsigned.
My concern is (3). There have been examples in core edk2 modules that
explicitly relied on undefined behavior, such as left shifts of negative
integers and such. We've only slowly fixed them up after compilers /
analyzers started flagging them.
If we don't think (3) is a real risk, I'm fine with the approach of
these patches. (I don't think I'll be able to send a real R-b for them,
because that would require me to evaluate all uses, and that's a
Herculean task I just can't take on; apologies.)
>> Let me give you a pathologic example (it *does* occur in the wild,
>> although maybe not in edk2):
>>
>> {
>> UINT64 MyMask;
>>
>> MyMask = ~BIT0;
>> }
>>
>> Before your patch, MyMask is assigned the value
>> 0x___FFFE.
>>
>> This is because:
>>
>> - BIT0 has type "int" and value 1
>>
>> - Due to our (implementation-defined) representation of negative
>> integers, namely two's complement, ~BIT0 (also having type "int")
>> stands for value (- 2).
>
> There is pretty much where my issue is. I don't think triggering
> implementation-defined behavior is a good idea, not to speak about
> relying on it. EDK no longer targets solely x86, there is even a new
> architecture (Arch-V or something?) on the way, so I consider relying
> on compiler specifics very dangerous.
Oh I totally agree about that! As I said, the goal of the patches is
great, I absolutely agree about it; my concern is, will we find tricks
like the above by source code audit, or by a series of functional
regressions, drawn out over time?
[...]
>> My point is that such innocent-looking (and otherwise really good!)
>> changes for *central* macros require a large audit effort.
>
> Indeed that audit effort is required. How would that be handled for
> non-edk2 code anyway? Might there be a note in the UDK changelog, if
> accepted?
Very good question, and I don't have the slightest idea.
Considering the Linux kernel as an example, if your stuff is "out of
tree", you are on your own, when incompatible changes are committed. (If
you are "in-tree", t
Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise operations.
Hey Laszlo,
Thank you very much for your review, again. Comments are inline.
Regards,
Marvin.
> -Original Message-
> From: Laszlo Ersek <ler...@redhat.com>
> Sent: Tuesday, February 27, 2018 8:54 PM
> To: Marvin Häuser <marvin.haeu...@outlook.com>; edk2-
> de...@lists.01.org
> Cc: michael.d.kin...@intel.com; liming....@intel.com
> Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise
> operations.
>
> On 02/27/18 17:47, Marvin Häuser wrote:
> > As per the C standard, bit-level operations on signed integers are
> > either undefined or implementation-defined. Hence, mark all BIT
> > defines and shifts as unsigned to safely allow such operations.
>
> Sigh, this is why threading is important in patch sets. :/ This patch comes
> first,
> and it sort of invalidates my review for patch #2 in this series.
>
> I think this change is a good idea *in theory* (arguably the macros should
> have been introduced like this in the first place). However, at this point,
> quite
> a bit of code could silently see its meaning changed.
You are right that likely at least some vendor has code that relies on the
defines being signed. However, you seem to agree that bit definitions are
naturally assumed to be unsigned and the same as this change could break code,
not making the change could lead to non-functioning code that looks valid at
first sight.
Regarding your comment on the 'truncating patches', saying the code relies on
the definitions being signed: I honestly doubt the code was intentionally
written with that in mind, but rather intuitively and it happened to work. I
might be wrong of course.
>
> Let me give you a pathologic example (it *does* occur in the wild, although
> maybe not in edk2):
>
> {
> UINT64 MyMask;
>
> MyMask = ~BIT0;
> }
>
> Before your patch, MyMask is assigned the value 0x___FFFE.
>
> This is because:
>
> - BIT0 has type "int" and value 1
>
> - Due to our (implementation-defined) representation of negative integers,
> namely two's complement, ~BIT0 (also having type "int") stands for value (-
> 2).
There is pretty much where my issue is. I don't think triggering
implementation-defined behavior is a good idea, not to speak about relying on
it. EDK no longer targets solely x86, there is even a new architecture (Arch-V
or something?) on the way, so I consider relying on compiler specifics very
dangerous.
>
> - When this value is converted to UINT64 ("unsigned long long"), we get
> 18446744073709551615 + 1 + (-2) == 18446744073709551614. That's
> 0x___FFFE, which is the mask value that we intended.
>
> (An alternative way people sometimes put this is that ~BIT0 is "sign
> extended". I strongly dislike this term when speaking about C. It is
> appropriate for (x86?) assembly language, but not for C, in my opinion.)
>
>
> In the above example, the "clever" programmer consciously exploited the
> two's complement representation for the bit-neg, and the conversion of the
> negative value to "unsigned long long" (as dictated by the standard).
>
>
> Now, if you change BIT0 to have type "unsigned int" (aka UINT32), then
> ~BIT0 will have value 4294967294 (0x_FFFE), and type "unsigned int".
>
> When this ~BIT0 is converted to UINT64, the value will be preserved, and
> MyMask will end up with 0x___FFFE. Likely not the mask that
> the clever programmer intended.
>
> (An alternative way to state this is that ~BIT0 is "zero extended".
> Again, I think this is inappropriate for C language discussion; it's fine for
> (x86?)
> assembly.)
>
> My point is that such innocent-looking (and otherwise really good!) changes
> for *central* macros require a large audit effort.
Indeed that audit effort is required. How would that be handled for non-edk2
code anyway? Might there be a note in the UDK changelog, if accepted?
>
> More comments below:
>
> > For the SIGNATURE macros, add several casts to account for int
> > promotions, which might be signed.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Marvin Haeuser <marvin.haeu...@outlook.com>
> > ---
> > MdePkg/Include/Base.h | 160 ++--
> > 1 file changed, 80 insertions(+), 80 deletions(-)
> >
> > diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h index
> > a94182f08886..f108ed92eb0b 100644
> > --- a/MdePkg/Include/Base.h
> > +++ b/MdePkg/Include/Base.h
> > @@ -404,38 +404,38 @@ struct _LIST_ENTRY { #define MIN_INT32
> > (((INT32) -2147483647) - 1) #define MIN_INT64 (((INT64)
> > -9223372
Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise operations.
On 02/27/18 17:47, Marvin Häuser wrote:
> As per the C standard, bit-level operations on signed integers are
> either undefined or implementation-defined. Hence, mark all BIT
> defines and shifts as unsigned to safely allow such operations.
Sigh, this is why threading is important in patch sets. :/ This patch
comes first, and it sort of invalidates my review for patch #2 in this
series.
I think this change is a good idea *in theory* (arguably the macros
should have been introduced like this in the first place). However, at
this point, quite a bit of code could silently see its meaning changed.
Let me give you a pathologic example (it *does* occur in the wild,
although maybe not in edk2):
{
UINT64 MyMask;
MyMask = ~BIT0;
}
Before your patch, MyMask is assigned the value 0x___FFFE.
This is because:
- BIT0 has type "int" and value 1
- Due to our (implementation-defined) representation of negative
integers, namely two's complement, ~BIT0 (also having type "int") stands
for value (-2).
- When this value is converted to UINT64 ("unsigned long long"), we get
18446744073709551615 + 1 + (-2) == 18446744073709551614. That's
0x___FFFE, which is the mask value that we intended.
(An alternative way people sometimes put this is that ~BIT0 is "sign
extended". I strongly dislike this term when speaking about C. It is
appropriate for (x86?) assembly language, but not for C, in my opinion.)
In the above example, the "clever" programmer consciously exploited the
two's complement representation for the bit-neg, and the conversion of
the negative value to "unsigned long long" (as dictated by the standard).
Now, if you change BIT0 to have type "unsigned int" (aka UINT32), then
~BIT0 will have value 4294967294 (0x_FFFE), and type "unsigned int".
When this ~BIT0 is converted to UINT64, the value will be preserved, and
MyMask will end up with 0x___FFFE. Likely not the mask that
the clever programmer intended.
(An alternative way to state this is that ~BIT0 is "zero extended".
Again, I think this is inappropriate for C language discussion; it's
fine for (x86?) assembly.)
My point is that such innocent-looking (and otherwise really good!)
changes for *central* macros require a large audit effort.
More comments below:
> For the SIGNATURE macros, add several casts to account for int
> promotions, which might be signed.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marvin Haeuser
> ---
> MdePkg/Include/Base.h | 160 ++--
> 1 file changed, 80 insertions(+), 80 deletions(-)
>
> diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h
> index a94182f08886..f108ed92eb0b 100644
> --- a/MdePkg/Include/Base.h
> +++ b/MdePkg/Include/Base.h
> @@ -404,38 +404,38 @@ struct _LIST_ENTRY {
> #define MIN_INT32 (((INT32) -2147483647) - 1)
> #define MIN_INT64 (((INT64) -9223372036854775807LL) - 1)
>
> -#define BIT0 0x0001
> -#define BIT1 0x0002
> -#define BIT2 0x0004
> -#define BIT3 0x0008
> -#define BIT4 0x0010
> -#define BIT5 0x0020
> -#define BIT6 0x0040
> -#define BIT7 0x0080
> -#define BIT8 0x0100
> -#define BIT9 0x0200
> -#define BIT100x0400
> -#define BIT110x0800
> -#define BIT120x1000
> -#define BIT130x2000
> -#define BIT140x4000
> -#define BIT150x8000
> -#define BIT160x0001
> -#define BIT170x0002
> -#define BIT180x0004
> -#define BIT190x0008
> -#define BIT200x0010
> -#define BIT210x0020
> -#define BIT220x0040
> -#define BIT230x0080
> -#define BIT240x0100
> -#define BIT250x0200
> -#define BIT260x0400
> -#define BIT270x0800
> -#define BIT280x1000
> -#define BIT290x2000
> -#define BIT300x4000
> -#define BIT310x8000
> +#define BIT0 0x0001U
> +#define BIT1 0x0002U
> +#define BIT2 0x0004U
> +#define BIT3 0x0008U
> +#define BIT4 0x0010U
> +#define BIT5 0x0020U
> +#define BIT6 0x0040U
> +#define BIT7 0x0080U
> +#define BIT8 0x0100U
> +#define BIT9 0x0200U
> +#define BIT100x0400U
> +#define BIT110x0800U
> +#define BIT120x1000U
> +#define BIT130x2000U
> +#define BIT140x4000U
> +#define BIT150x8000U
> +#define BIT160x0001U
> +#define BIT170x0002U
> +#define BIT180x0004U
> +#define BIT190x0008U
> +#define BIT200x0010U
> +#define BIT210x0020U
> +#define BIT220x0040U
> +#define BIT230x0080U
> +#define BIT240x0100U
> +#define BIT250x0200U
> +#define BIT260x0400U
> +#define BIT270x0800U
> +#define BIT280x1000U
> +#define BIT290x2000U
> +#define BIT30
