Agreed. These are real issues, but pre-date any of the recent change here. Lord only knows how long they’ve been there.
Tim, if you’re interested, this would indeed be a nice cleanup. -Jim On Jan 26, 2014, at 5:10 PM, Evan Cheng <[email protected]> wrote: > Hi Alp, > > I agree there are problem with the version checks. These issues are there > before mine and Jim's patch, as far as I can tell. It should be handled in a > follow up patch. Jim, is Tim still working on issues related to embedded > targets / triples? It would be nice if he or someone else could survey the > code closely and clean it up. > > Evan > > On Jan 26, 2014, at 4:28 PM, Alp Toker <[email protected]> wrote: > >> Hi Evan, >> >> Some problems here and in Jim's r195149, review inline... >> >> >> On 26/01/2014 23:12, Evan Cheng wrote: >>> Author: evancheng >>> Date: Sun Jan 26 17:12:43 2014 >>> New Revision: 200164 >>> >>> URL: http://llvm.org/viewvc/llvm-project?rev=200164&view=rev >>> Log: >>> Fix r195149. Triple should correctly reflect that target. If it contains >>> ios, >>> e.g. thumbv7m-apple-ios3.0.0-eabi, then it should mean it's an iOS target. >>> For >>> embedded targets, the OS should be unknown, e.g. >>> thumbv7m-apple-unknown-macho. >>> Since Tim has recently fixed the triple, r195149 is no longer needed. >>> rdar://15911035 >>> >>> Modified: >>> cfe/trunk/lib/Basic/Targets.cpp >>> cfe/trunk/test/Frontend/darwin-eabi.c >>> >>> Modified: cfe/trunk/lib/Basic/Targets.cpp >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/Targets.cpp?rev=200164&r1=200163&r2=200164&view=diff >>> ============================================================================== >>> --- cfe/trunk/lib/Basic/Targets.cpp (original) >>> +++ cfe/trunk/lib/Basic/Targets.cpp Sun Jan 26 17:12:43 2014 >>> @@ -137,37 +137,31 @@ static void getDarwinDefines(MacroBuilde >>> return; >>> } >>> >>> - // If there's an environment specified in the triple, that means we're >>> dealing >>> - // with an embedded variant of some sort and don't want the platform >>> - // version-min defines, so only add them if there's not one. >>> - if (Triple.getEnvironmentName().empty()) { >>> - // Set the appropriate OS version define. >>> - if (Triple.isiOS()) { >>> - assert(Maj < 10 && Min < 100 && Rev < 100 && "Invalid version!"); >>> - char Str[6]; >>> - Str[0] = '0' + Maj; >>> - Str[1] = '0' + (Min / 10); >>> - Str[2] = '0' + (Min % 10); >>> - Str[3] = '0' + (Rev / 10); >>> - Str[4] = '0' + (Rev % 10); >>> - Str[5] = '\0'; >>> - Builder.defineMacro("__ENVIRONMENT_IPHONE_OS_VERSION_MIN_REQUIRED__", >>> - Str); >>> - } else if (Triple.isMacOSX()) { >>> - // Note that the Driver allows versions which aren't representable >>> in the >>> - // define (because we only get a single digit for the minor and micro >>> - // revision numbers). So, we limit them to the maximum representable >>> - // version. >>> - assert(Triple.getEnvironmentName().empty() && "Invalid >>> environment!"); >>> - assert(Maj < 100 && Min < 100 && Rev < 100 && "Invalid version!"); >>> - char Str[5]; >>> - Str[0] = '0' + (Maj / 10); >>> - Str[1] = '0' + (Maj % 10); >>> - Str[2] = '0' + std::min(Min, 9U); >>> - Str[3] = '0' + std::min(Rev, 9U); >>> - Str[4] = '\0'; >>> - Builder.defineMacro("__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__", >>> Str); >>> - } >>> + // Set the appropriate OS version define. >>> + if (Triple.isiOS()) { >>> + assert(Maj < 10 && Min < 100 && Rev < 100 && "Invalid version!"); >> >> >> This iOS assert doesn't look right. The versions are still user-specified >> and not yet validated at this point: >> >> $ clang -cc1 -triple armv7-apple-ios10 >> >> Assertion failed: (Maj < 10 && Min < 100 && Rev < 100 && "Invalid >> version!"), function getDarwinDefines, file clang/lib/Basic/Targets.cpp, >> line 146. >> 0 clang 0x000000010b70674b >> llvm::sys::PrintStackTrace(__sFILE*) + 40 >> 1 clang 0x000000010b706b35 SignalHandler(int) + 245 >> 2 libsystem_platform.dylib 0x00007fff9580c5aa _sigtramp + 26 >> 3 libsystem_platform.dylib 000000000000000000 _sigtramp + 1786722928 >> 4 clang 0x000000010b7069a9 abort + 22 >> 5 clang 0x000000010b706993 abort + 0 >> 6 clang 0x000000010b72c049 >> getDarwinDefines(clang::MacroBuilder&, clang::LangOptions const&, >> llvm::Triple const&, llvm::StringRef&, clang::VersionTuple&) + 1563 >> 7 clang 0x000000010b7d81f6 >> InitializePredefinedMacros(clang::TargetInfo const&, clang::LangOptions >> const&, clang::FrontendOptions const&, clang::MacroBuilder&) + 10802 >> >> >>> + char Str[6]; >>> + Str[0] = '0' + Maj; >>> + Str[1] = '0' + (Min / 10); >>> + Str[2] = '0' + (Min % 10); >>> + Str[3] = '0' + (Rev / 10); >>> + Str[4] = '0' + (Rev % 10); >>> + Str[5] = '\0'; >>> + Builder.defineMacro("__ENVIRONMENT_IPHONE_OS_VERSION_MIN_REQUIRED__", >>> + Str); >>> + } else if (Triple.isMacOSX()) { >>> + // Note that the Driver allows versions which aren't representable in >>> the >>> + // define (because we only get a single digit for the minor and micro >>> + // revision numbers). So, we limit them to the maximum representable >>> + // version. >>> + assert(Maj < 100 && Min < 100 && Rev < 100 && "Invalid version!"); >> >> >> Different check for OS X, similar problem: >> >> $ clang -cc1 -triple i686-apple-darwin130 >> >> Assertion failed: (Maj < 100 && Min < 100 && Rev < 100 && "Invalid >> version!"), function getDarwinDefines, file clang/lib/Basic/Targets.cpp, >> line 162. >> 0 clang 0x000000011069874b >> llvm::sys::PrintStackTrace(__sFILE*) + 40 >> 1 clang 0x0000000110698b35 SignalHandler(int) + 245 >> 2 libsystem_platform.dylib 0x00007fff9580c5aa _sigtramp + 26 >> 3 libsystem_platform.dylib 000000000000000000 _sigtramp + 1786722928 >> 4 clang 0x00000001106989a9 abort + 22 >> 5 clang 0x0000000110698993 abort + 0 >> 6 clang 0x00000001106be02a >> getDarwinDefines(clang::MacroBuilder&, clang::LangOptions const&, >> llvm::Triple const&, llvm::StringRef&, clang::VersionTuple&) + 1532 >> 7 clang 0x000000011076a1f6 >> InitializePredefinedMacros(clang::TargetInfo const&, clang::LangOptions >> const&, clang::FrontendOptions const&, clang::MacroBuilder&) + 10802 >> >> >> For the OS X version you may want to check the return value of >> getMacOSXVersion() which "returns true if successful" but is currently >> ignored. >> >> >>> + char Str[5]; >>> + Str[0] = '0' + (Maj / 10); >>> + Str[1] = '0' + (Maj % 10); >>> + Str[2] = '0' + std::min(Min, 9U); >>> + Str[3] = '0' + std::min(Rev, 9U); >>> + Str[4] = '\0'; >> >> >> The OS X/iOS checks are in various places and it's becoming non-obvious what >> values are acceptable and whether they're validated. >> >> Perhaps you could pull this together into a safe encodeVersionForMacro() >> function? It's not a hot path so bonus points if it can be reused for other >> version encoding macros of differing size. >> >> Alp. >> >> >> >>> + Builder.defineMacro("__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__", >>> Str); >>> } >>> >>> // Tell users about the kernel if there is one. >>> >>> Modified: cfe/trunk/test/Frontend/darwin-eabi.c >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Frontend/darwin-eabi.c?rev=200164&r1=200163&r2=200164&view=diff >>> ============================================================================== >>> --- cfe/trunk/test/Frontend/darwin-eabi.c (original) >>> +++ cfe/trunk/test/Frontend/darwin-eabi.c Sun Jan 26 17:12:43 2014 >>> @@ -1,7 +1,7 @@ >>> // RUN: %clang -arch armv6m -dM -E %s | FileCheck %s >>> // RUN: %clang -arch armv7m -dM -E %s | FileCheck %s >>> // RUN: %clang -arch armv7em -dM -E %s | FileCheck %s >>> -// RUN: %clang -arch armv7 -target thumbv7-apple-darwin-eabi -dM -E %s | >>> FileCheck %s >>> +// RUN: %clang_cc1 -triple thumbv7m-apple-unknown-macho -dM -E %s | >>> FileCheck %s >>> >>> // CHECK-NOT: __ENVIRONMENT_IPHONE_OS_VERSION_MIN_REQUIRED__ >>> // CHECK-NOT: __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ >>> >>> >>> _______________________________________________ >>> cfe-commits mailing list >>> [email protected] >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> >> -- >> http://www.nuanti.com >> the browser experts >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
