Fix clrcompression windows build #7840
Conversation
ianhays
added
2 - In Progress
area-System.IO.Compression
packaging
I'm not suggesting we not do this, but do we understand what, if any, perf impact there is to using the guard flag for this lib? If nothing else, if there is an impact, it'd be good to understand what that impact is in case we hear of it resulting in any regressions for folks. |
I was under the impression it was required or at least strongly encouraged so I wrote it off as inevitable. I'll run the Canterbury benchmarks and see if there's any appreciable difference. Edit: I saw no appreciable difference. Possibly a tenth to a hundredth of a millisecond but I didn't run enough iterations to get that level of definition. |
| @@ -10,7 +10,7 @@ | |||
| <ItemGroup> | |||
| <!-- use Win10 temporarily as the distinguishing factor, in reality the distinction is app-container vs not. --> | |||
Excellent. Thanks for checking. |
| @@ -62,10 +62,18 @@ include_directories("${CMAKE_BINARY_DIR}/../../") | |||
| add_library(clrcompression | |||
| SHARED | |||
| ${NATIVECOMPRESSION_SOURCES} | |||
| # This will add versioning to the library | |||
| # This will add versioning to the library | |||
|
Responded/fixed all feedback! Any more feedback/last words before I squash and merge? I'd like to merge by EOD to unblock #7756 |
|
All the changes except the Thanks for doing this! |
|
LGTM |
The Open Windows native build of clrcompression was inferior to the one in TFS for a few reasons: - The Open clrcompression imported vcruntime140.dll - It did not produce a pdb for either build config - The linker/compiler flags being used were incorrect or inadequate Those problems have been resolved. The clrcompression.dll produced in the open is now the same as the one produced internally with the exception that it is linked against the UCRT instead of a fixed msvcrt from TFS. I also took this opportunity to fix the appx package and add the /guard:cf flag which is missing from the TFS clrcompression.
|
Thanks for the feedback, Matt/Eric/Eric! |
The windows build already includes /Zi /Zl as part of commit 920fd2f (PR #7840). It looks like it was simply missed on Unix. This change also makes the native debug information closer to what CoreCLR does on all platforms. See dotnet/coreclr#3445 for more information. This is also needed for the end-to-end debuginfo generation as part of source-build. See dotnet/source-build#267
…kaging Fix clrcompression windows build Commit migrated from dotnet/corefx@fedba42
) The windows build already includes /Zi /Zl as part of commit 920fd2f (PR dotnet/corefx#7840). It looks like it was simply missed on Unix. This change also makes the native debug information closer to what CoreCLR does on all platforms. See dotnet/coreclr#3445 for more information. This is also needed for the end-to-end debuginfo generation as part of source-build. See dotnet/source-build#267 Commit migrated from dotnet/corefx@efe7652
The Open Windows native build of clrcompression was inferior to the one in TFS for a few reasons:
Those problems have been resolved. The clrcompression.dll produced in the open is now the same as the one produced internally with the exception that it is linked against the UCRT instead of a fixed msvcrt from TFS. I also took this opportunity to fix the appx package and add the /guard:cf flag which is missing from the TFS clrcompression.
resolves #7436
resolves #7748
related to #7756
@dagood @ellismg @gkhanna79