[PATCH] D93668: [clang] Override the Fuchsia platform ABI using the triple environment

[Leonard Chan via Phabricator via cfe-commits]

leonardchan abandoned this revision.
leonardchan added a comment.

Abandoned in favor of D85802 .


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93668/new/

https://reviews.llvm.org/D93668

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

[PATCH] D93668: [clang] Override the Fuchsia platform ABI using the triple environment

[Roland McGrath via Phabricator via cfe-commits]

mcgrathr added a comment.

I am also a C++ programmer working on C++ ABI support.  I'm glad to be working 
on a platform that made many intentional design decisions to enable keeping the 
C++ ABI out of the platform ABI.  It makes it vastly easier to do interesting 
work on C++ ABIs such as what Leo is doing.  It also makes it significantly 
easier to do interesting work on platform ABIs, which I also do.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93668/new/

https://reviews.llvm.org/D93668

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

[PATCH] D93668: [clang] Override the Fuchsia platform ABI using the triple environment

[Petr Hosek via Phabricator via cfe-commits]

phosek accepted this revision.
phosek added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93668/new/

https://reviews.llvm.org/D93668

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

[PATCH] D93668: [clang] Override the Fuchsia platform ABI using the triple environment

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

I guess the spelling -fc++-abi= is intuitive, discoverable, and easy to use. 
I'd be OK going back to that, as long as the compiler knows which ABIs work on 
which targets and rejects the unsupported configurations.

Regarding the idea of separating the platform ABI from the C++ ABI, I can't say 
I really believe in the idea of language neutral platform ABIs. The platform 
ABI always incorporates aspects of the most widely used source languages, 
typically C. This is where LLVM gets into trouble when non-C frontends want to 
pass _Complex values or structs as arguments, for example. Whether you think of 
C++ features as being part of the platform or not probably depends on your view 
on whether C++ support is a critical part of the platform. I tend to think of 
C++ as pretty critical, but I'm a C++ programmer working on C++ ABI support, so 
that's my view.

Regarding reducing global ABI switches, yes, I would like to minimize them. We 
have many, and they have costs.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93668/new/

https://reviews.llvm.org/D93668

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

[PATCH] D93668: [clang] Override the Fuchsia platform ABI using the triple environment

[Roland McGrath via Phabricator via cfe-commits]

mcgrathr added a comment.

The Fuchsia platform ABI includes the SS & SCS ABI.  There is no compatibility 
issue with those.  PIC-friendly optimizations are purely an optimization and 
correctness issue for any PIC/PIE code, and is not an ABI issue.
The only thing where we have an incompatible difference in ABI between our 
compiler environments is the C++ ABI.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93668/new/

https://reviews.llvm.org/D93668

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

[PATCH] D93668: [clang] Override the Fuchsia platform ABI using the triple environment

[Petr Hosek via Phabricator via cfe-commits]

phosek added a comment.

In D93668#2510463 , @leonardchan wrote:

> Also, this probably isn't very important, but just as a heads up: if we were 
> to depend on the environment component in the triple as the flag for 
> selecting which multilib to use, then the multilib would be stored in a 
> directory containing that environment.
>
> So assuming we would check for the flag `-target x86_64-fuchsia-gnu` the 
> multilib path would be something like:
>
>   ${BUILD_DIR}/bin/../lib/x86_64-fuchsia-gnu/c++/compat+noexcept/
>
> rather than
>
>   ${BUILD_DIR}/bin/../lib/x86_64-fuchsia/c++/compat+noexcept/

I don't think we would use `compat` multilib in that case, instead we would 
simply use multiarch since these are now distinct targets. That is we would use 
just `${BUILD_DIR}/bin/../lib/x86_64-fuchsia-gnu/c++`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93668/new/

https://reviews.llvm.org/D93668

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

[PATCH] D93668: [clang] Override the Fuchsia platform ABI using the triple environment

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added a comment.

Also, this probably isn't very important, but just as a heads up: if we were to 
depend on the environment component in the triple as the flag for selecting 
which multilib to use, then the multilib would be stored in a directory 
containing that environment.

So assuming we would check for the flag `-target x86_64-fuchsia-gnu` the 
multilib path would be something like:

  ${BUILD_DIR}/bin/../lib/x86_64-fuchsia-gnu/c++/compat+noexcept/

rather than

  ${BUILD_DIR}/bin/../lib/x86_64-fuchsia/c++/compat+noexcept/


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93668/new/

https://reviews.llvm.org/D93668

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

[PATCH] D93668: [clang] Override the Fuchsia platform ABI using the triple environment

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added a comment.

In D93668#2492081 , @phosek wrote:

