Skip to content

Conversation

ZijunZhaoCCK
Copy link
Contributor

Add isWasm() check for here: #78655 (comment)

@ZijunZhaoCCK ZijunZhaoCCK requested a review from MaskRay February 6, 2024 02:05
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Feb 6, 2024
@llvmbot
Copy link
Member

llvmbot commented Feb 6, 2024

@llvm/pr-subscribers-clang

Author: None (ZijunZhaoCCK)

Changes

Add isWasm() check for here: #78655 (comment)


Full diff: https://github.com/llvm/llvm-project/pull/80783.diff

1 Files Affected:

  • (modified) clang/lib/Driver/Driver.cpp (+12-10)
diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index 29db9543f3655..04d02ea500d19 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -1443,16 +1443,18 @@ Compilation *Driver::BuildCompilation(ArrayRef<const char *> ArgList) {
   const ToolChain &TC = getToolChain(
       *UArgs, computeTargetTriple(*this, TargetTriple, *UArgs));
 
-  // Check if the environment version is valid.
-  llvm::Triple Triple = TC.getTriple();
-  StringRef TripleVersionName = Triple.getEnvironmentVersionString();
-  StringRef TripleObjectFormat =
-      Triple.getObjectFormatTypeName(Triple.getObjectFormat());
-  if (Triple.getEnvironmentVersion().empty() && TripleVersionName != "" &&
-      TripleVersionName != TripleObjectFormat) {
-    Diags.Report(diag::err_drv_triple_version_invalid)
-        << TripleVersionName << TC.getTripleString();
-    ContainsError = true;
+  // Check if the environment version is valid except wasm case.
+  if (!TC.getTriple().isWasm()) {
+    llvm::Triple Triple = TC.getTriple();
+    StringRef TripleVersionName = Triple.getEnvironmentVersionString();
+    StringRef TripleObjectFormat =
+        Triple.getObjectFormatTypeName(Triple.getObjectFormat());
+    if (Triple.getEnvironmentVersion().empty() && TripleVersionName != "" &&
+        TripleVersionName != TripleObjectFormat) {
+      Diags.Report(diag::err_drv_triple_version_invalid)
+          << TripleVersionName << TC.getTripleString();
+      ContainsError = true;
+    }
   }
 
   // Report warning when arm64EC option is overridden by specified target

@llvmbot
Copy link
Member

llvmbot commented Feb 6, 2024

@llvm/pr-subscribers-clang-driver

Author: None (ZijunZhaoCCK)

Changes

Add isWasm() check for here: #78655 (comment)


Full diff: https://github.com/llvm/llvm-project/pull/80783.diff

1 Files Affected:

  • (modified) clang/lib/Driver/Driver.cpp (+12-10)
diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index 29db9543f36553..04d02ea500d19f 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -1443,16 +1443,18 @@ Compilation *Driver::BuildCompilation(ArrayRef<const char *> ArgList) {
   const ToolChain &TC = getToolChain(
       *UArgs, computeTargetTriple(*this, TargetTriple, *UArgs));
 
-  // Check if the environment version is valid.
-  llvm::Triple Triple = TC.getTriple();
-  StringRef TripleVersionName = Triple.getEnvironmentVersionString();
-  StringRef TripleObjectFormat =
-      Triple.getObjectFormatTypeName(Triple.getObjectFormat());
-  if (Triple.getEnvironmentVersion().empty() && TripleVersionName != "" &&
-      TripleVersionName != TripleObjectFormat) {
-    Diags.Report(diag::err_drv_triple_version_invalid)
-        << TripleVersionName << TC.getTripleString();
-    ContainsError = true;
+  // Check if the environment version is valid except wasm case.
+  if (!TC.getTriple().isWasm()) {
+    llvm::Triple Triple = TC.getTriple();
+    StringRef TripleVersionName = Triple.getEnvironmentVersionString();
+    StringRef TripleObjectFormat =
+        Triple.getObjectFormatTypeName(Triple.getObjectFormat());
+    if (Triple.getEnvironmentVersion().empty() && TripleVersionName != "" &&
+        TripleVersionName != TripleObjectFormat) {
+      Diags.Report(diag::err_drv_triple_version_invalid)
+          << TripleVersionName << TC.getTripleString();
+      ContainsError = true;
+    }
   }
 
   // Report warning when arm64EC option is overridden by specified target

@MaskRay
Copy link
Member

MaskRay commented Feb 6, 2024

Add a test?

@ZijunZhaoCCK
Copy link
Contributor Author

Add a test?

Done. Add several tests.

@MaskRay
Copy link
Member

MaskRay commented Feb 6, 2024

wasm tests in android-* seem weird. A wasm file should be used

Copy link
Member

@MaskRay MaskRay left a comment

Choose a reason for hiding this comment

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

LGTM with glandium's suggestion.

@ZijunZhaoCCK ZijunZhaoCCK merged commit fbded66 into llvm:main Feb 6, 2024
@ZijunZhaoCCK ZijunZhaoCCK deleted the checkversion-update branch February 6, 2024 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants