[PATCH] D44753: [Preprocessor] Rename __is_{target -> host}_* function-like builtin macros

[John Ericson via Phabricator via cfe-commits]

Ericson2314 added a comment.

Bummer, I didn't realize this had already shipped.

I'm happy to discuss on the mailing list. Indeed I responded there first 
  but it would 
appear nobody saw my message. Should I reply to my own message with a summary 
and link of this read, or would one of you like to do that?


Repository:
  rC Clang

https://reviews.llvm.org/D44753



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44753: [Preprocessor] Rename __is_{target -> host}_* function-like builtin macros

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith requested changes to this revision.
dexonsmith added a comment.
This revision now requires changes to proceed.

I agree with Saleem and Bob: `__is_target_*` is not confusing here and seems to 
be a straightforward spelling.  It has also already shipped in LLVM 6.0.0: it 
would be awkward to stop supporting this syntax.

Regardless, it's not clear that this patch is the right direction (i.e., we're 
not discussing the patch at all right now).  I suggest moving the discussion 
back to the wider audience on cfe-dev until we have consensus for a change.


Repository:
  rC Clang

https://reviews.llvm.org/D44753



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44753: [Preprocessor] Rename __is_{target -> host}_* function-like builtin macros

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added a comment.

I may be a bit biased but I agree with @bob.wilson and @steven_wu.  The current 
names are better from the user’s perspective.  GCC’s build is a very bad 
example as it has runtime components built as part of it (libgcc).  When 
building any code, even in a Canadian cross-compile, the target will always be 
what you are running on.  The preprocessor macros are part of the code that you 
are building for a given target.  The association with the command line option 
makes it more obvious what it is going to use to determine the value.  Having a 
pithy name should also be considered a design goal.  Recreating new terminology 
only muddles the problem.

Even if you are compiling a compiler, there is nothing special.  It is a 
standard user space program that will run on a specific target.  Even if you 
treat it as a perspective of the program, if you bootstrap on Linux, the 
bootstrapping compiler’s Target will be Linux even if the final compiler has a 
target of Windows.  The compiler is answering from the perspective of the 
program :)


Repository:
  rC Clang

https://reviews.llvm.org/D44753



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44753: [Preprocessor] Rename __is_{target -> host}_* function-like builtin macros

[John Ericson via Phabricator via cfe-commits]

Ericson2314 added a comment.

@steven_wu Maybe there is something outside of "build" "host" or "target" that 
won't suffer from these problems of vantage point? `__will_be_built_for_*` for 
a very lengthy example, but hopefully something shorter too?


Repository:
  rC Clang

https://reviews.llvm.org/D44753



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44753: [Preprocessor] Rename __is_{target -> host}_* function-like builtin macros

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

I am not trying to discuss which english word is best here. My point is simply:

1. macros are evaluated during compile time
2. "host"means either the platform you compiled on during compile time or the 
platform you run on during the runtime
3. __is_host_* is not a good name, because it is misleading as it either 
implies "runtime" as a compile-time constant, or indicates the wrong platform.


Repository:
  rC Clang

https://reviews.llvm.org/D44753



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44753: [Preprocessor] Rename __is_{target -> host}_* function-like builtin macros

[John Ericson via Phabricator via cfe-commits]

Ericson2314 added a comment.

> I'm sure I could fine a GCC example

FWIW  
https://github.com/gcc-mirror/gcc/blob/master/gcc/config/aarch64/aarch64.c vs 
https://github.com/gcc-mirror/gcc/blob/master/gcc/common/config/aarch64/aarch64-common.c

@steven_wu

> It is not about matching command line name to builtin marco name.

Whew :)

> "target" is the platform we are compiling for, whether it is host or device 
> or something else.

I'm in total agreement, if we are saying that from the compiler's perspective.

> It is a different concept when you talking about cross-compiling, which 
> "target" is strictly not host and "build" or "host" doesn't matter to 
> compiler at all.

So to be clear, I don't think there is a legitament reason *why* GHC and GCC 
care about the target platform at compile time. LLVM's approaching of always 
being multi-target and only choosing at run-time is far superior. Part of the 
philosophy behind that approach is moving towards a world where everything just 
works whether cross compiling or not. Redefining terminology based on whether 
we are cross compiling is counter to that goal.

> This example is bad because you do not know about runtime when you do 
> compilation.

I'm intrigued you singled out my first `#if` and not my second. If the binary 
is compiled with `clang` targeting windows, then (absent wine or something) we 
can be sure it is running on windows. On the other hand it seems odd and 
gcc-like to decide at compile time whether the newly built binary is targeting 
Darwin. The "emit" platform of the newly-built binary is strictly more removed 
from `clang`'s purview than the "run" platform.