> I agree that if we want to allow selecting C++ ABI only, a flag like 
> `-fc++abi=` with some additional checks to disallow invalid combinations is 
> better. The question is whether we want to allow that in the first place? The 
> motivation behind this change is to support generating code that's ABI 
> compatible with the code generated by GCC. That could be expressed using the 
> target triple (one idea is to use `*-*-fuchsia-gnu`), which would cover both 
> C++ ABI as well as other aspects of the ABI (for example it could also 
> disable SafeStack/ShadowCallStack) so developers don't need to concern 
> themselves with all the details of the ABI, which could evolve over time 
> expanding the set of flags you'd need to pass to the compiler to generate 
> code that's ABI compatible with GCC.

In the context of representing the fourth component of the triple to represent 
the *platform* ABI rather than exclusively the C++ ABI, I think this would make 
sense. Using this other component would essentially just mean adding another 
layer of default flag configurations whose default values differ from those set 
by the first three components. I think we'd only want to have this extra layer 
if we have a sizable number of flags that we'd want separate from the default 
triple.

For our case, I think this mainly includes

- the C++ ABI (which encompasses "return this" on ctors/dtors and relative 
vtables),
- SS/SCS
- and maybe the PIC-friendly lookup tables if that ends up getting hidden 
behind a switch

but there could be new ABI changes down the line that could fall under this (or 
others that exist now but don't come to mind right now). Unless I'm missing 
some other features, this list seems fairly small and might not be worth 
tucking this under something that's meant to control a set of flags, but if we 
care more about being preventative from adding new flags, then it seems fine to 
just stick with using the triple.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93668/new/

https://reviews.llvm.org/D93668

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

[PATCH] D93668: [clang] Override the Fuchsia platform ABI using the triple environment

[Petr Hosek via Phabricator via cfe-commits]

phosek added a comment.

I agree that if we want to allow selecting C++ ABI only, a flag like 
`-fc++abi=` with some additional checks to disallow invalid combinations is 
better. The question is whether we want to allow that in the first place? The 
motivation behind this change is to support generating code that's ABI 
compatible with the code generated by GCC. That could be expressed using the 
target triple (one idea is to use `*-*-fuchsia-gnu`), which would cover both 
C++ ABI as well as other aspects of the ABI (for example it could also disable 
SafeStack/ShadowCallStack) so developers don't need to concern themselves with 
all the details of the ABI, which could evolve over time expanding the set of 
flags you'd need to pass to the compiler to generate code that's ABI compatible 
with GCC.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93668/new/

https://reviews.llvm.org/D93668

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

[PATCH] D93668: [clang] Override the Fuchsia platform ABI using the triple environment

[Leonard Chan via Phabricator via cfe-commits]

leonardchan updated this revision to Diff 315951.
leonardchan retitled this revision from "[clang] Add -ffuchsia-c++-abi flag to 
explicitly use the Fuchsia C++ ABI" to "[clang] Override the Fuchsia platform 
ABI using the triple environment".
leonardchan edited the summary of this revision.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93668/new/

https://reviews.llvm.org/D93668

Files:
  clang/include/clang/AST/ASTContext.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/test/CodeGenCXX/constructor-destructor-return-this.cpp

Index: clang/test/CodeGenCXX/constructor-destructor-return-this.cpp
===
--- clang/test/CodeGenCXX/constructor-destructor-return-this.cpp
+++ clang/test/CodeGenCXX/constructor-destructor-return-this.cpp
@@ -5,6 +5,8 @@
 //RUN:   | FileCheck --check-prefix=CHECKARM %s
 //RUN: %clang_cc1 %s -emit-llvm -o - -triple=x86_64-unknown-fuchsia | FileCheck --check-prefix=CHECKFUCHSIA %s
 //RUN: %clang_cc1 %s -emit-llvm -o - -triple=aarch64-unknown-fuchsia | FileCheck --check-prefix=CHECKFUCHSIA %s
+//RUN: %clang_cc1 %s -emit-llvm -o - -triple=x86_64-unknown-fuchsia-gnu | FileCheck --check-prefix=CHECKGEN_64 %s
+//RUN: %clang_cc1 %s -emit-llvm -o - -triple=aarch64-unknown-fuchsia-gnu | FileCheck --check-prefix=CHECKGEN_64 %s
 //RUN: %clang_cc1 %s -emit-llvm -o - -triple=i386-pc-win32 -fno-rtti | FileCheck --check-prefix=CHECKMS %s
 // FIXME: these tests crash on the bots when run with -triple=x86_64-pc-win32
 
@@ -52,6 +54,11 @@
 // CHECKFUCHSIA-LABEL: define{{.*}} %class.B* @_ZN1BD2Ev(%class.B* {{[^,]*}} returned {{[^,]*}} %this)
 // CHECKFUCHSIA-LABEL: define{{.*}} %class.B* @_ZN1BD1Ev(%class.B* {{[^,]*}} returned {{[^,]*}} %this)
 
+// CHECKGEN_64-LABEL: define{{.*}} void @_ZN1BC2EPi(%class.B* {{[^,]*}} %this, i32* %i)
+// CHECKGEN_64-LABEL: define{{.*}} void @_ZN1BC1EPi(%class.B* {{[^,]*}} %this, i32* %i)
+// CHECKGEN_64-LABEL: define{{.*}} void @_ZN1BD2Ev(%class.B* {{[^,]*}} %this)
+// CHECKGEN_64-LABEL: define{{.*}} void @_ZN1BD1Ev(%class.B* {{[^,]*}} %this)
+
 // CHECKMS-LABEL: define dso_local x86_thiscallcc %class.B* @"??0B@@QAE@PAH@Z"(%class.B* {{[^,]*}} returned {{[^,]*}} %this, i32* %i)
 // CHECKMS-LABEL: define dso_local x86_thiscallcc void @"??1B@@UAE@XZ"(%class.B* {{[^,]*}} %this)
 
@@ -98,6 +105,14 @@
 // CHECKFUCHSIA-LABEL: define{{.*}} void @_ZN1CD0Ev(%class.C* {{[^,]*}} %this)
 // CHECKFUCHSIA-LABEL: define{{.*}} void @_ZThn16_N1CD0Ev(%class.C* {{[^,]*}} %this)
 
+// CHECKGEN_64-LABEL: define{{.*}} void @_ZN1CC2EPiPc(%class.C* {{[^,]*}} %this, i32* %i, i8* %c)
+// CHECKGEN_64-LABEL: define{{.*}} void @_ZN1CC1EPiPc(%class.C* {{[^,]*}} %this, i32* %i, i8* %c)
+// CHECKGEN_64-LABEL: define{{.*}} void @_ZN1CD2Ev(%class.C* {{[^,]*}} %this)
+// CHECKGEN_64-LABEL: define{{.*}} void @_ZN1CD1Ev(%class.C* {{[^,]*}} %this)
+// CHECKGEN_64-LABEL: define{{.*}} void @_ZThn16_N1CD1Ev(%class.C* {{[^,]*}} %this)
+// CHECKGEN_64-LABEL: define{{.*}} void @_ZN1CD0Ev(%class.C* {{[^,]*}} %this)
+// CHECKGEN_64-LABEL: define{{.*}} void @_ZThn16_N1CD0Ev(%class.C* {{[^,]*}} %this)
+
 // CHECKMS-LABEL: define dso_local x86_thiscallcc %class.C* @"??0C@@QAE@PAHPAD@Z"(%class.C* {{[^,]*}} returned {{[^,]*}} %this, i32* %i, i8* %c)
 // CHECKMS-LABEL: define dso_local x86_thiscallcc void @"??1C@@UAE@XZ"(%class.C* {{[^,]*}} %this)
 
@@ -130,6 +145,11 @@
 // CHECKFUCHSIA-LABEL: define{{.*}} %class.D* @_ZN1DD2Ev(%class.D* {{[^,]*}} returned {{[^,]*}} %this, i8** %vtt)
 // CHECKFUCHSIA-LABEL: define{{.*}} %class.D* @_ZN1DD1Ev(%class.D* {{[^,]*}} returned {{[^,]*}} %this)
 
+// CHECKGEN_64-LABEL: define{{.*}} void @_ZN1DC2Ev(%class.D* {{[^,]*}} %this, i8** %vtt)
+// CHECKGEN_64-LABEL: define{{.*}} void @_ZN1DC1Ev(%class.D* {{[^,]*}} %this)
+// CHECKGEN_64-LABEL: define{{.*}} void @_ZN1DD2Ev(%class.D* {{[^,]*}} %this, i8** %vtt)
+// CHECKGEN_64-LABEL: define{{.*}} void @_ZN1DD1Ev(%class.D* {{[^,]*}} %this)
+
 // CHECKMS-LABEL: define dso_local x86_thiscallcc %class.D* @"??0D@@QAE@XZ"(%class.D* {{[^,]*}} returned {{[^,]*}} %this, i32 %is_most_derived)
 // CHECKMS-LABEL: define dso_local x86_thiscallcc void @"??1D@@UAE@XZ"(%class.D* {{[^,]*}} %this)
 
Index: clang/lib/CodeGen/ItaniumCXXABI.cpp
===
--- clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -543,7 +543,7 @@
 }
 
 CodeGen::CGCXXABI *CodeGen::CreateItaniumCXXABI(CodeGenModule ) {
-  switch (CGM.getTarget().getCXXABI().getKind()) {
+  switch (CGM.getContext().getCXXABIKind()) {
   // For IR-generation purposes, there's no significant difference
   // between the ARM and iOS ABIs.
   case TargetCXXABI::GenericARM:
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++