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

Remove mentions of Rotor from codebase #20298

Merged
merged 16 commits into from Oct 9, 2018

Conversation

Projects
None yet
2 participants
@AustinWise
Copy link
Contributor

AustinWise commented Oct 8, 2018

Many references to Rotor in this codebase are confusing, as they tend to contrast Rotor's behavior with .NET Framework (neither of which matches CoreCLR) or reference compatibility problems with 15 year old compilers.

This series of commits should remove all mentions rotor from the codebase, except from the many non-compiling PAL tests. Many of them reflect a time when the PAL was built as a shared library, so once they are cleaned up they will be purged of the reference to librotor_pal.

Fixes #9872

AustinWise added some commits Oct 7, 2018

Moving parsing from TypeNameParser ctor to a separate method.
It seems a bit odd to have the constructor parsing and then use
a dummy method (MakeRotorHappy) to make it look more normal.
Remove CorMarkThreadInThreadPool.
It is neither referenced nor exported.
Remove reference to rotor from Strike/vm.cpp.
This file is only built for Windows.
Remove reference to rotor from debugreturn.h
This is the only file the defines these macros, so there is no need to
undef them first.
Remove references to deleted tests from DisabledTests.txt
I can't find any evidence that this file is actually used.
Remove dead and misleading code from profilinghelper.cpp.
FEATURE_PROFAPI_EVENT_LOGGING is always defined when PROFILING_SUPPORTED
is defined. And the entire contents of profilinghelper.cpp is surrounded
with "ifdef PROFILING_SUPPORTED". So all sections in
"ifndef FEATURE_PROFAPI_EVENT_LOGGING" are dead.

Furthermore, in coreclr this does not use the eventlog, so the macro name
is misleading.
Remove dead code in excep.cpp.
This entire function is surrounded with "ifndef FEATURE_PAL".
Remove refererences to rotor from safemath.h
This does not appear to cause any compile problems, so nobody was using
safemath.h without _ASSERTE defined.

Also S_SIZE_T_WP64BUG is not used anywhere.
Remove dead code from palclr.h.
I don't know why these check to see if the macro is undefined immediately
after defining them.

Also the comment appears to reference some unions that are no longer in
this file.
Expose ISymUnmanagedWriter2 from SymWriter as required by COM.
The comment talks about the C# compiler using this, however I cannot see
a way for the C# compiler to get an instance of this. It is only used
internally by AssemblyBuilder and not exposed otherwise.
@AustinWise

This comment has been minimized.

Copy link
Contributor Author

AustinWise commented Oct 8, 2018

Some of the changes might be more risky than others, so I have split this PR into many commits. That way if something is problematic, it can be easily reverted.

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Oct 8, 2018

Build break:

...\src\inc\safemath.h(275): error C3861: '_ASSERTE': identifier not found (compiling source file ...\src\ildasm\ceeload.cpp) [...\bin\obj\Windows_NT.x64.Checked\src\ildasm\exe\ildasm.vcxproj]

Restore check for _ASSERTE in safemath.h.
On Windows sometimes that this file is included without
_ASSERTE being defined. As the existing comment suggests, it appears
that SOS explicitly does not want _ASSERTE to do anything.
@AustinWise

This comment has been minimized.

Copy link
Contributor Author

AustinWise commented Oct 8, 2018

Interestingly, I did not have any trouble with building this on Linux. So that might mean linux builds have a few more asserts than Windows builds. Perhaps this is something worth investigating in the future?

I reverted my changes in safemath.h for now.

Show resolved Hide resolved src/inc/safemath.h Outdated
@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Oct 8, 2018

Looks good modulo a few nits. Thank you!

@jkotas

jkotas approved these changes Oct 8, 2018

Copy link
Member

jkotas left a comment

Thanks

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Oct 9, 2018

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test please

@jkotas jkotas merged commit 3f40e52 into dotnet:master Oct 9, 2018

28 of 32 checks passed

