Hi Marvin, Yes. I have been reading the thread.
Lots of really good analysis. However, it does not make sense to me to add 'u' to the smaller BITxx, BASExx, and SIZExx macros if we have constants in other places that do not add the 'u'. I think this patch may increase the inconsistency of the whole tree. If we don’t make this change, what types of issues do we need to fix and what would the fix entail? Mike > -----Original Message----- > From: Marvin Häuser [mailto:marvin.haeu...@outlook.com] > Sent: Wednesday, February 28, 2018 10:52 AM > To: edk2-devel@lists.01.org; Kinney, Michael D > <michael.d.kin...@intel.com> > Cc: ler...@redhat.com; Gao, Liming > <liming....@intel.com> > Subject: RE: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure > safe bitwise operations. > > Hey Mike, > > You are right, the patch was premature because I did > not consider any 'incorrect' or 'clever' usages of > these definitions. > The problem is not primarily undefined behavior, but > implementation-defined behavior. > Any bitwise operation to a signed integer results in > implementation-defined behavior, which compilers > usually do not warn about, while well-defined behavior > is desirable. > > Have you read Laszlo's comments? They are quite good at > showing up what logics might be and are relied on, > which however are not guaranteed to be the case for > non-x86 architectures, > or even for x86 in case a development team decides to > change this behavior some day or a new toolchain not > having adopted them in the first place should be added. > > Furthermore, I don't think inconsistency between the > definitions generally is desirable. > > Thanks, > Marvin. > > > -----Original Message----- > > From: Kinney, Michael D <michael.d.kin...@intel.com> > > Sent: Wednesday, February 28, 2018 7:37 PM > > To: Marvin Häuser <marvin.haeu...@outlook.com>; edk2- > > de...@lists.01.org; Laszlo Ersek <ler...@redhat.com>; > Kinney, Michael D > > <michael.d.kin...@intel.com> > > Cc: Gao, Liming <liming....@intel.com> > > Subject: RE: [edk2] [PATCH 1/2] MdePkg/Base.h: Ensure > safe bitwise > > operations. > > > > 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.haeu...@outlook.com] > > > Sent: Wednesday, February 28, 2018 6:21 AM > > > To: edk2-devel@lists.01.org; Laszlo Ersek > <ler...@redhat.com> > > > Cc: Kinney, Michael D <michael.d.kin...@intel.com>; > Gao, Liming > > > <liming....@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 <ler...@redhat.com> > > > > Sent: Wednesday, February 28, 2018 2:57 PM > > > > To: Marvin Häuser <marvin.haeu...@outlook.com>; > edk2- > > > > de...@lists.01.org > > > > Cc: michael.d.kin...@intel.com; > liming....@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
