-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[PowerPC][clang] Fix triple constructor ambiguity causing "unknown" target triple on AIX #147488
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
Conversation
arsenm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this testable? The TargetParser really shouldn't depend on the host triple for any reason
llvm/lib/TargetParser/Unix/Host.inc
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function shouldn't depend on the host triple in the first place
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a test file to lock this down.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function shouldn't depend on the host triple in the first place
This function intends to transfer host characteristics (the OS version information) to AIX target triples (which is then used in Clang for predefined macros, among other things). The host characteristics have no reasonable relation to the AIX target triple unless the compiler is hosted on AIX.
e2993cc to
d6c634b
Compare
|
@llvm/pr-subscribers-clang Author: Tony Varghese (tonykuttai) ChangesPR #145685 introduced constructor overload ambiguity in the Triple class, causing Used Full diff: https://github.com/llvm/llvm-project/pull/147488.diff 2 Files Affected:
diff --git a/clang/test/Driver/aix-default-target-triple.c b/clang/test/Driver/aix-default-target-triple.c
new file mode 100644
index 0000000000000..2802e6a60719a
--- /dev/null
+++ b/clang/test/Driver/aix-default-target-triple.c
@@ -0,0 +1,18 @@
+// Test for the Triple constructor ambiguity fix on AIX
+// This test verifies that the default target triple is correctly resolved
+// and doesn't fall back to "unknown" due to constructor ambiguity.
+
+// REQUIRES: system-aix
+// RUN: %clang -v %s -c 2>&1 | FileCheck %s --check-prefix=CHECK-TARGET
+
+// Test that the target triple contains AIX and is not "unknown"
+// The target should be something like "powerpc-ibm-aix7.3.0.0"
+// CHECK-TARGET: Target: {{.*}}aix{{.*}}
+// CHECK-TARGET-NOT: error: unknown target triple 'unknown'
+
+#include <stdio.h>
+
+int main() {
+ printf("Target triple resolution test passed\n");
+ return 0;
+}
diff --git a/llvm/lib/TargetParser/Unix/Host.inc b/llvm/lib/TargetParser/Unix/Host.inc
index ef9e288ee3a01..aeb2f59218700 100644
--- a/llvm/lib/TargetParser/Unix/Host.inc
+++ b/llvm/lib/TargetParser/Unix/Host.inc
@@ -55,7 +55,7 @@ static std::string updateTripleOSVersion(std::string TargetTripleString) {
// On AIX, the AIX version and release should be that of the current host
// unless if the version has already been specified.
if (Triple(LLVM_HOST_TRIPLE).getOS() == Triple::AIX) {
- Triple TT(std::move(TargetTripleString));
+ Triple TT{TargetTripleString};
if (TT.getOS() == Triple::AIX && !TT.getOSMajorVersion()) {
struct utsname name;
if (uname(&name) != -1) {
|
|
@llvm/pr-subscribers-clang-driver Author: Tony Varghese (tonykuttai) ChangesPR #145685 introduced constructor overload ambiguity in the Triple class, causing Used Full diff: https://github.com/llvm/llvm-project/pull/147488.diff 2 Files Affected:
diff --git a/clang/test/Driver/aix-default-target-triple.c b/clang/test/Driver/aix-default-target-triple.c
new file mode 100644
index 0000000000000..2802e6a60719a
--- /dev/null
+++ b/clang/test/Driver/aix-default-target-triple.c
@@ -0,0 +1,18 @@
+// Test for the Triple constructor ambiguity fix on AIX
+// This test verifies that the default target triple is correctly resolved
+// and doesn't fall back to "unknown" due to constructor ambiguity.
+
+// REQUIRES: system-aix
+// RUN: %clang -v %s -c 2>&1 | FileCheck %s --check-prefix=CHECK-TARGET
+
+// Test that the target triple contains AIX and is not "unknown"
+// The target should be something like "powerpc-ibm-aix7.3.0.0"
+// CHECK-TARGET: Target: {{.*}}aix{{.*}}
+// CHECK-TARGET-NOT: error: unknown target triple 'unknown'
+
+#include <stdio.h>
+
+int main() {
+ printf("Target triple resolution test passed\n");
+ return 0;
+}
diff --git a/llvm/lib/TargetParser/Unix/Host.inc b/llvm/lib/TargetParser/Unix/Host.inc
index ef9e288ee3a01..aeb2f59218700 100644
--- a/llvm/lib/TargetParser/Unix/Host.inc
+++ b/llvm/lib/TargetParser/Unix/Host.inc
@@ -55,7 +55,7 @@ static std::string updateTripleOSVersion(std::string TargetTripleString) {
// On AIX, the AIX version and release should be that of the current host
// unless if the version has already been specified.
if (Triple(LLVM_HOST_TRIPLE).getOS() == Triple::AIX) {
- Triple TT(std::move(TargetTripleString));
+ Triple TT{TargetTripleString};
if (TT.getOS() == Triple::AIX && !TT.getOSMajorVersion()) {
struct utsname name;
if (uname(&name) != -1) {
|
d6c634b to
859bdf0
Compare
…arget triple on AIX
859bdf0 to
9d9ed1f
Compare
arsenm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm with test nits
Co-authored-by: Matt Arsenault <[email protected]>
| // This test verifies that the default target triple is correctly resolved | ||
| // and doesn't fall back to "unknown" due to constructor ambiguity. | ||
|
|
||
| // REQUIRES: system-aix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tonykuttai, this is not sufficient. The target also has to be AIX to begin with (as opposed to an AIX-hosted cross-compiling configuration).
…ple.c for AIX specific targets (#147584) This is a followup on the [[PowerPC][clang] Fix triple constructor ambiguity causing "unknown" target triple on AIX](#147488 (comment)) to address the [post-commit review comment](#147488 (comment)). --------- Co-authored-by: Tony Varghese <[email protected]>
…-target-triple.c for AIX specific targets (#147584) This is a followup on the [[PowerPC][clang] Fix triple constructor ambiguity causing "unknown" target triple on AIX](llvm/llvm-project#147488 (comment)) to address the [post-commit review comment](llvm/llvm-project#147488 (comment)). --------- Co-authored-by: Tony Varghese <[email protected]>
PR #145685 introduced constructor overload ambiguity in the Triple class, causing
updateTripleOSVersion()to construct Triple objects withunknowninstead of the configured target triple (e.g.,powerpc-ibm-aix7.3.0.0). This results in Clang driver errors likeerror: unknown target triple 'unknown'.Used
Twineconstructor with braced initialization to bypass ambiguity.