Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OSArchitecture is wrong on non-Windows #58463

Closed
ericstj opened this issue Aug 31, 2021 · 18 comments
Closed

OSArchitecture is wrong on non-Windows #58463

ericstj opened this issue Aug 31, 2021 · 18 comments

Comments

@ericstj
Copy link
Member

ericstj commented Aug 31, 2021

Related: #26612

Description

We hardcode the value as that which we're building for:

int32_t SystemNative_GetOSArchitecture()
{
#if defined(TARGET_ARM)
return ARCH_ARM;
#elif defined(TARGET_ARM64)
return ARCH_ARM64;
#elif defined(TARGET_AMD64)
return ARCH_X64;
#elif defined(TARGET_X86)
return ARCH_X86;
#elif defined(TARGET_WASM)
return ARCH_WASM;
#elif defined(TARGET_S390X)
return ARCH_S390X;
#else
#error Unidentified Architecture
#endif
}

This will be incorrect when running x86 on x64, or x64 on ARM64. @richlander

I noticed this when looking into how to differentiate x64 on ARM64. My current approach is to use sysctl("hw.machine") which seems like it could work here. Not sure if that's "best practice" for identifying the machine architecture or not.

Configuration

.NET 6.0. Tested on x64 build on ARM64 but would be true on any case where running an emulated architecture.

Regression?

No

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Aug 31, 2021
@richlander
Copy link
Member

If we switch this API to return the correct value, then do we have another to return the architecture of the process? If not, then it seems like we might be favoring one class of bugs over another.

@jeffhandley
Copy link
Member

Closing with the same rationale as #26612 that the current behavior is reasonable.

@ericstj
Copy link
Member Author

ericstj commented Oct 28, 2021

If we switch this API to return the correct value, then do we have another to return the architecture of the process? If not, then it seems like we might be favoring one class of bugs over another.

We have two APIs. One is OSArchitecture, the other is ProcessArchitecture

ProcessorArchitecture is useful when you're trying to write interop code and you need to understand different calling conventions / integer widths / which build of a native dependency you want to load in your same process.

OSArchitecture is useful for logging, system diagnostics, and deployment. In those cases you'd like to know what other processes the system is capable of running.

Reactivating this since there is as an active PR on this.

@ericstj ericstj reopened this Oct 28, 2021
@AaronRobinsonMSFT AaronRobinsonMSFT removed the untriaged New issue has not been triaged by the area owner label Nov 22, 2021
@sgmunn
Copy link

sgmunn commented Dec 6, 2021

Just discovered this issue when actually trying to report the OS architecture. The current implementation does not appear to meet the expectations of the documentation for this.

@jkotas
Copy link
Member

jkotas commented Dec 6, 2021

@sgmunn What was the environment where you encountered this issue?

@ghost
Copy link

ghost commented Dec 6, 2021

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

Related: #26612

Description

We hardcode the value as that which we're building for:

int32_t SystemNative_GetOSArchitecture()
{
#if defined(TARGET_ARM)
return ARCH_ARM;
#elif defined(TARGET_ARM64)
return ARCH_ARM64;
#elif defined(TARGET_AMD64)
return ARCH_X64;
#elif defined(TARGET_X86)
return ARCH_X86;
#elif defined(TARGET_WASM)
return ARCH_WASM;
#elif defined(TARGET_S390X)
return ARCH_S390X;
#else
#error Unidentified Architecture
#endif
}

This will be incorrect when running x86 on x64, or x64 on ARM64. @richlander

I noticed this when looking into how to differentiate x64 on ARM64. My current approach is to use sysctl("hw.machine") which seems like it could work here. Not sure if that's "best practice" for identifying the machine architecture or not.

Configuration

.NET 6.0. Tested on x64 build on ARM64 but would be true on any case where running an emulated architecture.

Regression?

No

Author: ericstj
Assignees: -
Labels:

area-System.Runtime

Milestone: -

@tommcdon
Copy link
Member

tommcdon commented Dec 6, 2021

@jeffhandley I updated the area path per the https://github.com/dotnet/runtime/blob/main/docs/area-owners.md. Feel free to re-assign if needed.

@tommcdon tommcdon added the untriaged New issue has not been triaged by the area owner label Dec 6, 2021
@sgmunn
Copy link

sgmunn commented Dec 6, 2021

@jkotas we were trying to report the OS architecture of VSMac when running on x64 or arm64. This hides the real OS architecture when x64 builds are run on arm64. I was expecting it to return the architecture of the OS, not what had been built against. I guess in some ways I can see why, but both ProcessArchitecture and OSArchitecture return the exact same thing, it seems redundant and non-useful, whereas reporting the actual architecture would be useful to know if you have x64 builds running on an arm64 (via rosetta).

@AraHaan
Copy link
Member

AraHaan commented Dec 6, 2021

Why not rename this to ProcessArchetecture or put this into System.Diagnostics.Process as a static property, then have OSArchitecture use (on windows) GetSystemInfo() winapi function to get the real os architecture (at least on windows), and fall back to the renamed version of this when not on windows?

@AraHaan
Copy link
Member

AraHaan commented Dec 6, 2021

it seems it’s also possible to do this but have it’s implementation in coreclr.

Here is how it can be done in unix (would require some c++ and an header):
B2761C83-8B2A-4935-A33C-32DCA87D5668

