Re: [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise operations.

["Marvin Häuser" <Marvin.Haeuser@outlook.com>]

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.kinney@intel.com>
> Sent: Thursday, March 1, 2018 6:19 PM
> To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2-
> devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: lersek@redhat.com; Gao, Liming <liming.gao@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 = 0xffff
> } __VERIFY_UINT16_ENUM_SIZE;
> 
> typedef enum {
>   __VerifyUint32EnumValue = 0xffffffff
> } __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-
> > bounces@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.kinney@intel.com>
> > Cc: lersek@redhat.com; Gao, Liming
> > <liming.gao@intel.com>
> > Subject: Re: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure safe bitwise
> > operations.
> >
> >
> > > -----Original Message-----
> > > From: Kinney, Michael D <michael.d.kinney@intel.com>
> > > Sent: Thursday, March 1, 2018 2:42 AM
> > > To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2-
> > > devel@lists.01.org; Kinney, Michael D
> > <michael.d.kinney@intel.com>
> > > Cc: lersek@redhat.com; Gao, Liming
> > <liming.gao@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.Haeuser@outlook.com]
> > > > Sent: Wednesday, February 28, 2018 10:52 AM
> > > > To: edk2-devel@lists.01.org; Kinney, Michael D
> > > > <michael.d.kinney@intel.com>
> > > > Cc: lersek@redhat.com; Gao, Liming <liming.gao@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.kinney@intel.com>
> > > > > Sent: Wednesday, February 28, 2018 7:37 PM
> > > > > To: Marvin Häuser <Marvin.Haeuser@outlook.com>;
> > edk2-
> > > > > devel@lists.01.org; Laszlo Ersek
> > <lersek@redhat.com>;
> > > > Kinney, Michael D
> > > > > <michael.d.kinney@intel.com>
> > > > > Cc: Gao, Liming <liming.gao@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.Haeuser@outlook.com]
> > > > > > Sent: Wednesday, February 28, 2018 6:21 AM
> > > > > > To: edk2-devel@lists.01.org; Laszlo Ersek
> > > > <lersek@redhat.com>
> > > > > > Cc: Kinney, Michael D
> > <michael.d.kinney@intel.com>;
> > > > Gao, Liming
> > > > > > <liming.gao@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 <lersek@redhat.com>
> > > > > > > Sent: Wednesday, February 28, 2018 2:57 PM
> > > > > > > To: Marvin Häuser
> > <Marvin.Haeuser@outlook.com>;
> > > > edk2-
> > > > > > > devel@lists.01.org
> > > > > > > Cc: michael.d.kinney@intel.com;
> > > > liming.gao@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