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
