r200164 - Fix r195149. Triple should correctly reflect that target. If it contains ios,
Alp Toker
alp at nuanti.com
Tue Jan 28 17:04:12 PST 2014
Thanks guys, I didn't mean to point the finger, just you both had deltas
covering the questionable code :-)
Alp.
On 29/01/2014 01:02, Jim Grosbach wrote:
> 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 <evan.cheng at apple.com
> <mailto:evan.cheng at apple.com>> 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 <alp at nuanti.com
>> <mailto: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
>>
>
--
http://www.nuanti.com
the browser experts
More information about the cfe-commits
mailing list