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

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

> -----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.
> > > >
> > > > [...]