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

Rename cross compilation related defines #2256

Merged
merged 3 commits into from
Feb 1, 2020

Conversation

sdmaclea
Copy link
Contributor

@sdmaclea sdmaclea commented Jan 27, 2020

Rename cross compilation related defines

To drive explicit and consitent usage of the terms TARGET and HOST, and
to remove usage of reserved identifiers, rename several macro defines

  _ARM_                -> HOST_ARM       in src/coreclr
  _ARM64_              -> HOST_ARM64     in src/coreclr
  _AMD64_              -> HOST_AMD64     in src/coreclr
  _X86_                -> HOST_X86       in src/coreclr
  _HOST_UNIX_          -> HOST_UNIX      in src/coreclr
  _HOST_AMD64_         -> HOST_AMD64     in src/coreclr
  _HOST_ARM64_         -> HOST_ARM64     in src/coreclr
  _HOST_ARM_           -> HOST_ARM       in src/coreclr
  _HOST_X86_           -> HOST_X86       in src/coreclr
  DBG_TARGET_AMD64     -> TARGET_AMD64   in src/coreclr
  DBG_TARGET_ARM64     -> TARGET_ARM64   in src/coreclr
  DBG_TARGET_ARM       -> TARGET_ARM     in src/coreclr
  DBG_TARGET_X86       -> TARGET_X86     in src/coreclr
  _TARGET_AMD64_       -> TARGET_AMD64   in src/coreclr
  _TARGET_ARM_         -> TARGET_ARM     in src/coreclr
  _TARGET_ARM64_       -> TARGET_ARM64   in src/coreclr
  _TARGET_X86_         -> TARGET_X86     in src/coreclr
  _TARGET_ARMARCH_     -> TARGET_ARMARCH in src/coreclr
  _TARGET_XARCH_       -> TARGET_XARCH   in src/coreclr
  _HOST_64BIT_         -> HOST_64BIT     in src/coreclr
  BIT64                -> HOST_64BIT     in src/coreclr
  _TARGET_64BIT_       -> TARGET_64BIT   in src/coreclr
  DBG_TARGET_64BIT     -> TARGET_64BIT   in src/coreclr
  HOST_IS_WINDOWS_OS   -> HOST_WINDOWS   in src/coreclr
  PLATFORM_UNIX        -> HOST_UNIX      in src/coreclr/*.cmake
  PLATFORM_WINDOWS     -> HOST_WINDOWS   in src/coreclr/*.cmake
  _TARGET_MAC64        -> TARGET_OSX     in src/coreclr
  FEATURE_PAL          -> TARGET_UNIX    in src/coreclr
  _TARGET_UNIX_        -> TARGET_UNIX    in src/coreclr
  _TARGET_WINDOWS_     -> TARGET_WINDOWS in src/coreclr
  PLATFORM_UNIX        -> TARGET_UNIX    in src/coreclr

The first commit is purely a big search and replace.
The second commit is changes necessary to fix compilation issues.
The third commit is clang-format of the src/jit directory.

A future PR will rename some of the TARGET_* to HOST_* and vice-versa as needed. This mostly a brute force search and replace.

@sdmaclea sdmaclea self-assigned this Jan 27, 2020
@@ -60,7 +60,7 @@
<DefineConstants Condition="'$(FeatureCominteropWinRTManagedActivation)' == 'true'">$(DefineConstants);FEATURE_COMINTEROP_WINRT_MANAGED_ACTIVATION</DefineConstants>
<DefineConstants Condition="'$(FeatureManagedEtw)' == 'true'">$(DefineConstants);FEATURE_MANAGED_ETW</DefineConstants>
<DefineConstants Condition="'$(FeatureManagedEtwChannels)' == 'true'">$(DefineConstants);FEATURE_MANAGED_ETW_CHANNELS</DefineConstants>
<DefineConstants Condition="'$(FeaturePal)' == 'true'">$(DefineConstants);FEATURE_PAL</DefineConstants>
<DefineConstants Condition="'$(FeaturePal)' == 'true'">$(DefineConstants);TARGET_UNIX</DefineConstants>
Copy link
Member

Choose a reason for hiding this comment

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

I would be nice to delete FeaturePal as well (in future PR).

@sdmaclea sdmaclea force-pushed the TARGET_UNIX branch 2 times, most recently from 5fbd3db to 6095894 Compare January 29, 2020 00:36
@sdmaclea sdmaclea changed the title Rename FEATURE_PAL & PLATFORM_* Rename cross compilation related defines Jan 29, 2020
@jkotas
Copy link
Member

jkotas commented Jan 29, 2020

BIT64 -> HOST_64BIT

We have choose TARGET_ as the default in all other place. Why use HOST_ as the default for this one?

@sdmaclea
Copy link
Contributor Author

sdmaclea commented Jan 29, 2020

BIT64 -> HOST_64BIT

We have choose TARGET_ as the default in all other place. Why use HOST_ as the default for this one?

It was currently set based on the host architecture. It looks wrong in most places, but I wanted to make this mostly a non-functional change. Given the JIT/crossgen supports cross bitness builds, I didn't want to mess with this here.

I also switched the PLATFORM_UNIX to map to HOST_UNIX on the similar rationale.

@jkotas
Copy link
Member

jkotas commented Jan 29, 2020

Ok, hope most them are going to be flipped to be TARGET_XXX in follow up work. I see places in the current delta where the HOST_XXX and TARGET_XXX mixes within same function or file are pretty confusing.

@sdmaclea
Copy link
Contributor Author

I certainly want to minimize HOST_XXX instances. They seem problematic outside the PAL. My guess is they are only likely to be right in JIT and zapper. I will definitely audit them.

@sdmaclea
Copy link
Contributor Author

sdmaclea commented Jan 29, 2020

I had originally been doing the bulk edit replaces in my editor. Turns out my editor was skipping some files for some reason....

Anyways I decided it was less error prone to script this. So I wrote a bash/git/sed script to replace a single editor within a git subtree. Then I wrote a script for all the bulk edits.

I kept the commits more separate to ease review burden. The bulk search replace are separate commits.

The manual edits are separate commits.

I replaced the BIT64 rename to TARGET_BIT64 except in cmake, zap and jit (which had no references).

This patch was a machine generated search and replace of
identifiers.  The replacement was done in the order listed
for the path specified.

A regex was used to find the identifiers.

s/(^|[^A-CE-Za-z0-9_])${find}($|[^A-Za-z0-9_])

The 'D' character was specifically allowed a s a prefix
to catch add_definitions(-D${find}).

The patch also reverts all managed code changes after replacement.

    _ARM_                -> HOST_ARM       in src/coreclr
    _ARM64_              -> HOST_ARM64     in src/coreclr
    _AMD64_              -> HOST_AMD64     in src/coreclr
    _X86_                -> HOST_X86       in src/coreclr
    _HOST_UNIX_          -> HOST_UNIX      in src/coreclr
    _HOST_AMD64_         -> HOST_AMD64     in src/coreclr
    _HOST_ARM64_         -> HOST_ARM64     in src/coreclr
    _HOST_ARM_           -> HOST_ARM       in src/coreclr
    _HOST_X86_           -> HOST_X86       in src/coreclr
    DBG_TARGET_AMD64     -> TARGET_AMD64   in src/coreclr
    DBG_TARGET_ARM64     -> TARGET_ARM64   in src/coreclr
    DBG_TARGET_ARM       -> TARGET_ARM     in src/coreclr
    DBG_TARGET_X86       -> TARGET_X86     in src/coreclr
    _TARGET_AMD64_       -> TARGET_AMD64   in src/coreclr
    _TARGET_ARM_         -> TARGET_ARM     in src/coreclr
    _TARGET_ARM64_       -> TARGET_ARM64   in src/coreclr
    _TARGET_X86_         -> TARGET_X86     in src/coreclr
    _TARGET_ARMARCH_     -> TARGET_ARMARCH in src/coreclr
    _TARGET_XARCH_       -> TARGET_XARCH   in src/coreclr
    _HOST_64BIT_         -> HOST_64BIT     in src/coreclr
    BIT64                -> HOST_64BIT     in src/coreclr
    _TARGET_64BIT_       -> TARGET_64BIT   in src/coreclr
    DBG_TARGET_64BIT     -> TARGET_64BIT   in src/coreclr
    HOST_IS_WINDOWS_OS   -> HOST_WINDOWS   in src/coreclr
    PLATFORM_UNIX        -> HOST_UNIX      in src/coreclr/*.cmake
    PLATFORM_WINDOWS     -> HOST_WINDOWS   in src/coreclr/*.cmake
    _TARGET_MAC64        -> TARGET_OSX     in src/coreclr
    FEATURE_PAL          -> TARGET_UNIX    in src/coreclr
    _TARGET_UNIX_        -> TARGET_UNIX    in src/coreclr
    _TARGET_WINDOWS_     -> TARGET_WINDOWS in src/coreclr
    PLATFORM_UNIX        -> TARGET_UNIX    in src/coreclr
    PLATFORM_WINDOWS     -> TARGET_WINDOWS in src/coreclr
@sdmaclea sdmaclea force-pushed the TARGET_UNIX branch 3 times, most recently from 4735c94 to 08e3951 Compare January 31, 2020 22:03
Remove unused defines

Remove BIT32
Remove DBG_TARGET_AMD64_UNIX
Remove DBG_TARGET_ARM64_UNIX
Remove DBG_TARGET_ARM_UNIX
Remove DBG_TARGET_32BIT

Fixes for HOST_<arch> rename

Move TARGET_<Arch> and TARGET_<bit>

Move from clrdefinitions.cmake
to configurecompiler.cmake so it is used globally.

More jit.h
@sdmaclea sdmaclea merged commit fcd862e into dotnet:master Feb 1, 2020
@sdmaclea sdmaclea deleted the TARGET_UNIX branch February 1, 2020 00:36
@BruceForstall
Copy link
Member

fwiw, this change deserved to be (and still deserves to be) much more widely before being merged. Anyone with in-progress changes that added architecture-specific code (for example), then rebased, might not get the behavior they expect because previously-working #ifdef _TARGET_ARM64_ would now be dead code.

@dotnet/jit-contrib fyi: many global #ifdefs have changed names.

Also, can we change TARGET_OSX to TARGET_MACOS? Apple replaced the name OSX with macOS in 2016.

@jkotas
Copy link
Member

jkotas commented Feb 4, 2020

Also, can we change TARGET_OSX to TARGET_MACOS?

There is discussion about it here: #31659 (comment) .

We have OSX name in the number of places today that we cannot change (e.g. https://docs.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.osplatform.osx?view=netframework-4.8).

If we want to change it to something, I would vote for DARWIN. DARWIN should be immune to marketing whims.

@AndyAyersMS
Copy link
Member

Can we make the old names cause errors for some interim period, so it's more obvious they won't work anymore? Otherwise stuff may just silently get dropped.

@CarolEidt
Copy link
Contributor

I filed #31732 to clean up some inconsistencies in the JIT

@sdmaclea
Copy link
Contributor Author

sdmaclea commented Feb 4, 2020

@dotnet/jit-contrib My apologies for not socializing this earlier. I had been concerned that my PR would shear with respect to master. That somehow I would have missed a rename.... I checked immediately after I merged so that I could prevent an outage. I didn't really anticipate the possibility that other PRs might go in after w/o having merge conflicts with this one....

@AndyAyersMS I am not sure how to make use of an undefined macro cause a build break.

Something like

#define _TARGET_ARM_ static_assert(BadDefine != BadDefine, "_TARGET_ARM_ macro is obsolete.  Use TARGET_ARM")

Might work in some circumstances, but not in all. I don't think it would work in the most common cases. Particularly:

#ifdef _TARGET_ARM_
#if defined(_TARGET_ARM_)

@erozenfeld
Copy link
Member

Will
#pragma deprecated("_TARGET_ARM_")
work?
From https://docs.microsoft.com/en-us/cpp/preprocessor/deprecated-c-cpp?view=vs-2019
You can deprecate macro names. Place the macro name in quotes or else macro expansion will occur.

@lpereira
Copy link
Contributor

lpereira commented Feb 5, 2020

For non-MSVC compilers, that #pragma won't work. In C (from C99 onwards at least), there's _Pragma(), and using it like so works in GCC and (probably) Clang:

#define WARN(msg) WARN2(GCC warning msg)
#define WARN2(s) _Pragma(#s)
#define DEPRECATE_MACRO WARN("Macro is deprecated")

And then use it like so:

#define _TARGET_ARM_ DEPRECATE_MACRO

I don't know, however, if there's something similar for C++ or even MSVC, though.

@janvorli
Copy link
Member

janvorli commented Feb 5, 2020

While this mechanism works with clang, it doesn't help our purpose. The problem is that it only warns for #if _TARGET_ARM_, but not for #ifdef _TARGET_ARM_. The latter is used in all the cases we have. Unfortunately, I don't see any way to create a warning for the #ifdef usage case.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants