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

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

One comment is inline.

Thank you in advance,
Marvin.

> -----Original Message-----
> From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Marvin
> Häuser
> Sent: Wednesday, February 28, 2018 7:46 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.
> 
> 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.

I only just realized that including C headers was not a NASM feature, but it is actually edk2 invoking the PP.
Might the best solution just be to introduce a casting macro, which casts when it's invoked for a C compiler and doesn't when it's invoked for an assembler?
Basically would require nothing else than adding a "-D__EDK2_ASSEMBLER__" or something alike to the PP flags when applicable.

Any opinion on that?

> * 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
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel