r200164 - Fix r195149. Triple should correctly reflect that target. If it contains ios,

Alp Toker alp at nuanti.com
Sun Jan 26 16:28:57 PST 2014


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
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

-- 
http://www.nuanti.com
the browser experts

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140127/5b513efe/attachment.html>


More information about the cfe-commits mailing list