OSX10.12 x64 Checked Innerloop Build and Test Build finished.
Details
Ubuntu x64 Checked CoreFX Tests Build finished.
Details
Windows_NT x64 Checked CoreFX Tests Build finished.
Details
Windows_NT x64 Release CoreFX Tests Build finished.
Details
CROSS Check Build finished.
Details
CentOS7.1 x64 Checked Innerloop Build and Test Build finished.
Details
CentOS7.1 x64 Debug Innerloop Build Build finished.
Details
Linux-musl x64 Debug Build Build finished.
Details
Tizen armel Cross Checked Innerloop Build and Test Build finished.
Details
Ubuntu arm Cross Checked Innerloop Build and Test Build finished.
Details
Ubuntu arm Cross Checked crossgen_comparison Build and Test Build finished.
Details
Ubuntu arm Cross Checked no_tiered_compilation_innerloop Build and Test Build finished.
Details
Ubuntu arm Cross Release crossgen_comparison Build and Test Build finished.
Details
Ubuntu x64 Checked Innerloop Build and Test Build finished.
Details
Ubuntu x64 Checked Innerloop Build and Test (Jit - TieredCompilation=0) Build finished.
Details
Ubuntu x64 Formatting Build finished.
Details
Ubuntu16.04 arm64 Cross Checked Innerloop Build and Test Build finished.
Details
Ubuntu16.04 arm64 Cross Checked no_tiered_compilation_innerloop Build and Test Build finished.
Details
WIP ready for review
Details
Windows_NT arm Cross Debug Innerloop Build Build finished.
Details
Windows_NT arm64 Cross Debug Innerloop Build Build finished.
Details
Windows_NT x64 Checked Innerloop Build and Test Build finished.
Details
Windows_NT x64 Checked Innerloop Build and Test (Jit - TieredCompilation=0) Build finished.
Details
Windows_NT x64 Formatting Build finished.
Details
Windows_NT x64 full_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
Windows_NT x64 min_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
Windows_NT x86 Checked Innerloop Build and Test Build finished.
Details
Windows_NT x86 Checked Innerloop Build and Test (Jit - TieredCompilation=0) Build finished.
Details
Windows_NT x86 Release Innerloop Build and Test Build finished.
Details
Windows_NT x86 full_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
Windows_NT x86 min_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
license/cla All CLA requirements met.
Details

@AustinWise AustinWise deleted the AustinWise:austin/RemoveRotorCode branch Oct 9, 2018

A-And added a commit to A-And/coreclr that referenced this pull request Nov 20, 2018

Remove mentions of Rotor from codebase (dotnet#20298)
* Moving parsing from TypeNameParser ctor to a separate method.

It seems a bit odd to have the constructor parsing and then use
a dummy method (MakeRotorHappy) to make it look more normal.

* Remove CorMarkThreadInThreadPool.

It is neither referenced nor exported.

* Remove reference to rotor from securitywrapper.h

* Remove reference to rotor from Strike/vm.cpp.

This file is only built for Windows.

* Remove reference to rotor from debugreturn.h

This is the only file the defines these macros, so there is no need to
undef them first.

* Remove unused code refering to rotor from PAL.

* Remove references to Rotor from PAL.

* Remove references to deleted tests from DisabledTests.txt

I can't find any evidence that this file is actually used.

* Remove unneeded casts.

* Remove dead and misleading code from profilinghelper.cpp.

FEATURE_PROFAPI_EVENT_LOGGING is always defined when PROFILING_SUPPORTED
is defined. And the entire contents of profilinghelper.cpp is surrounded
with "ifdef PROFILING_SUPPORTED". So all sections in
"ifndef FEATURE_PROFAPI_EVENT_LOGGING" are dead.

Furthermore, in coreclr this does not use the eventlog, so the macro name
is misleading.

* Remove dead code in excep.cpp.

This entire function is surrounded with "ifndef FEATURE_PAL".

* Remove refererences to rotor from safemath.h

This does not appear to cause any compile problems, so nobody was using
safemath.h without _ASSERTE defined.

Also S_SIZE_T_WP64BUG is not used anywhere.

* Remove dead code from palclr.h.

I don't know why these check to see if the macro is undefined immediately
after defining them.

Also the comment appears to reference some unions that are no longer in
this file.

* Expose ISymUnmanagedWriter2 from SymWriter as required by COM.

The comment talks about the C# compiler using this, however I cannot see
a way for the C# compiler to get an instance of this. It is only used
internally by AssemblyBuilder and not exposed otherwise.

* Restore check for _ASSERTE in safemath.h.

On Windows sometimes that this file is included without
_ASSERTE being defined. As the existing comment suggests, it appears
that SOS explicitly does not want _ASSERTE to do anything.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment