Add a new warning to notify users of mismatched SDK and deployment target

[beanz]

Summary

This patch adds a new driver warning -Wincompatible-sdk which notifies the user when they are mismatching the version min options and the sysroot.

The patch works by checking the sysroot (if present) for an SDK name, then matching that against the target platform. In the case of a mismatch it logs a warning.

[bob.wilson]

This is a good idea overall. See comments.

include/clang/Basic/DiagnosticDriverKinds.td

198 ↗

(On Diff #50437)

Instead of the driver option, you should add this to the diagnostic: InGroup<DiagGroup<"incompatible-sdk">>

include/clang/Driver/Options.td

1090 ↗

(On Diff #50437)

You should not need to add this option. See above.

lib/Driver/ToolChains.cpp

333 ↗

(On Diff #50437)

Please add a space before the open paren.

742 ↗

(On Diff #50437)

Please add a space before the open paren.

750 ↗

(On Diff #50437)

Please add a space before the open paren. You should run the patch through clang-format since you have several formatting issues.

[edward-san]

Excuse me for intruding, but from what I see, this warning applies only to Apple SDK. Since SDK could mean everything, what about naming the warning accordingly, like '-Wincompatible-apple-sdk' or '-Wapple-incompatible-sdk' and adjust the description accordingly?

[bob.wilson]

Yes, in its current usage this warning is only used for Apple's SDKs, but how does it help to put "apple" in the name of the diagnostic? Are you concerned about a name conflict with a similar diagnostic for non-Apple SDKs?

[edward-san]

I'm more concerned about some users who might think that option would work also with their favourite sdk (visual studio comes into mind as a common sdk).

[beanz]

Perhaps naming this warning 'incompatible-sdk' isn't the best name. I can see how it would cause some confusion for Visual Studio users. On Darwin the SDK is the collection of headers and libraries you link against, not the toolkit you have installed.

Perhaps calling the warning 'incompatible-sysroot' is a better name. Even though this patch only implements it for Darwin I imagine that similar functionality could be added for Windows and Linux too. The idea being to have the compiler perform some basic check to ensure that the headers and libraries you are building against actually support your target.

Updated patches coming shortly.

[beanz]

Updates based on review feedback.

[beanz]

Ping?

[rsmith]

lib/Driver/ToolChains.cpp

718–720 ↗

(On Diff #51813)

This seems to get a couple of corner cases wrong (/ between the platform name and .sdk, .sdk not at the end of the path component, ...). Have you considered using llvm::sys::path's path iterators here? Not a big deal either way, though.

722 ↗

(On Diff #51813)

According to slice's documentation, if EndSDK < BeginSDK + 5, this slice call will return a slice from BeginSDK + 5 to the end of the string, and you won't be checking for a .sdk prefix as you intended to.

[rsmith]

lib/Driver/ToolChains.cpp

722 ↗

(On Diff #51813)

Huh. That's a bug in slice's documentation (fixed in r267831).

[beanz]

Use llvm::sys::path::const_iterator instead of hand slicing the paths, based on feedback from @rsmith.

Thanks,
-Chris

[rsmith]

lib/Driver/ToolChains.cpp

722–733 ↗

(On Diff #55472)

Can you share some of the code between the parsing the SDK version out of -isysroot here and above?

[beanz]

Cleanup and refactoring based on feedback from @rsmith.

[rsmith]

lib/Driver/ToolChains.cpp

733 ↗

(On Diff #55652)

I assume it's intentional that you allow targeting FooOS with a sysroot for FooSimulator and vice versa?

[beanz]

Thanks!

Will commit soon.

lib/Driver/ToolChains.cpp

733 ↗

(On Diff #55652)

Yes. It turns out that setting the iOS version min flag when using the simulator SDK is *way* too common of a case to warn about, and since it all turns into the same load command anyways it doesn't matter too much.

cfe/trunk/include/clang/Basic/DiagnosticDriverKinds.td

def warn_drv_treating_input_as_cxx : Warning<
   "treating '%0' input as '%1' when in C++ mode, this behavior is deprecated">,
   InGroup<Deprecated>;
 def warn_drv_objc_gc_unsupported : Warning<
   "Objective-C garbage collection is not supported on this platform, ignoring '%0'">;
 def warn_drv_pch_not_first_include : Warning<
   "precompiled header '%0' was ignored because '%1' is not first '-include'">;
 def warn_missing_sysroot : Warning<"no such sysroot directory: '%0'">,
   InGroup<DiagGroup<"missing-sysroot">>;
+def warn_incompatible_sysroot : Warning<"using sysroot for '%0' but targeting '%1'">,
+  InGroup<DiagGroup<"incompatible-sysroot">>;
 def warn_debug_compression_unavailable : Warning<"cannot compress debug sections (zlib not installed)">,
   InGroup<DiagGroup<"debug-compression-unavailable">>;
 def warn_drv_enabling_rtti_with_exceptions : Warning<
   "implicitly enabling rtti for exception handling">,
   InGroup<DiagGroup<"rtti-for-exceptions">>;
 def warn_drv_disabling_vptr_no_rtti_default : Warning<
   "implicitly disabling vptr sanitizer because rtti wasn't enabled">,
   InGroup<DiagGroup<"auto-disable-vptr-sanitizer">>;

cfe/trunk/lib/Driver/ToolChains.h

bool isIPhoneOSVersionLT(unsigned V0, unsigned V1 = 0,
     return TargetVersion < VersionTuple(V0, V1, V2);
   }
 
   bool isMacosxVersionLT(unsigned V0, unsigned V1 = 0, unsigned V2 = 0) const {
     assert(isTargetMacOS() && "Unexpected call for non OS X target!");
     return TargetVersion < VersionTuple(V0, V1, V2);
   }
 
+  StringRef getPlatformFamily() const;
+  static StringRef getSDKName(StringRef isysroot);
   StringRef getOSLibraryNameSuffix() const;
 
 public:
   /// }
   /// @name ToolChain Implementation
   /// {
 
   // Darwin tools support multiple architecture (e.g., i386 and x86_64) and

cfe/trunk/lib/Driver/ToolChains.cpp

if (AddRPath) {
 
     // Add the path to the resource dir to rpath to support using the dylib
     // from the default location without copying.
     CmdArgs.push_back("-rpath");
     CmdArgs.push_back(Args.MakeArgString(Dir));
   }
 }
 
+StringRef Darwin::getPlatformFamily() const {
+  switch (TargetPlatform) {
+    case DarwinPlatformKind::MacOS:
+      return "MacOSX";
+    case DarwinPlatformKind::IPhoneOS:
+    case DarwinPlatformKind::IPhoneOSSimulator:
+      return "iPhone";
+    case DarwinPlatformKind::TvOS:
+    case DarwinPlatformKind::TvOSSimulator:
+      return "AppleTV";
+    case DarwinPlatformKind::WatchOS:
+    case DarwinPlatformKind::WatchOSSimulator:
+      return "Watch";
+  }
+  llvm_unreachable("Unsupported platform");
+}
+
+StringRef Darwin::getSDKName(StringRef isysroot) {
+  // Assume SDK has path: SOME_PATH/SDKs/PlatformXX.YY.sdk
+  llvm::sys::path::const_iterator SDKDir;
+  auto BeginSDK = llvm::sys::path::begin(isysroot);
+  auto EndSDK = llvm::sys::path::end(isysroot);
+  for (auto IT = BeginSDK; IT != EndSDK; ++IT) {
+    StringRef SDK = *IT;
+    if (SDK.endswith(".sdk"))
+      return SDK.slice(0, SDK.size() - 4);
+  }
+  return "";
+}
+
 StringRef Darwin::getOSLibraryNameSuffix() const {
   switch(TargetPlatform) {
   case DarwinPlatformKind::MacOS:
     return "osx";
   case DarwinPlatformKind::IPhoneOS:
     return "ios";
   case DarwinPlatformKind::IPhoneOSSimulator:
     return "iossim";
if (OSXVersion && (iOSVersion || TvOSVersion || WatchOSVersion)) {
 
     // If there is no command-line argument to specify the Target version and
     // no environment variable defined, see if we can set the default based
     // on -isysroot.
     if (OSXTarget.empty() && iOSTarget.empty() && WatchOSTarget.empty() &&
         TvOSTarget.empty() && Args.hasArg(options::OPT_isysroot)) {
       if (const Arg *A = Args.getLastArg(options::OPT_isysroot)) {
         StringRef isysroot = A->getValue();
-        // Assume SDK has path: SOME_PATH/SDKs/PlatformXX.YY.sdk
-        size_t BeginSDK = isysroot.rfind("SDKs/");
-        size_t EndSDK = isysroot.rfind(".sdk");
-        if (BeginSDK != StringRef::npos && EndSDK != StringRef::npos) {
-          StringRef SDK = isysroot.slice(BeginSDK + 5, EndSDK);
+        StringRef SDK = getSDKName(isysroot);
+        if (SDK.size() > 0) {
           // Slice the version number out.
           // Version number is between the first and the last number.
           size_t StartVer = SDK.find_first_of("0123456789");
           size_t EndVer = SDK.find_last_of("0123456789");
           if (StartVer != StringRef::npos && EndVer > StartVer) {
             StringRef Version = SDK.slice(StartVer, EndVer + 1);
             if (SDK.startswith("iPhoneOS") ||
                 SDK.startswith("iPhoneSimulator"))
void Darwin::AddDeploymentTarget(DerivedArgList &Args) const {
   if (TvOSVersion && (getTriple().getArch() == llvm::Triple::x86 ||
                       getTriple().getArch() == llvm::Triple::x86_64))
     Platform = TvOSSimulator;
   if (WatchOSVersion && (getTriple().getArch() == llvm::Triple::x86 ||
                          getTriple().getArch() == llvm::Triple::x86_64))
     Platform = WatchOSSimulator;
 
   setTarget(Platform, Major, Minor, Micro);
+
+  if (const Arg *A = Args.getLastArg(options::OPT_isysroot)) {
+    StringRef SDK = getSDKName(A->getValue());
+    if (SDK.size() > 0) {
+      size_t StartVer = SDK.find_first_of("0123456789");
+      StringRef SDKName = SDK.slice(0, StartVer);
+      if (!SDKName.startswith(getPlatformFamily()))
+        getDriver().Diag(diag::warn_incompatible_sysroot)
+            << SDKName << getPlatformFamily();
+    }
+  }
 }
 
 void DarwinClang::AddCXXStdlibLibArgs(const ArgList &Args,
                                       ArgStringList &CmdArgs) const {
   CXXStdlibType Type = GetCXXStdlibType(Args);
 
   switch (Type) {
   case ToolChain::CST_Libcxx:

cfe/trunk/test/Driver/incompatible_sysroot.c

+// RUN: %clang -Wincompatible-sysroot -isysroot SDKs/MacOSX10.9.sdk -mios-version-min=9.0 -S -o - %s 2>&1 | FileCheck -check-prefix CHECK-OSX-IOS %s
+// RUN: %clang -Wincompatible-sysroot -isysroot SDKs/iPhoneOS9.2.sdk -mwatchos-version-min=2.0 -S -o - %s 2>&1 | FileCheck -check-prefix CHECK-IOS-WATCHOS %s
+// RUN: %clang -Wincompatible-sysroot -isysroot SDKs/iPhoneOS9.2.sdk -mtvos-version-min=9.0 -S -o - %s 2>&1 | FileCheck -check-prefix CHECK-IOS-TVOS %s
+// RUN: %clang -Wincompatible-sysroot -isysroot SDKs/iPhoneSimulator9.2.sdk -mios-version-min=9.0 -S -o - %s 2>&1 | FileCheck -check-prefix CHECK-IOS-IOSSIM %s
+// RUN: %clang -Wno-incompatible-sysroot -isysroot SDKs/MacOSX10.9.sdk -mios-version-min=9.0 -S -o - %s 2>&1 | FileCheck -check-prefix CHECK-OSX-IOS-DISABLED %s
+
+int main() { return 0; }
+// CHECK-OSX-IOS: warning: using sysroot for 'MacOSX' but targeting 'iPhone'
+// CHECK-IOS-WATCHOS: warning: using sysroot for 'iPhoneOS' but targeting 'Watch'
+// CHECK-IOS-TVOS: warning: using sysroot for 'iPhoneOS' but targeting 'AppleTV'
+// CHECK-IOS-IOSSIM-NOT: warning: using sysroot for '{{.*}}' but targeting '{{.*}}'
+// CHECK-OSX-IOS-DISABLED-NOT: warning: using sysroot for '{{.*}}' but targeting '{{.*}}'