Unify some cmake function definitions #33716

am11 · 2020-03-18T11:28:16Z

Also cleaned up usage of remaining _TARGET_* in favor TARGET_* macros.

eng/native/configurecompiler.cmake

am11 · 2020-03-20T04:21:15Z

@jkotas, this is ready for review. The no_tiered compilation test on OSX seems to be failing for quite some time (since Feburary): https://dev.azure.com/dnceng/public/_build/results?buildId=566314&view=ms.vss-test-web.build-test-results-tab&runId=17801042&resultId=100000&paneView=history.
Performance Windows_NT x86 release had a timeout during the package restore.

jkotas · 2020-03-20T04:31:32Z

eng/native/configurecompiler.cmake

@@ -467,10 +470,6 @@ if (MSVC)
  add_compile_options(/ZH:SHA_256) # use SHA256 for generating hashes of compiler processed source files.
  add_compile_options(/source-charset:utf-8) # Force MSVC to compile source as UTF-8.

-  if (CLR_CMAKE_HOST_ARCH_I386)
-    add_compile_options(/Gz)


Is there a problem with using /Gz everywhere?

With /Gz in shared location, we get the following error in installer x86 build:

.\build.cmd -subsetcategory installer -arch x86 ... "C:\Users\adeel\Source\Repos\runtime2\artifacts\obj\win-x86.Debug\corehost\INSTALL.vcxproj" (rebuild target) (1) -> "C:\Users\adeel\Source\Repos\runtime2\artifacts\obj\win-x86.Debug\corehost\ALL_BUILD.vcxproj" (default target) (3:2) -> "C:\Users\adeel\Source\Repos\runtime2\artifacts\obj\win-x86.Debug\corehost\cli\comhost\comhost.vcxproj" (default target) (4:2) -> C:\Users\adeel\Source\Repos\runtime2\src\installer\corehost\cli\fxr_resolver.h(83,1): error C2664: 'propagate_error_writer_t::propagate_error_writer_t(const propagate_error_writer_t &)': cannot convert argument 1 from 'hostfxr_set_error_writer_fn' to 'propagate_error_writer_t::set_error_writer_fn' (compiling source file C:\Users\adeel\Source\Repos\runtime2\src\installer\corehost\cli\comhost\comhost.cpp) [C:\Users\adeel\Source\Repos\runtime2\artifacts\obj\win-x86.Debug\corehost\cli\comhost\comhost.vcxproj]

I think it is because this function requires __cdecl?

Yes, looks like some places are missing HOSTPOLICY_CALLTYPE

That also worked, now /Gz is back in shared location: 2264fa0. __cdecl is defined as empty for other platforms in pal.h, so I just used __cdecl or HOSTPOLICY_CALLTYPE, whichever was available via included headers.

jkotas · 2020-03-20T04:32:11Z

eng/native/configurecompiler.cmake

@@ -18,7 +20,7 @@ include(${CMAKE_CURRENT_LIST_DIR}/configureoptimization.cmake)
 #-----------------------------------------------------

 if(MSVC)
-    add_compile_options(/Zi /FC /Zc:strictStrings)


What happened to /Zc:strictStrings ?

It was causing an error in installer:

.\build.cmd -subsetcategory installer fxr_resolver.cpp 6>C:\Users\adeel\Source\Repos\runtime2\src\installer\corehost\cli\hostmisc\pal.windows.cpp(230,51): error C2440: 'initializing': cannot convert from 'const unsigned short [18]' to 'pal::char_t *' [C:\Users\adeel\Source\Repos\runtime2\artifacts\obj\win-x64.Debug\corehost\cli\hostmisc\libhostmisc.vcxproj] C:\Users\adeel\Source\Repos\runtime2\src\installer\corehost\cli\hostmisc\pal.windows.cpp(230,26): message : Conversion from string literal loses const qualifier (see /Zc:strictStrings) [C:\Users\adeel\Source\Repos\runtime2\artifacts\obj\win-x64.Debug\corehost\cli\hostmisc\libhostmisc.vcxproj] longfile.windows.cpp 6>C:\Users\adeel\Source\Repos\runtime2\src\installer\corehost\cli\hostmisc\pal.windows.cpp(280,52): error C2440: '=': cannot convert from 'const unsigned short [18]' to 'pal::char_t *' [C:\Users\adeel\Source\Repos\runtime2\artifacts\obj\win-x64.Debug\corehost\cli\hostmisc\libhostmisc.vcxproj] C:\Users\adeel\Source\Repos\runtime2\src\installer\corehost\cli\hostmisc\pal.windows.cpp(280,29): message : Conversion from string literal loses const qualifier (see /Zc:strictStrings) [C:\Users\adeel\Source\Repos\runtime2\artifacts\obj\win-x64.Debug\corehost\cli\hostmisc\libhostmisc.vcxproj]

