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

Fixed build errors resulting from upgrading to VS2019 compilers #3894

Merged
merged 3 commits into from
Jun 21, 2019
Merged

Fixed build errors resulting from upgrading to VS2019 compilers #3894

merged 3 commits into from
Jun 21, 2019

Conversation

harishsk
Copy link
Contributor

Fixes #3893

The default CMAKE_C_FLAG for debug configuration sets /ZI to generate a PDB capable of edit and continue. In the new compilers, this is incompatible with /guard:cf which we set for security reasons. So to fix, we need to go back to a regular pdb generated by the /Zi flag.

Since this part of the default CMake rules, we need to override the full default set and update only that particular flag.

# which is incompatible with the /guard:cf flag we set below
# for security. So we use the default flags set by CMake
# and reset /ZI with /Zi
set(CMAKE_C_FLAGS_DEBUG "/MDd /Zi /Ob0 /Od /RTC1 /JMC")
Copy link
Member

Choose a reason for hiding this comment

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

Where do those flags /MDd /Zi /Ob0 /Od /RTC1 /JMC come from? Are they default flags in CMake? Could you please add more details for maintaining them in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, those are the default flags in CMake. I will add a bit more detail and submit another PR soon.

@codemzs codemzs self-requested a review June 21, 2019 17:24
Copy link
Member

@codemzs codemzs left a comment

Choose a reason for hiding this comment

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

thanks, @harishsk

@harishsk harishsk merged commit e66e19e into dotnet:master Jun 21, 2019
@harishsk
Copy link
Contributor Author

Purely for educational purposes...these are the recent changes in Windows-MSVC.cmake that caused this issue:

# default value for Just My Code flag
set(_JMC "")
# default value for Debug Information Format flag
set(_DEBUGINFOFORMAT_DEBUG "/Zi")


if("x${CMAKE_C_COMPILER_ID}" STREQUAL "xMSVC" OR "x${CMAKE_CXX_COMPILER_ID}" STREQUAL "xMSVC" )
    # JMC only supported in MSVC
    if(MSVC_VERSION GREATER_EQUAL 1915)
      set(_JMC "/JMC")
    endif()
    # /ZI is supported by MSVC only, not by clang-cl for example.
    set(_DEBUGINFOFORMAT_DEBUG "/ZI")
  endif()

string(APPEND CMAKE_${lang}_FLAGS_DEBUG_INIT " /MDd ${_DEBUGINFOFORMAT_DEBUG} /Ob0 /Od ${_RTC1} ${_JMC}")

@janvorli
Copy link
Member

@harishsk where have you found the stuff above? I've looked at the Windows-MSVC.cmake in all versions from 3.14 till 3.15 and none of this was there (looking via https://github.com/Kitware/CMake). And cmake 3.14.2 with VS2019 builds fine without your change (And the vcxproj files contain /Zi, not /ZI).

@eerhardt
Copy link
Member

I am experiencing the same as @janvorli. I can build successfully on VS 2019 with CMake 3.14.5 without this change.

I sort of think this change should be reverted until we know more. I don't like the fact that we are clobbering the defaults.

@harishsk
Copy link
Contributor Author

Where is your CMake located? And what is the version reported for CMake.exe --version

Mine is here: "C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\Common7\IDE\CommonExtensions\Microsoft\CMake\CMake\bin\cmake.exe"

And it reports: cmake version 3.14.19050301-MSVC_2

The changes I reported above are in this file:
c:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\Common7\IDE\CommonExtensions\Microsoft\CMake\CMake\share\cmake-3.14\Modules\Platform\Windows-MSVC.cmake

@eerhardt
Copy link
Member

My CMake is located at C:\Program Files\CMake and I installed 3.14.5 from https://cmake.org/download/ and added it to my $PATH. This is all documented on https://github.com/dotnet/machinelearning/blob/master/docs/building/windows-instructions.md#required-software.

@janvorli informed me there might be an issue with the CMake shipped by VS 2019:

Comparing the Windows-MSVC.cmake of vanilla 3.14.2 and the 3.14 in VS2019, I can see that they removed this at one place:

# default value for Just My Code flag
set(_JMC "")
# default value for Debug Information Format flag
set(_DEBUGINFOFORMAT_DEBUG "/Zi")

And added this at another

  if("x${CMAKE_C_COMPILER_ID}" STREQUAL "xMSVC" OR "x${CMAKE_CXX_COMPILER_ID}" STREQUAL "xMSVC" )
    # JMC only supported in MSVC
    if(MSVC_VERSION GREATER_EQUAL 1915)
      set(_JMC "/JMC")
    endif()
    # /ZI is supported by MSVC only, not by clang-cl for example.
    set(_DEBUGINFOFORMAT_DEBUG "/ZI")
  endif()

It seems as if they just made a mistake in the "i" case

@jkotas - do you happen to know anyone on the Visual Studio team who works on the CMake they ship? I'd like to find out if this is a bug in VS 2019.

@janvorli
Copy link
Member

Btw, it seems that a cleaner way to fix the problem would be to use string(REPLACE ....) cmake command to just replace /ZI by /Zi in CMAKE_C_FLAGS_DEBUG instead of overwriting all the options with what we assume should be there. Like this:

string(REPLACE "/ZI" "/Zi" CMAKE_C_FLAGS_DEBUG  ${CMAKE_C_FLAGS_DEBUG})

@harishsk
Copy link
Contributor Author

Yes, I have CMake installed in Program Files on my laptop and I don't see this bug. On my desktop, I don't have CMake installed and the one shipped in Visual Studio gets used, resulting in this bug.

This is a bug in VS2019. And as my comment in the fix states, we can remove those lines after the defaults are fixed.

I agree what you propose is a cleaner fix. I have tested it and submitted a new pull request.

@janvorli and @eerhardt Can you please review and approve?

@jkotas
Copy link
Member

jkotas commented Jun 21, 2019

do you happen to know anyone on the Visual Studio team who works on the CMake they ship?

Take a look at https://github.com/microsoft/CMake . I believe it has the sources for the version that VS ships; and you can also find the people who work on it there.

Dmitry-A pushed a commit to Dmitry-A/machinelearning that referenced this pull request Jul 24, 2019
…et#3894)

* Fixed build errors resulting from upgrade to VS2019 compilers

* Added additional message describing the previous fix
@ghost ghost locked as resolved and limited conversation to collaborators Mar 21, 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.

Visual Studio 2019 Building Problem
6 participants