Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Fixing arm subsystem version #8211

Merged
merged 1 commit into from
Nov 20, 2016
Merged

Fixing arm subsystem version #8211

merged 1 commit into from
Nov 20, 2016

Conversation

Petermarcu
Copy link
Member

Being explicit about the windows subsystem for arm and getting rid of all the linker warnings about it.

LINK : warning LNK4010: invalid subsystem version number 6.00; default subsystem version assumed

@Petermarcu
Copy link
Member Author

@gkhanna79 , PTAL

@@ -293,7 +293,12 @@ if (WIN32)
#
# Disable the following line for UNIX altjit on Windows
set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} /MANIFEST:NO") #Do not create Side-by-Side Assembly Manifest
set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} /SUBSYSTEM:WINDOWS,6.00") #windows subsystem
if (CLR_CMAKE_HOST_ARCH STREQUAL arm)
set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} /SUBSYSTEM:WINDOWS,6.02") #windows subsystem - arm minimum is 6.02
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a specific reason we need to set this?

@Petermarcu
Copy link
Member Author

Because we are passing an invalid version today, getting a bunch of warnings about it and then letting the toolset float us to whatever it decides to default to.

@Petermarcu Petermarcu merged commit fc3607f into dotnet:master Nov 20, 2016
@jkotas
Copy link
Member

jkotas commented Nov 21, 2016

Do we need similar fix for arm64 as well?

@gkhanna79
Copy link
Member

Looking at https://ci.dot.net/job/dotnet_coreclr/job/master/job/arm64_cross_checked_windows_nt/81/consoleFull, we see the same warnings for Arm64 as well. Given this, I am wondering if we even need to specify the subsystem version given that the linker will pick the correct default version as described in the table at https://msdn.microsoft.com/en-us/library/fcc1zstk.aspx.

@jkotas What do you think?

@jkotas
Copy link
Member

jkotas commented Nov 21, 2016

the linker will pick the correct default version

It will pick Vista (6.00) as the default for x86/x64 that is not quite right because of CoreCLR does not work on Vista. Even the Win8 (6.02) default for arm is not quite right because of I do not think CoreCLR will work on Win8 ARM.

It may be ok to use the default for simplicity though.

@gkhanna79
Copy link
Member

The change (and the current implementation) actually implement the same rules that you call out below.

It may be ok to use the default for simplicity though.

Yes, that is my goal.

@Petermarcu
Copy link
Member Author

I've seen problems in the past where we weren't explicit about things like this and pulling in a new toolset floats the default to something we didn't intend. Wouldn't it be most correct to get it set to the right value that is valid for each leg?

@gkhanna79
Copy link
Member

What we are setting explicitly is what the linker will do (see the table in the link), independent of the toolset.

Wouldn't it be most correct to get it set to the right value that is valid for each leg?

Unless CMake has a bug that we wish to workaround, I would suggest we validate that linker does the right thing by default and work from there.

@Petermarcu
Copy link
Member Author

BTW, I'm happy to make the arm64 fix as well. I can also update to 6.0.1 for the non-arm platforms to represent our Win7 minimum. Once we get this right, it won't show up again until we have a new platform where the existing non-arm value isn't correct or explicitly want to drop an older platform.

@Petermarcu
Copy link
Member Author

My point is that those defaults are for that version of the linker. Those defaults move over time. New versions of the linker will have different defaults. For example, before 6.00 existed, the linker defaulted to something less.

@gkhanna79
Copy link
Member

New versions of the linker will have different defaults.

If the toolset changes its defaults for the version of subsystem it supports, I believe that would imply that the previous versions of the OS may not be supported by that toolset (and thus, specifying an explicit version may not protect us against such updates).

I would expect such toolset updates to be effectively dropping support of platforms and thus, we should be cognizant about picking them up anyways. And if we do pick such updates, then we again want the toolset to specify its defaults as opposed to us specifying something that results in new warnings (about unsupported subsystem version).

@Petermarcu
Copy link
Member Author

No, thats why there is a minimum column. You can compare these two pages... minimum support moves seperately from default.

https://msdn.microsoft.com/en-us/library/fcc1zstk(v=vs.140).aspx
https://msdn.microsoft.com/en-us/library/fcc1zstk(v=vs.100).aspx

Default is a policy decision not a correctness one and we want to control that policy. I was a part of an effort a few years ago where a bunch of people broke because the default jumped from 5 to 6 and that dropped XP support even though the toolset was totally able to target 5. The fix was to go through everyones build system's and make sure they were explicit about their minimum and pass it in.

@gkhanna79
Copy link
Member

Interesting! Given this, I am fine making the subsystem version explicit. Also, please make the corresponding changes in CoreFX and Core-Setup since you are on the roll :)

@Petermarcu
Copy link
Member Author

Happy to go do it in the other places :)

@Petermarcu Petermarcu deleted the subsystem branch November 22, 2016 01:20
@karelz karelz modified the milestone: 2.0.0 Aug 28, 2017
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants