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

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

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Thursday, March 1, 2018 11:40 AM
> 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 22:07, Marvin Häuser wrote:
> > 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?
> 
> Sigh, I don't know what to answer. On one hand (if we can get it to work
> without regressions) I like the idea of making all BITx macros ULL. On the
> other hand, defining the same macro with different replacement text,
> dependent on whether the including source code is assembly or C, looks
> dirty. I can't really put my finger on it, but I feel such dual definitions could
> cause issues or confusion. If BaseTools people are OK with the dual
> definition, I guess I could live with it.

Indeed it is dirty, however I don't think there is any choice but the smallest devil.
Leaving them signed might become dangerous, relying on suffixes is not a proper solution considering the new 128-bit type and casting results in the sharing issue between C and NASM.
Actually I would abandon the "two definitions" concept as of the idea of introducing __EDK2_ASSEMBLER__.

The solution I think would be the best to ensure a safe and forward-compatible is:
1) Cast all generic defines that might be used as masks to the highest available integer type (macro), including BITx.
2) Introduce a casting macro which would roughly look like this and apply it to all "named bit" definitions:

#ifdef __EDK2_ASSEMLER__
  #define PP_CAST(Value, Type) (Value)
#else
  #define PP_CAST(Value, Type) ((Type)(Value))
#endif

This way:
* Bit operations on all types of unsigned integers are safe and well-defined.
* One can intuitively use inverses for both generic and "named" masks.
* One can continue to intuitively assign "named bits" to variables of their type (except for when integer promotion happens as part of an OP, of course, but this is unrelated).
* Code not casting correctly will raise compile-time errors.

The only alternative worth arguing I see is scrapping it all and introducing Unit Tests. However, should a Unit Test ever fail a specific compiler, we would be back here again.

Regards,
Marvin.

> 
> Thanks,
> Laszlo
> 
> >
> >> * 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