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

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

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