microsoft / Windows-Dev-Performance

A repo for developers on Windows to file issues that impede their productivity, efficiency, and efficacy
MIT License
434 stars 20 forks source link

"Must define a target architecture" issue from winnt.h when included by `.rc` files #98

Closed saschanaz closed 8 months ago

saschanaz commented 2 years ago

When building Firefox with the new Windows 11 SDK:

 0:48.90 In file included from module.rc:6:
 0:48.90 In file included from C:\Program Files (x86)\Windows Kits\10\include\10.0.22000.0\um\winuser.h:61:
 0:48.90 In file included from C:\Program Files (x86)\Windows Kits\10\include\10.0.22000.0\um\libloaderapi.h:18:
 0:48.90 In file included from C:\Program Files (x86)\Windows Kits\10\include\10.0.22000.0\shared\minwindef.h:182:
 0:48.90 C:\Program Files (x86)\Windows Kits\10\include\10.0.22000.0\um\winnt.h(253,2): error: Must define a target architecture.
 0:48.90 #error Must define a target architecture.
 0:48.90  ^
 0:49.05 1 error generated.

Here, the module.rc is autogenerated and looks like this: https://searchfox.org/mozilla-central/rev/7e36862b61622889b28492d60564dbab1e6d8bbf/config/create_rc.py

And the error is from SYSTEM_CACHE_ALIGNMENT_SIZE definition:

#ifndef SYSTEM_CACHE_ALIGNMENT_SIZE
#if defined(_AMD64_) || defined(_X86_)
#define SYSTEM_CACHE_ALIGNMENT_SIZE X86_CACHE_ALIGNMENT_SIZE
#elif defined(_ARM64_) || defined(_ARM_)
#define SYSTEM_CACHE_ALIGNMENT_SIZE ARM_CACHE_ALIGNMENT_SIZE
#else
#error Must define a target architecture.
#endif
#endif // SYSTEM_CACHE_ALIGNMENT_SIZE

Looking at winnt.h, I found this comment:

//
// Note: RC_INVOKED is checked in PROBE_ALIGNMENT to maintain compatibility with previous
//       versions of the SDK which did not block inclusion in an .RC file.
//

I guess SYSTEM_CACHE_ALIGNMENT_SIZE should get the same treatment as PROBE_ALIGNMENT and there should be RC_INVOKED check, or does Windows 11 SDK now blocks inclusion in an .RC file?

See also https://bugzilla.mozilla.org/show_bug.cgi?id=1721208.

Eli-Black-Work commented 1 year ago

@saschanaz Are you building this with Visual Studio?

saschanaz commented 1 year ago

Nope.

zooba commented 1 year ago

If this is the same issue I ran into on a separate project, the answer was to #include <windows.h> instead of winuser.h in the RC file. (I don't have any special knowledge as a MSFT employee here, just ran into a similar issue myself a while back.)

sylveon commented 1 year ago

This is correct, windows.h defines the macros identifying the target arch. There's a reason almost all winapi docs say "header.h (include windows.h)"

saschanaz commented 1 year ago

Well, my point is that:

//
// Note: RC_INVOKED is checked in PROBE_ALIGNMENT to maintain compatibility with previous
//       versions of the SDK which did not block inclusion in an .RC file.
//

#if defined(_AMD64_) || defined(_X86_) || defined(_ARM64EC_)
#define PROBE_ALIGNMENT( _s ) TYPE_ALIGNMENT( DWORD )
#elif defined(_IA64_) || defined(_ARM_) || defined(_ARM64_)

//
// TODO: WOWXX - Unblock ARM. Make all alignment checks DWORD for now.
//

#define PROBE_ALIGNMENT( _s ) TYPE_ALIGNMENT( DWORD )
#elif !defined(RC_INVOKED)
#error "No Target Architecture"
#endif

This compatibility is now broken.

zooba commented 1 year ago

winuser.h isn't meant for RC files. It should be winuser.rh, but including windows.h figures that out. I don't know exactly when this was changed, but it caught me out too.

Basically, don't include winuser.h directly (which was always the instruction, but I guess a sample came out at some point that led us astray).

AdamBraden commented 8 months ago

closing as comments above seem to have resolved the issue.