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

["Kinney, Michael D" <michael.d.kinney@intel.com>]

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