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

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

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