Repository:
  rC Clang

https://reviews.llvm.org/D44753



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44753: [Preprocessor] Rename __is_{target -> host}_* function-like builtin macros

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

It is not about matching command line name to builtin marco name. "target" is 
the platform we are compiling for, whether it is host or device or something 
else. It is a different concept when you talking about cross-compiling, which 
"target" is strictly not host and "build" or "host" doesn't matter to compiler 
at all.

> #if __is_run(window)
> 
>   printf("Hello, Satya");
> 
> #elif __is_run(darwin)
> 
>   printf("Hello, Tim");
> 
> #else
> 
>   prinf("Unclear who I am talking too.");
> 
> #endif

This example is bad because you do not know about runtime when you do 
compilation. Putting runtime environment onto #if is just wrong in many ways. 
If autoconf really has to name it to something else, you can always write a 
"#define" to rename __is_target.


Repository:
  rC Clang

https://reviews.llvm.org/D44753



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44753: [Preprocessor] Rename __is_{target -> host}_* function-like builtin macros

[John Ericson via Phabricator via cfe-commits]

Ericson2314 added a comment.

@steven_wu but what you say is directly contradicted by the GHC example. I'm 
sure I could fine a GCC example too where some macro with "target" is in the 
name affects the target of the compiler being built. In the vast majority of 
programs, no more than one platform need affect preprocessing, but when 
multiple platforms affect preprocessing, they are *always* named from the 
perspective of the being-built tool being run.


Repository:
  rC Clang

https://reviews.llvm.org/D44753



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44753: [Preprocessor] Rename __is_{target -> host}_* function-like builtin macros

[John Ericson via Phabricator via cfe-commits]

Ericson2314 added a comment.

One that that might make my position clearer is to substitute the name "build", 
"host", and "target" for "build", "run", and "emit". [A colleague of mine 
proposed these alternative names and I do think they are vastly more human 
friendly.]

Then if we write

  int main(void) {
  
  #if __is_run(window)
printf("Hello, Satya");
  #elif __is_run(darwin)
printf("Hello, Tim");
  #else
prinf("Unclear who I am talking too.");
  #endif
  
  #if __is_emit(darwin)
#error "What's a Mach-O?"
  #else
/* do something with binutils */
  #endif
  
return 0;
  }

and run

  clang -emit something main.c

it is clear the intention is *not* for `-emit` to control `__is_emit`.

To me, this makes clear that the problem isn't the name-shift I am proposing, 
but the inherent vagary of the terms "host" and "target" relative to their 
specific meaning in Autoconf's jargon.


Repository:
  rC Clang

https://reviews.llvm.org/D44753



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44753: [Preprocessor] Rename __is_{target -> host}_* function-like builtin macros

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

I disagree. I think "target" is the correct name, even for cross compiling. For 
something compiled with -target foo, we are consistent calling it "target" 
during compile time. It only becomes appropriate to call it "host" during the 
runtime of the executable. There is no such concept as "host" when you are 
doing cross compiling.

The builtin macros are compile time constant, so following the compile time 
naming is much better.


Repository:
  rC Clang

https://reviews.llvm.org/D44753



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44753: [Preprocessor] Rename __is_{target -> host}_* function-like builtin macros

[John Ericson via Phabricator via cfe-commits]

Ericson2314 added a comment.

In https://reviews.llvm.org/D44753#1044935, @bob.wilson wrote:

> Sorry that I missed your earlier comment about this. The confusion could only 
> arise in the context of a tool (like a compiler) that is being used for 
> cross-compilation. That is a small fraction of the audience for Clang, and we 
> should design this in a way that makes the most sense for the majority of 
> users. If there's a naming scheme that is better for both, then we should do 
> that, but I don't think this is it.


I agree. But I believe mine no worse for both, and significantly better for the 
compiling-a-compiler case.

> When dealing with a cross compiler, there is a need to distinguish the 
> "target" where the compiler will run (which as you point out is typically 
> referred to as the "host") from the "target" code produced by that cross 
> compiler.

I Agree.

> There are two points in time: (1) when compiling the cross compiler, and (2) 
> when running the cross compiler. In step (1), the compiler will be invoked 
> with a "-target" option that specifies the "host".

