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

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

I have just locally updated all BIT defines to use the ULL prefix and added casts to defines using them.
I did that to ensure that 1) inversions always produce the correct value and 2) assignments never result in implicit casts to a smaller int, which would raise a warning.

After I was done doing it for MdePkg, a build showed that (N)ASM files consumed these definitions.

I only see a bunch of possible solutions to that:
* Prohibit the usage of such defines in assembly code (which I would strongly dislike).
* Introduce a "DEFINE_BIT" macro which produces one definition for C code and one for assembly.
* Rely on 'ULL' always producing the biggest possible value (including the 128-bit range new to the spec) or documenting an exception for it, and insist on the caller casting (which I would find quite ugly).
* Scrap the patch and continue to rely on compiler-/architecture-specific behavior, which could cause issues seemingly randomly.

Thanks,
Marvin.

> -----Original Message-----
> From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Marvin
> Häuser
> Sent: Wednesday, February 28, 2018 3:21 PM
> To: edk2-devel@lists.01.org; Laszlo Ersek <lersek@redhat.com>
> Cc: michael.d.kinney@intel.com; 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