r200164 - Fix r195149. Triple should correctly reflect that target. If it contains ios,
Evan Cheng
evan.cheng at apple.com
Sun Jan 26 17:10:42 PST 2014
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 <alp at nuanti.com> 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
>> 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/20140126/d7b64b2d/attachment.html>
More information about the cfe-commits
mailing list