I prefer not to think times of points of time but in terms of different 
programs having different perspectives. The bootstrapping compiler was built on 
A, runs on B, and is passed `-target` for C. The new compiler was built on B, 
runs on C, and targets some set D... (which is not constrained). So the two 
compilers' frame of reference is shifted by 1, but the frame of reference per 
compiler is constant whether we are building it or running it.

> The compiler option name will be "-target" regardless. Using "target" names 
> in the macros is consistent with that compiler option name.



> The obvious connection between these macros and the value specified by the 
> "-target" option would be lost.

So I do wonder if `-target` was the best name, but agreed that ship has long 
since sailed.

Furthermore, with the way of per-compiler, not per-time thinking I described 
above, one can reconcile the `-target` flag with the autoconf terminology by 
saying is specifying the "target" of the compiler, not the "target" of the 
thing being built. Indeed, might build the new compiler like

  ./Configure 'CC=clang -target foo-bar-baz` --host foo-bar-baz --target 
alpha-beta-gamma
  make



> The preprocessor checks are compile-time checks, so there no way that one of 
> these macros in the source code of the compiler itself could be referring to 
> the target in step (2).

Yes, agreed clang won't know what the target of the compiler being built is 
(for that would be clang's "post target"). The problem with the status quo is 
that the new compiler's build system will define its own macros, and those will 
clash with this in very confusing ways.

For example, check out this file of GHC's 
https://github.com/ghc/ghc/blob/master/compiler/ghc.mk#L155-L192. The 
`__is_target_*` macros made by LLVM would correspond to to the `*_HOST_*` 
macros produces by the build system and *not* the `*_TARGET_*` ones.


Repository:
  rC Clang

https://reviews.llvm.org/D44753



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44753: [Preprocessor] Rename __is_{target -> host}_* function-like builtin macros

[Bob Wilson via Phabricator via cfe-commits]

bob.wilson added a comment.

Sorry that I missed your earlier comment about this. The confusion could only 
arise in the context of a tool (like a compiler) that is being used for 
cross-compilation. That is a small fraction of the audience for Clang, and we 
should design this in a way that makes the most sense for the majority of 
users. If there's a naming scheme that is better for both, then we should do 
that, but I don't think this is it.

When dealing with a cross compiler, there is a need to distinguish the "target" 
where the compiler will run (which as you point out is typically referred to as 
the "host") from the "target" code produced by that cross compiler. There are 
two points in time: (1) when compiling the cross compiler, and (2) when running 
the cross compiler. In step (1), the compiler will be invoked with a "-target" 
option that specifies the "host". The preprocessor checks are compile-time 
checks, so there no way that one of these macros in the source code of the 
compiler itself could be referring to the target in step (2). The compiler 
option name will be "-target" regardless. Using "target" names in the macros is 
consistent with that compiler option name.

When dealing with anything other than a cross compiler (or similar cross-target 
development tool), the "host" terminology is not commonly used. The obvious 
connection between these macros and the value specified by the "-target" option 
would be lost. I really don't think this is a good alternative.


Repository:
  rC Clang

https://reviews.llvm.org/D44753



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44753: [Preprocessor] Rename __is_{target -> host}_* function-like builtin macros

[John Ericson via Phabricator via cfe-commits]

Ericson2314 created this revision.
Ericson2314 added reviewers: arphaman, compnerd, dexonsmith, bob.wilson, 
steven_wu.
Herald added subscribers: cfe-commits, javed.absar.

Per my belated [reply] to the mailing list, I believe the "target"
nomenclature incorrect for cross compilation.

[reply]: http://lists.llvm.org/pipermail/cfe-dev/2018-March/057258.html


Repository:
  rC Clang

https://reviews.llvm.org/D44753

Files:
  include/clang/Lex/Preprocessor.h
  lib/Lex/PPMacroExpansion.cpp
  test/Preprocessor/is_host.c
  test/Preprocessor/is_host_arm.c
  test/Preprocessor/is_host_arm64.c
  test/Preprocessor/is_host_environment_version.c
  test/Preprocessor/is_host_os_darwin.c
  test/Preprocessor/is_host_unknown.c
  test/Preprocessor/is_target.c
  test/Preprocessor/is_target_arm.c
  test/Preprocessor/is_target_arm64.c
  test/Preprocessor/is_target_environment_version.c
  test/Preprocessor/is_target_os_darwin.c
  test/Preprocessor/is_target_unknown.c

Index: test/Preprocessor/is_target.c
===
--- test/Preprocessor/is_target.c
+++ /dev/null
@@ -1,67 +0,0 @@
-// RUN: %clang_cc1 -fsyntax-only -triple x86_64-apple-darwin-simulator -verify %s
-
-#if !__is_target_arch(x86_64) || !__is_target_arch(X86_64)
-  #error "mismatching arch"
-#endif
-
-#if __is_target_arch(arm64)
-  #error "mismatching arch"
-#endif
-
-// Silently ignore invalid archs. This will ensure that older compilers will
-// accept headers that support new arches/vendors/os variants.
-#if __is_target_arch(foo)
-  #error "invalid arch"
-#endif
-
-#if !__is_target_vendor(apple) || !__is_target_vendor(APPLE)
-  #error "mismatching vendor"
-#endif
-
-#if __is_target_vendor(unknown)
-  #error "mismatching vendor"
-#endif
-
-#if __is_target_vendor(foo)
-  #error "invalid vendor"
-#endif
-
-#if !__is_target_os(darwin) || !__is_target_os(DARWIN)
-  #error "mismatching os"
-#endif
-
-#if __is_target_os(ios)
-  #error "mismatching os"
-#endif
-
-#if __is_target_os(foo)
-  #error "invalid os"
-#endif
-
-#if !__is_target_environment(simulator) || !__is_target_environment(SIMULATOR)
-  #error "mismatching environment"
-#endif
-
-#if __is_target_environment(unknown)
-  #error "mismatching environment"
-#endif
-
-#if __is_target_environment(foo)
-  #error "invalid environment"
-#endif
-
-#if !__has_builtin(__is_target_arch) || !__has_builtin(__is_target_os) || !__has_builtin(__is_target_vendor) || !__has_builtin(__is_target_environment)
-  #error "has builtin doesn't work"
-#endif
-
-#if __is_target_arch(11) // expected-error {{builtin feature check macro requires a parenthesized identifier}}
-  #error "invalid arch"
-#endif
-
-#if __is_target_arch x86 // expected-error{{missing '(' after '__is_target_arch'}}
-  #error "invalid arch"
-#endif
-
-#if __is_target_arch ( x86  // expected-error {{unterminated function-like macro invocation}}
-  #error "invalid arch"
-#endif // expected-error@-2 {{expected value in expression}}
Index: test/Preprocessor/is_host_unknown.c
===
--- test/Preprocessor/is_host_unknown.c
+++ test/Preprocessor/is_host_unknown.c
@@ -2,21 +2,21 @@
 // RUN: %clang_cc1 -fsyntax-only -triple i686-- -verify %s
 // expected-no-diagnostics
 
-#if __is_target_arch(unknown)
+#if __is_host_arch(unknown)
   #error "mismatching arch"
 #endif
 
 // Unknown vendor is allowed.
-#if !__is_target_vendor(unknown)
+#if !__is_host_vendor(unknown)
   #error "mismatching vendor"
 #endif
 
 // Unknown OS is allowed.
-#if !__is_target_os(unknown)
+#if !__is_host_os(unknown)
   #error "mismatching OS"
 #endif
 
 // Unknown environment is allowed.
-#if !__is_target_environment(unknown)
+#if !__is_host_environment(unknown)
   #error "mismatching environment"
 #endif
Index: test/Preprocessor/is_host_os_darwin.c
===
--- test/Preprocessor/is_host_os_darwin.c
+++ test/Preprocessor/is_host_os_darwin.c
@@ -4,22 +4,22 @@
 // RUN: %clang_cc1 -fsyntax-only -triple x86_64-apple-watchos -verify %s
 // expected-no-diagnostics
 
-#if !__is_target_os(darwin)
+#if !__is_host_os(darwin)
   #error "mismatching os"
 #endif
 
 // macOS matches both macOS and macOSX.
 #ifdef MAC
 
-#if !__is_target_os(macos)
+#if !__is_host_os(macos)
   #error "mismatching os"
 #endif
 
-#if !__is_target_os(macosx)
+#if !__is_host_os(macosx)
   #error "mismatching os"
 #endif
 
-#if __is_target_os(ios)
+#if __is_host_os(ios)
   #error "mismatching os"
 #endif
 
Index: test/Preprocessor/is_host_environment_version.c
===
--- test/Preprocessor/is_host_environment_version.c
+++ test/Preprocessor/is_host_environment_version.c
@@ -1,6 +1,6 @@
 // RUN: %clang_cc1 -fsyntax-only -triple x86_64-pc-windows-msvc18.0.0 -verify %s
 // expected-no-diagnostics
 
-#if !__is_target_environment(msvc)
+#if !__is_host_environment(msvc)
   #error "mismatching environment"
 #endif