@tannergooding tannergooding removed the untriaged New issue has not been triaged by the area owner label Jan 24, 2022
@tannergooding
Copy link
Member

tannergooding commented Jan 24, 2022

We have two APIs. One is OSArchitecture, the other is ProcessArchitecture

@ericstj, I think part of the confusion here is that for all intents and purposes the OSArchitecture for x64 on ARM64 is indeed x64. This is also what Windows reports and fixes up all your environment variables and other OS level API queries to report.

If you are running in a SysWOW64 process, then %PROCESSOR_ARCHITECTURE%=x86.

The fact that you are functionally in a pseudo-VM running on top of a different OS is only available via alternative environment variables and APIs. For example, you must query %PROCESSOR_ARCHITEW6432% (same for AMD64 for x86 on x64 or Arm32 on Arm64 or x64 on Arm64) and utilize IsWow64Process2 to get all the relevant information, because everything else is intentionally lying to you, by design.

Changing ProcessArchitecture to report the root native information is likely going to cause bugs, particularly when looking at how this has always worked for x86 on x64 in the underlying OS. We should likely be looking at mirroring Windows here and introducing a separate API that allows you to directly query the information about the "native machine".

@AraHaan
Copy link
Member

AraHaan commented Jan 24, 2022

I would support having the information about the "native machine" to be placed in an NativeProcessArchitecture class, however I do not like how big the class name would be.

Perhaps it could be shorted to NativeArchitecture instead???

@jkotas
Copy link
Member

jkotas commented Jan 24, 2022

There are number of different emulation technologies. Which ones would the new API be able to see through? I think it is the key challenge with the existing API or any new API like that.

Changing ProcessArchitecture to report the root native information is likely going to cause bugs,

Yes, it might like any other breaking change. On the other hand, it would keep the API surface somewhat understandable. If we want to do something here, my vote would be to modify the behavior of the existing API. If we were to introduce similar partial redundant APIs, I think it would be very hard to tell which one is the right one to use in a given context.

@ericstj
Copy link
Member Author

ericstj commented Jan 25, 2022

I think part of the confusion here is that for all intents and purposes the OSArchitecture for x64 on ARM64 is indeed x64. This is also what Windows reports and fixes up all your environment variables and other OS level API queries to report.

OSArchitecture is our concept. We decide what it means. Windows has an API https://docs.microsoft.com/en-us/windows/win32/api/sysinfoapi/nf-sysinfoapi-getnativesysteminfo which we used and that's what reports on x64 on ARM64. Windows decided to make that API to report x64 on arm64 based on the number of apps that were already using it that they wanted to work on x64 on ARM64. Given that API was expressing a different concept, I don't think we necessarily have to follow their lead. IMO the concept we're trying to represent maps more closely to pNativeMachine returned by https://docs.microsoft.com/en-us/windows/win32/api/wow64apiset/nf-wow64apiset-iswow64process2.

Changing ProcessArchitecture to report the root native information is likely going to cause bugs,

No changes are suggested to ProcessArchitecture only OSArchitecture.

In my opinion if we don't do our best effort to make OSArchitecture represent the real architecture of the OS then we should obsolete the API and tell folks we aren't going to provide any information in this regard.

@hamarb123
Copy link
Contributor

hamarb123 commented Feb 8, 2022

I ran into this issue a while ago (~December last year) and was confused why it was wrong, so I now P/Invoke IsWow64Process2 with a fallback to OSArchitecture/GetNativeSystemInfo when unrecognised arch (in case of future additions, it will just be "wrong" like it currently is ie. report x86 or something) or unsupported Windows version.

I ran into this issue when writing code that would make sure the correct files got extracted, it seemed to always think it was x64 on the arm64 machine I was testing it on. I hope this gets fixed, but if not it should state in the documentation that it may be wrong for windows arm64 and whatever other OSes it's wrong for so programmers can account for this.

ITO whether in a VM it should return the VM's architecture or the host's architecture (#60910), I don't even know how it would be able to access the host's architecture, but if (for example) the host happened to be arm64 and the VM happened to be x64, then getting back arm64 wouldn't be very useful IMO since you can't run arm64 apps in the VM, if someone wanted this behaviour, perhaps there could be another API to detect this (I'd have no use for this, I just want OSArchitecture to tell me the truth about the OS), but I'd imagine most would think OSArchitecture would simply return the OS's architecture, even if the OS is a VM.

@jkotas #60910 (comment) my situation is that the app would detect if it is using the wrong architecture executables and then get them replaced with what it deemed correct, this could happen in multiple ways with this app eg. being copied, new architecture in future .NET & Windows versions, restoring a backup of Windows onto a new machine with a different architecture, or supporting arm64 in the first place (which was an update, so as you can probably guess, this helped me discover this issue). It would be nice if this was fixed for .NET 7/8 since that's what I plan to use next. I can test my app with it if you want, I'll just need the exe installer for the PR (or I could try to built it myself...)

@dotMorten
Copy link

I report process and is architecture as part of telemetry to understanding whether we should be adding support for more Architectures. I don’t want to be lied about the OS architecture. There’s naturally a distinct difference between OS and process architecture.

@jkotas
Copy link
Member

jkotas commented Apr 25, 2022

Fixed by #60910

@jkotas jkotas closed this as completed Apr 25, 2022
@ghost ghost locked as resolved and limited conversation to collaborators May 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests