1866562 - _M_X64 definition is not consistent between regular builds and mingwclang x64 builds

Status: RESOLVED FIXED

(Firefox Build System :: General: Unsupported Platforms, defect)

Description

[Yannis Juglaret [:yannis]]

According to recent pushes to try, it seems that in mingwclang builds #if defined(_M_X64) is expanded differently when compiling code that lives in mozglue/misc/ C++ files (where _M_X64 is never defined) compared to other parts of the code e.g. widget/windows/ (where _M_X64 is defined when targeting x86_64 as expected). This can lead to compilation failures and also to successful compilation of incoherent code, depending on how the code that depends on this definition is written.

To illustrate this issue I have written a minimal patch here. I expect the associated x64 mingwclang jobs to create binaries that will crash on startup. In this patch I define the following function:

inline bool IsTargetingX64() {
#if defined(_M_X64)
  return true;
#else
  return false;
#endif
}

And I use it in two locations. First in mozglue/misc/MingwTestFile.cpp where I expect that we will always incorrectly return false:

MFBT_API bool IsMozglueTargetingX64() { return IsTargetingX64(); }

And then in widget/windows/nsAppShell.cpp where I compare its result to the value returned by the other function:

nsresult nsAppShell::Init() {
  MOZ_RELEASE_ASSERT(IsTargetingX64() == IsMozglueTargetingX64());
  // ...
}

I expected this code to crash in mingwclang x64 builds because IsTargetingX64() will evaluate to true in widget/windows/nsAppShell.cpp whereas IsMozglueTargetingX64() will return false.

Comment 1

[Yannis Juglaret [:yannis]]

Nevermind, everything seems to work fine. I think the fault is my own!

Comment 2

[Yannis Juglaret [:yannis]]

I initially misunderstood my actual problem, so here it is. This code results in successful regular builds but failed MinGW builds:

#if defined(_M_X64)
constexpr bool kInitiallyTargetingX64 = true;
#else
constexpr bool kInitiallyTargetingX64 = false;
#endif

#include <stdint.h>

#if defined(_M_X64)
constexpr bool kFinallyTargetingX64 = true;
#else
constexpr bool kFinallyTargetingX64 = false;
#endif

static_assert(kInitiallyTargetingX64 == kFinallyTargetingX64);

In regular builds, _M_X64 is defined directly by the compiler if targeting x64. When you compile something _M_X64 will be available from the start of the C++ file. However in mingwclang builds, _M_X64 is not directly defined like this. It only appears after including some header files, such as windows.h or stdint.h (the list in unclear to me), likely because they end up including _mingw_mac.h. (Including stddef.h does not result in defining _M_X64, for example.)

Comment 3

[Yannis Juglaret [:yannis]]

Attachment: Bug 1866562 - Make MSVC-style architecture constants consistent in mingwclang builds. r=tjr

This patch makes mingwclang build compile with -include _mingw.h so that
MSVC-style constants are defined from the start of C++ files rather than
at the point where they start including files. This makes mingwclang
builds behave more closely to regular builds, thus potentially avoiding
future backouts for mingwclang build failures and/or successful but
incorrect mingwclang builds.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9d5244ee58a6
Make MSVC-style architecture constants consistent in mingwclang builds. r=tjr

Comment 5

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/9d5244ee58a6