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

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

Thanks. Comments are inline.

Best regards,
Marvin.

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Wednesday, February 28, 2018 12:00 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/27/18 21:31, Marvin Häuser wrote:
> >> -----Original Message-----
> >> From: Laszlo Ersek <lersek@redhat.com>
> >> Sent: Tuesday, February 27, 2018 8:54 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.
> >>
[...]
> 
> This is exactly how I feel, yes. My concern is that making the change now
> runs the risk of tricky regressions that we might not even prevent with a
> detailed audit.

Actually, your explanations and the rest of the bit defines made me wonder, whether marking all BIT defines and bit masks of any kind, anywhere, as ULL, might be the best solution.
+1) Inversion can be used for any integer size uncasted, as edk2 does not support vendor extensions such as __int128 anyway.
+2) Binary operations (AND, OR, ...) should not raise any problems at all (except for our fellow using VS2005x86 :) )
+3) Assignments and passing-by-arguments will result in compiler-time errors when not done properly in the sense of this patch, which eliminates the need to audit all usages manually, but just compile the entire codebase and fix the errors one-by-one (which is of course still not a quick task, but if the package authors agree with the proposal, I might attempt it).
-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?

> 
> Anyway, I don't want to spread FUD about this -- I think the goal of these
> changes is good. It's up to the MdePkg maintainers to evaluate the risks, I
> just wanted us to be aware of them. Once we reach an end state where all
> the BIT, SIZE and BASE macros are unsigned, and no regressions are found (or
> none remain), that's for the best!

Nah, your comments are great, thanks!

[...]
> I think I agree with your assessment, considering the usual application of
> these macros in source code. I distinguish three kinds of uses:
> 
[...]
> 
> (3) The clever code: is aware that BIT macros are (mostly) signed, and
>     exploits that fact for various ends.
> 
> I agree that most of the edk2 codebase should fall under (1).
> 
> As you may expect, I personally write (2), and code like (2) should not worry
> about BIT / SIZE / BASE becoming unsigned.
> 
> My concern is (3). There have been examples in core edk2 modules that
> explicitly relied on undefined behavior, such as left shifts of negative integers
> and such. We've only slowly fixed them up after compilers / analyzers started
> flagging them.
> 
> If we don't think (3) is a real risk, I'm fine with the approach of these patches.
> (I don't think I'll be able to send a real R-b for them, because that would
> require me to evaluate all uses, and that's a Herculean task I just can't take
> on; apologies.)

I hope that the 'all ULL' proposal uncovers them all. Would there be cases, where no error would be raised?

[...]
> 
> >>> -#define ENCODE_ERROR(StatusCode)     ((RETURN_STATUS)(MAX_BIT |
> >> (StatusCode)))
> >>> +#define ENCODE_ERROR(StatusCode)     ((RETURN_STATUS)(MAX_BIT
> |
> >> (StatusCode##ULL)))
> >>>
> >>
> >> MAX_BIT always has type UINTN (UINT64 aka "unsigned long long" on
> >> 64-bit platforms, and UINT32 aka "unsigned int" on 32-bit platforms).
> >> This means that ENCODE_ERROR results in a UINTN, right now.
> >
> > Good point, I didn't keep that in mind. It probably should be solely
> > 'U'.
> 
> In fact there's no need even for that: because MAX_BIT has type UINTN
> ("unsigned long long" or "unsigned int"), the StatusCode macro argument,
> which is expected to be a small nonnegative "int", will be converted to
> UINTN, for the bitwise OR operator.

Oh, right, thanks. I would still prefer it to be explicit, but I'll wait for a maintainer's comment.

[...]
> 
> What is implementation-defined in the current definition of
> ENCODE_ERROR()?

That was a result of the misunderstanding above, scrap that.

> 
> Given the current definition, if StatusCode is a signed integer, then one way
> or another, it will be converted to (not reinterpreted as) an unsigned integer
> type. This is due to the rules on the bitwise OR operator and the outermost
> cast (thank you for reminding me of that).
> Such conversions are fully defined in the C standard, they are not
> implementation-defined.

Thanks for noting. Would you still append the U-prefix for readability / making the intention clear?

[...]
> 
> Is your point that we shouldn't even assume INT32=="int" and
> UINT32=="unsigned int"?

Yes, pretty much. I don't think it does hurt either. While it is certainly a very ugly definition, that's my preferred use for macros anyway - hide ugly code behind a lovely name.

> 
> I think that would be a good point for a highly portable C codebase that was
> written from the ground up without type assumptions. But I don't think it
> applies to edk2. If UINT32 mapped to "unsigned short" and INT64 mapped to
> "int", edk2 would break in a thousand places immediately.

Of course it would, however I imagined a header file that is included in every file for every architecture to be as close to flawless as possible.
While of course I would also fix such assumptions in executable code if I found, and hope the maintainers care enough to review and merge, I consider header files a greater danger as that 'flawed' (in an idealistic sense) macro is expanded into external code, which might cause trouble while debugging, as the consumer assumes the mistake is within his code.
Headers >>> library code > module code, in my opinion, especially for the most central one.

[...]
> 
> I do get your point now. I believe you are trying to eliminate all (or
> most) implementation-defined traits from these macros, because "that's
> how portable C code should be written".

Exactly. :)

> 
> Personally I think that's a laudable goal, and I agree that all portable C code
> should be written like that. I also think however that there's a large
> disconnect between that goal / style, and how edk2 (and even the UEFI
> spec) exist today. In my opinion / experience, edk2 was never meant to be
> portable to unknown, future platforms. For one, it is tied to little endian byte
> order on the spec level.

Personally, I am fine to assert anything that the specification demands (that's what it's here for after all), however we are not aware of the compiler-specifics at spec-level.

> 
> Anyway, if you submit a v2 of these series (please make them threaded!), I'll
> try to review those with this in mind.
> 
> Thanks!
> Laszlo