<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">Agreed. These are real issues, but pre-date any of the recent change here. Lord only knows how long they’ve been there.<div><br></div><div>Tim, if you’re interested, this would indeed be a nice cleanup.</div><div><br></div><div>-Jim</div><div><br><div><div>On Jan 26, 2014, at 5:10 PM, Evan Cheng <<a href="mailto:evan.cheng@apple.com">evan.cheng@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><meta http-equiv="Content-Type" content="text/html charset=iso-8859-1"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">Hi Alp,<div><br></div><div>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.</div><div><br></div><div>Evan</div><div><br><div><div>On Jan 26, 2014, at 4:28 PM, Alp Toker <<a href="mailto:alp@nuanti.com">alp@nuanti.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">
<meta content="text/html; charset=ISO-8859-1" http-equiv="Content-Type">
<div bgcolor="#FFFFFF" text="#000000">
Hi Evan,<br>
<br>
Some problems here and in Jim's r195149, review inline...<br>
<br>
<br>
<div class="moz-cite-prefix">On 26/01/2014 23:12, Evan Cheng wrote:<br>
</div>
<blockquote cite="mid:20140126231243.6124C2A6C03D@llvm.org" type="cite">
<pre wrap="">Author: evancheng
Date: Sun Jan 26 17:12:43 2014
New Revision: 200164
URL: <a class="moz-txt-link-freetext" href="http://llvm.org/viewvc/llvm-project?rev=200164&view=rev">http://llvm.org/viewvc/llvm-project?rev=200164&view=rev</a>
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.
<a href="rdar://15911035">rdar://15911035</a>
Modified:
cfe/trunk/lib/Basic/Targets.cpp
cfe/trunk/test/Frontend/darwin-eabi.c
Modified: cfe/trunk/lib/Basic/Targets.cpp
URL: <a class="moz-txt-link-freetext" href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/Targets.cpp?rev=200164&r1=200163&r2=200164&view=diff">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/Targets.cpp?rev=200164&r1=200163&r2=200164&view=diff</a>
==============================================================================
--- 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!");</pre>
</blockquote>
<br>
<br>
This iOS assert doesn't look right. The versions are still
user-specified and not yet validated at this point:<br>
<br>
<code>$ clang -cc1 -triple armv7-apple-ios10</code><code><br>
</code><code>
</code><code><br>
</code><code>
Assertion failed: (Maj < 10 && Min < 100 &&
Rev < 100 && "Invalid version!"), function
getDarwinDefines, file clang/lib/Basic/Targets.cpp, line 146.</code><code><br>
</code><code>
0 clang 0x000000010b70674b
llvm::sys::PrintStackTrace(__sFILE*) + 40</code><code><br>
</code><code>
1 clang 0x000000010b706b35 SignalHandler(int)
+ 245</code><code><br>
</code><code>
2 libsystem_platform.dylib 0x00007fff9580c5aa _sigtramp + 26</code><code><br>
</code><code>
3 libsystem_platform.dylib 000000000000000000 _sigtramp +
1786722928</code><code><br>
</code><code>
4 clang 0x000000010b7069a9 abort + 22</code><code><br>
</code><code>
5 clang 0x000000010b706993 abort + 0</code><code><br>
</code><code>
6 clang 0x000000010b72c049
getDarwinDefines(clang::MacroBuilder&, clang::LangOptions
const&, llvm::Triple const&, llvm::StringRef&,
clang::VersionTuple&) + 1563</code><code><br>
</code><code>
7 clang 0x000000010b7d81f6
InitializePredefinedMacros(clang::TargetInfo const&,
clang::LangOptions const&, clang::FrontendOptions const&,
clang::MacroBuilder&) + 10802</code><br>
<br>
<br>
<blockquote cite="mid:20140126231243.6124C2A6C03D@llvm.org" type="cite">
<pre wrap="">+ 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!");</pre>
</blockquote>
<br>
<br>
Different check for OS X, similar problem:<br>
<br>
<code>$ clang -cc1 -triple i686-apple-darwin130</code><code><br>
</code><code><br>
</code><code>Assertion failed: (Maj < 100 && Min < 100
&& Rev < 100 && "Invalid version!"), function
getDarwinDefines, file clang/lib/Basic/Targets.cpp, line 162.</code><code><br>
</code><code>0 clang 0x000000011069874b
llvm::sys::PrintStackTrace(__sFILE*) + 40</code><code><br>
</code><code>1 clang 0x0000000110698b35
SignalHandler(int) + 245</code><code><br>
</code><code>2 libsystem_platform.dylib 0x00007fff9580c5aa
_sigtramp + 26</code><code><br>
</code><code>3 libsystem_platform.dylib 000000000000000000
_sigtramp + 1786722928</code><code><br>
</code><code>4 clang 0x00000001106989a9 abort +
22</code><code><br>
</code><code>5 clang 0x0000000110698993 abort +
0</code><code><br>
</code><code>6 clang 0x00000001106be02a
getDarwinDefines(clang::MacroBuilder&, clang::LangOptions
const&, llvm::Triple const&, llvm::StringRef&,
clang::VersionTuple&) + 1532</code><code><br>
</code><code>7 clang 0x000000011076a1f6
InitializePredefinedMacros(clang::TargetInfo const&,
clang::LangOptions const&, clang::FrontendOptions const&,
clang::MacroBuilder&) + 10802</code><br>
<br>
<br>
For the OS X version you may want to check the return value of
getMacOSXVersion() which "returns true if successful" but is
currently ignored.<br>
<br>
<br>
<blockquote cite="mid:20140126231243.6124C2A6C03D@llvm.org" type="cite">
<pre wrap="">+ 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';</pre>
</blockquote>
<br>
<br>
The OS X/iOS checks are in various places and it's becoming
non-obvious what values are acceptable and whether they're
validated.<br>
<br>
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.<br>
<br>
Alp.<br>
<br>
<br>
<br>
<blockquote cite="mid:20140126231243.6124C2A6C03D@llvm.org" type="cite">
<pre wrap="">+ 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: <a class="moz-txt-link-freetext" href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Frontend/darwin-eabi.c?rev=200164&r1=200163&r2=200164&view=diff">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Frontend/darwin-eabi.c?rev=200164&r1=200163&r2=200164&view=diff</a>
==============================================================================
--- 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
<a class="moz-txt-link-abbreviated" href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a>
<a class="moz-txt-link-freetext" href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a>
</pre>
</blockquote>
<br>
<pre class="moz-signature" cols="72">--
<a class="moz-txt-link-freetext" href="http://www.nuanti.com/">http://www.nuanti.com</a>
the browser experts
</pre>
</div>
</blockquote></div><br></div></div></blockquote></div><br></div></body></html>