Make clang report invalid target versions.

[ZijunZhaoCCK]

Clang always silently ignores garbage target versions and this makes debug harder. So clang will report when target versions are invalid.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[pirama-arumuga-nainar]

@llvm/clang-vendors since this change can affect downstream components.

Can we do this change in two steps?

1. Since we want this error for Android, let's make a PR that restricts the error only to Android.
2. Make a follow-up PR to report error for all environments. We can revert/iterate on this PR until all downstream errors are handled.

[rnk]

This change requires tests

[llvmbot]

@llvm/pr-subscribers-clang

Author: None (ZijunZhaoCCK)

Changes

Clang always silently ignores garbage target versions and this makes debug harder. So clang will report when target versions are invalid.

---

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

3 Files Affected:

• (modified) clang/lib/Basic/Targets/OSTargets.h (+5)
• (modified) llvm/include/llvm/TargetParser/Triple.h (+4)
• (modified) llvm/lib/TargetParser/Triple.cpp (+13-1)

diff --git a/clang/lib/Basic/Targets/OSTargets.h b/clang/lib/Basic/Targets/OSTargets.h
index 342af4bbc42b7b..bc28066019971c 100644
--- a/clang/lib/Basic/Targets/OSTargets.h
+++ b/clang/lib/Basic/Targets/OSTargets.h
@@ -323,6 +323,11 @@ class LLVM_LIBRARY_VISIBILITY LinuxTargetInfo : public OSTargetInfo<Target> {
         // This historical but ambiguous name for the minSdkVersion macro. Keep
         // defined for compatibility.
         Builder.defineMacro("__ANDROID_API__", "__ANDROID_MIN_SDK_VERSION__");
+      } else {
+        llvm::errs() << "version "<< Triple.getVersionName() <<
+        " in triple " << Triple.getArchName() << "-" << Triple.getVendorName()
+        << "-" << Triple.getOSAndEnvironmentName() << " is invalid\n";
+        exit(1);
       }
     } else {
         Builder.defineMacro("__gnu_linux__");
diff --git a/llvm/include/llvm/TargetParser/Triple.h b/llvm/include/llvm/TargetParser/Triple.h
index 47904621c0967f..05df1c489ad06e 100644
--- a/llvm/include/llvm/TargetParser/Triple.h
+++ b/llvm/include/llvm/TargetParser/Triple.h
@@ -434,6 +434,10 @@ class Triple {
   /// string (separated by a '-' if the environment component is present).
   StringRef getOSAndEnvironmentName() const;
 
+  /// Get the version component of the environment component as a single
+  /// string (the version after the environment).
+  StringRef getVersionName() const;
+
   /// @}
   /// @name Convenience Predicates
   /// @{
diff --git a/llvm/lib/TargetParser/Triple.cpp b/llvm/lib/TargetParser/Triple.cpp
index ac04dab0489717..db4ba7100781bc 100644
--- a/llvm/lib/TargetParser/Triple.cpp
+++ b/llvm/lib/TargetParser/Triple.cpp
@@ -11,6 +11,7 @@
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/raw_ostream.h"
 #include "llvm/Support/SwapByteOrder.h"
 #include "llvm/Support/VersionTuple.h"
 #include "llvm/TargetParser/ARMTargetParser.h"
@@ -1198,9 +1199,20 @@ StringRef Triple::getOSAndEnvironmentName() const {
   return Tmp.split('-').second;                      // Strip second component
 }
 
+StringRef Triple::getVersionName() const {
+  StringRef VersionName = getEnvironmentName();
+  StringRef EnvironmentTypeName = getEnvironmentTypeName(getEnvironment());
+  if (VersionName.startswith(EnvironmentTypeName))
+    return VersionName.substr(EnvironmentTypeName.size());
+  return VersionName;
+}
+
 static VersionTuple parseVersionFromName(StringRef Name) {
   VersionTuple Version;
-  Version.tryParse(Name);
+  if (Version.tryParse(Name)) {
+    errs() << "version "<< Name << " is invalid\n";
+    exit(1);
+  }
   return Version.withoutBuild();
 }
 

[tru]

This needs to be cleaned up before it can be merged. There needs to be only one commit in this PR without the merge commits from main.

[tru]

It also needs tests.

[zmodem]

Thanks for looking at this. Silently ignoring bad inputs is usually not a good idea.

However, it would seem better to emit a proper diagnostic from the driver rather than calling exit in getOSDefines(). That way the regular diagnostic mechanisms can be used to decide what to do with it (treat as error, ignore, etc.).

[MaskRay]

In addition, calling exit from library code is probably not good. (Think of a service that constructs a triple for each request. It doesn't want to just quit before of an invalid input!) The error can be surfaced by a very top-level component (e.g. driver as @zmodem mentioned).

[pirama-arumuga-nainar]

Please also wait for @MaskRay's LGTM before merging.