I forgot to add it in CoreCLR, will do. Or maybe I should first try to make installer compatible with strictStrings. 💭

Yes, it would be nice to sprinkle const as necessary to fix this.

I am running a clean Windows build on side, will get to this point soon.

Using const in few places fixed the build error withstrictStrings: 4fc15c2. Thanks. 👍

jkotas

@janvorli @jkoritzinsky Any other comments?

janvorli · 2020-03-20T09:37:16Z

eng/native/functions.cmake

@@ -289,6 +300,20 @@ function(strip_symbols targetName outputFilename skipStrip)
  endif(CLR_CMAKE_HOST_UNIX)
 endfunction()

+function(install_symbols targetName destination_path)


It is weird that we always strip symbols for libraries and strip them conditionally for coreclr. Since you are touching this code, I would take this opportunity to make the stripping unconditional. It should be fine (debugging should not be affected, I believe neither lldb nor gdb cares whether the symbols are in the .so files or in separate files) and it would also fix #32957 on OSX where debugging a core dump from checked / debug build is currently broken due to missing symbols.

Should we start by removing the top-level option:

runtime/eng/native/build-commons.sh

Line 190 in 8db7154

echo "-stripsymbols: skip native image generation."

II would do it in 3 steps in this order:

Make stripping the symbols unconditional in the cmake files (essentially just ignore the -stripsymbols option as part of this PR

Remove passing in the -stripsymbols to the build scripts in the CI / build lab scripts in a follow up PR

Remove the -stripsymbols option support from the build-commons.sh

The point is that we don't want to have intermediate period where we would not be stripping the symbols and we also cannot pass in the -stripsymbols from the CI / build lab scripts after we remove support for that option from the build-commons.sh

Part 1 is pushed f193079. I will make followup PRs for 2 and 3. Thanks!

janvorli

LGTM, thank you!

am11 · 2020-03-21T01:27:04Z

Looks like OOM, known issue perhaps related to: #32377

https://helix.dot.net/api/2019-06-17/jobs/c1c621cc-34bb-4eb9-b949-0b0837654e7e/workitems/System.Collections.NonGeneric.Tests/console

Executed on a002H1X
+ ./RunTests.sh --runtime-path /home/helixbot/work/B4890A01/p --rsp-file mono.issues.rsp
----- start Fri Mar 20 22:44:50 UTC 2020 =============== To repro directly: =====================================================
pushd .
/home/helixbot/work/B4890A01/p/dotnet exec --runtimeconfig System.IO.Tests.runtimeconfig.json --depsfile System.IO.Tests.deps.json xunit.console.dll System.IO.Tests.dll -xml testResults.xml -nologo -nocolor -notrait category=IgnoreForCI -notrait category=OuterLoop -notrait category=failing -notrait category=nonnetcoreapptests -notrait category=nonlinuxtests @mono.issues.rsp
popd
===========================================================================================================
~/work/B4890A01/w/BE740A2B/e ~/work/B4890A01/w/BE740A2B/e
  Discovering: System.IO.Tests (method display = ClassAndMethod, method display options = None)
mono_threads_pthread_kill: pthread_kill failed with error 11 - potential kernel OOM or signal queue overflow

am11 · 2020-03-21T02:22:19Z

Consecutive runs are using same instance on Helix a002H1X, (which seems to be running out of memory?) attempt 3, attempt 4.

eng/native/configureplatform.cmake

dotnet#33716 set TARGET_DARWIN for iOS too but most of the logic is really OSX-specific. Unset TARGET_DARWIN for iOS and rename the rest of the occurrences to TARGET_OSX.

#33716 set TARGET_DARWIN for iOS too but most of the logic is really OSX-specific. Unset TARGET_DARWIN for iOS and rename the rest of the occurrences to TARGET_OSX.

Dotnet-GitSync-Bot added the area-Infrastructure-libraries label Mar 18, 2020

lpereira reviewed Mar 18, 2020

View reviewed changes

eng/native/configurecompiler.cmake Outdated Show resolved Hide resolved

Unify some cmake function definitions

27eef50

am11 marked this pull request as ready for review March 20, 2020 04:21

jkotas reviewed Mar 20, 2020

View reviewed changes

jkotas approved these changes Mar 20, 2020

View reviewed changes

am11 added 2 commits March 20, 2020 07:07

Address CR feedback

5324f02

Make installers /Zc:strictStrings compatible

4fc15c2

janvorli reviewed Mar 20, 2020

View reviewed changes

am11 added 5 commits March 20, 2020 12:39

Make installer /Gz compatible

2264fa0

Remove skip-strip internal implementation

f193079

Use CLR_CMAKE_TARGET_DARWIN

ee03fc0

Establish iOS to Darwin relationship

b861b05

Simplify iOS conditions

1b2b5c0

janvorli approved these changes Mar 20, 2020

View reviewed changes

am11 mentioned this pull request Mar 21, 2020

Several tests fail on mono with mono_threads_pthread_kill: pthread_kill failed with error 11 #32377

Closed

jkotas merged commit b035137 into dotnet:master Mar 21, 2020

am11 deleted the feature/cmake-consolidations branch March 21, 2020 03:02

akoeplinger reviewed Mar 22, 2020

View reviewed changes

eng/native/configureplatform.cmake Show resolved Hide resolved

akoeplinger mentioned this pull request Mar 23, 2020

Rename TARGET_DARWIN to TARGET_OSX #33959

Merged

akoeplinger added a commit that referenced this pull request Mar 24, 2020

Rename TARGET_DARWIN to TARGET_OSX (#33959)

3d8073d

#33716 set TARGET_DARWIN for iOS too but most of the logic is really OSX-specific. Unset TARGET_DARWIN for iOS and rename the rest of the occurrences to TARGET_OSX.

echesakov mentioned this pull request Mar 26, 2020

Native debugging with symbols is broken on Linux/arm64 #34108

Closed

elinor-fung mentioned this pull request Apr 4, 2020

Fix host linker options to have -Bsymbolic #34534

Merged

elinor-fung mentioned this pull request Jun 30, 2020

Fix packaging of PDBs for host binaries #38589

Merged

ghost locked as resolved and limited conversation to collaborators Dec 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify some cmake function definitions #33716

Unify some cmake function definitions #33716

am11 commented Mar 18, 2020

am11 commented Mar 20, 2020

jkotas Mar 20, 2020

am11 Mar 20, 2020

jkotas Mar 20, 2020

am11 Mar 20, 2020 •

edited

Loading

jkotas Mar 20, 2020

am11 Mar 20, 2020

jkotas Mar 20, 2020

am11 Mar 20, 2020

am11 Mar 20, 2020

jkotas left a comment

janvorli Mar 20, 2020

am11 Mar 20, 2020

janvorli Mar 20, 2020

am11 Mar 20, 2020

janvorli left a comment

am11 commented Mar 21, 2020

am11 commented Mar 21, 2020

Unify some cmake function definitions #33716

Unify some cmake function definitions #33716

Conversation

am11 commented Mar 18, 2020

am11 commented Mar 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

am11 Mar 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janvorli left a comment

Choose a reason for hiding this comment

am11 commented Mar 21, 2020

am11 commented Mar 21, 2020

am11 Mar 20, 2020 •

edited

Loading