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

[Laszlo Ersek <lersek@redhat.com>]

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.

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