Restore XCODE5 build

[mikebeaton]

Description

Drifts in EDK 2, submodules and Xcode itself mean the XCODE5 toolchain currently does not build out-of-the-box.

The changes required to fix this are small, and are included in this sequence of commits.

It should not be hard to achieve CI as well, but some quick experiments show that because I do not control https://dev.azure.com/tianocore/edk2-ci, nor mu_nasm nor mu_iasl, that I believe that as a contributor I cannot set up working CI (it may be possible to fake it by overriding settings in existing CI).

• Breaking change?
• Impacts security?
• Includes tests?

How This Was Tested

Build and run OVMF on XCODE5 (multiple versions and various -D options), and on Linux (multiple toolchains), locally. Also confirm building and running under existing CI for existing toolchains.

Integration Instructions

N/A

[mdkinney]

There are 6 commits in this PR with only 2 of them related to the change. Looks like it may need to be rebased.

Also, the changes to the BEGIN/END macros need to consider that that the use of the macro is followed by a ';'. The expansion of the macro needs to consider this and make sure there are no compiler error/warnings for an extra ';' or a missing statement. These macros have not been modified in a very long time, so the issues related to the ';' may not be present in the currently supported compilers.

[mikebeaton]

I meant there to be 6 commits. Each of these 6 issues breaks the XCODE5 build, and the commit fixes it (albeit that some of the others are ostensibly 'wrong' and need fixing anyway, the other toolchains do not complain and XCODE5 does).

As regards the macro expansion, I did not write it (though ofc I've offered it as I believe it's correct), I don't know if @mhaeuser has any time to comment?

[mdkinney]

I agree all 6 commits are required. All good updates. do see multiple Signed-off-by. Could this be changed to single Signed-off-by and Co-authored-by tags?

I looked at git history. The style of BEGIN()/END() has not changed since imported 17 years ago. I seem to recall the addition of the extra local variable was done to support the ';' at the end of the macro usage. Perhaps removing the extra do loop and just ending with the local variable being set to FALSE would be a more compatible change and address the compiler warning.

[mikebeaton]

As regards the macro expansion, I did not write it (though ofc I've offered it as I believe it's correct), I don't know if @mhaeuser has any time to comment?

I looked at git history. The style of BEGIN()/END() has not changed since imported 17 years ago. I seem to recall the addition of the extra local variable was done to support the ';' at the end of the macro usage. Perhaps removing the extra do loop and just ending with the local variable being set to FALSE would be a more compatible change and address the compiler warning.

Perhaps I can add: it's my belief that the ; handling is already correct; from the comment in the relevant commit, the local variable is more to ensure matched BEGIN and END than to ensure support for ;; I guess that the reason the definition hasn't changed for 17 years is that nothing complained about this before, but I think the XCODE5 compiler's complaint about the current code is probably valid, and that the replacement code has the same effect, but without a 'set but unused' error (i.e. we can 'compile away' what does happen, but it's not a case of a set but unused variable).

EDIT: Reading the code carefully, prompted by your comment, I don't think there is an extra do loop, though it looks like it. The outer do loop is the standard idiom for a macro to end safely with respect to following semicolons, as required. That's unchanged. The inner do loop is new, but it's required as part of the fix to what's happening inside the outer do loop; and also looking at it carefully, I believe cannot be combined with the outer do loop (basically because https://stackoverflow.com/q/32334338).

[mdkinney]

If we want to go for the simplest form of these macros with formatting added to increase readability it would be:

#define DEBUG_CODE_BEGIN()      \
  do {                          \
    if (DebugCodeEnabled ()) {

#define DEBUG_CODE_END()  \
    }                     \
  } while (FALSE)

In this form, if DEBUG_CODE_BEGIN() is used without a ; at the end of the statement, it will still build. If the ; is missing at the end of a DEBUG_CODE_END() statement, then the build will fail for a missing semicolon. I think the original comment is misleading. I think the intent of the extra local variable is to catch use of DEBUG_CODE_BEGIN() with a missing ; at the end of the statement. By adding a local variable declaration, if the ; is missing, a compiler error will result.

We we need is a form that will generate compiler error if ; is missing and is compatible with XCODE.

[mhaeuser]

@mdkinney Thanks for clearing that up! This actually makes a lot of sense. Yet, the parity behaviour I described applies as well - even if unintentionally at first - and I don't think this should be regressed upon. As such, I suggest to add some no-op statement like (VOID) __DebugCodeLocal to require the terminating ;, but to keep the double loops for the parity property.

[mikebeaton]

@mdkinney - Many thanks for your input on this PR. I've redone the DEBUG_CODE and PERF_CODE macros in a new way, avoiding the local variable entirely. I think it looks sane and safe, and addresses what you raised. It compiles and runs correctly on XCODE5 and under CI. I've also switched to Co-authored-by: tags as requested.

[mdkinney]

Thanks for the updates. This version looks much cleaner than the original.

[liyi77]

CryptoPkg part looks good to me.