<p dir="ltr">I'm afk at the moment please revert and add a test case if you can?</p>
<br><div class="gmail_quote"><div dir="ltr">On Fri, Jan 15, 2016, 5:24 PM Chris Bieneman <<a href="mailto:beanz@apple.com">beanz@apple.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">+Eric,<div><br></div><div>If this patch can’t be quickly fixed, can it please be reverted?</div></div><div style="word-wrap:break-word"><div><div><br></div><div>-Chris</div></div></div><div style="word-wrap:break-word"><div><div><br><div><blockquote type="cite"><div>On Jan 15, 2016, at 4:36 PM, Chris Bieneman via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:</div><br><div><div style="word-wrap:break-word">This change breaks darwin fat binary support.<div><br></div><div><div style="margin:0px;line-height:normal;font-family:'Andale Mono';color:rgb(56,193,255);background-color:rgb(0,0,0)"><div style="margin:0px;line-height:normal"><span style="color:#29f914">> </span><span style="color:#457bf9">./bin/clang++</span><span style="color:#29f914"> </span><span style="color:#34bbc7">~</span>/dev/scratch/hello_world.cpp<span style="color:#29f914"> </span>-arch<span style="color:#29f914"> </span>armv7<span style="color:#29f914"> </span>-arch<span style="color:#29f914"> </span>armv7s<span style="color:#29f914"> </span>-isysroot<span style="color:#29f914"> </span>/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS.sdk/</div><div style="margin:0px;line-height:normal;color:rgb(41,249,20)">fatal error: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/lipo: /var/folders/pb/95pc7qfx2xl9kr9kg0gg_9cc0000gn/T/hello_world-1b17ac.out and /var/folders/pb/95pc7qfx2xl9kr9kg0gg_9cc0000gn/T/hello_world-1b17ac.out have the same architectures (armv7) and can't be in the same fat output file</div><div style="margin:0px;line-height:normal;color:rgb(41,249,20)">clang-3.9: error: lipo command failed with exit code 1 (use -v to see invocation)</div></div><div><br></div><div>Can someone more familiar with this code please take a look?</div><div><br></div><div>Thanks,</div><div><br></div><div>-Chris</div><div><br><div><blockquote type="cite"><div>On Jan 14, 2016, at 1:41 PM, Justin Lebar via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:</div><br><div><div>Author: jlebar<br>Date: Thu Jan 14 15:41:21 2016<br>New Revision: 257808<br><br>URL: <a href="http://llvm.org/viewvc/llvm-project?rev=257808&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=257808&view=rev</a><br>Log:<br>Don't build jobs for the same Action + ToolChain twice.<br><br>Summary:<br>Right now if the Action graph is a DAG and we encounter an action twice,<br>we will run it twice.<br><br>This patch is difficult to test as-is, but I have testcases for this as<br>used within CUDA compilation.<br><br>Reviewers:<br><br>Subscribers:<br><br>Modified:<br> cfe/trunk/include/clang/Driver/Driver.h<br> cfe/trunk/lib/Driver/Driver.cpp<br><br>Modified: cfe/trunk/include/clang/Driver/Driver.h<br>URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Driver.h?rev=257808&r1=257807&r2=257808&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Driver.h?rev=257808&r1=257807&r2=257808&view=diff</a><br>==============================================================================<br>--- cfe/trunk/include/clang/Driver/Driver.h (original)<br>+++ cfe/trunk/include/clang/Driver/Driver.h Thu Jan 14 15:41:21 2016<br>@@ -21,6 +21,7 @@<br> #include "llvm/Support/Path.h" // FIXME: Kill when CompilationInfo lands.<br><br> #include <list><br>+#include <map><br> #include <memory><br> #include <set><br> #include <string><br>@@ -381,10 +382,13 @@ public:<br><br> /// BuildJobsForAction - Construct the jobs to perform for the<br> /// action \p A and return an InputInfo for the result of running \p A.<br>+ /// Will only construct jobs for a given (Action, ToolChain) pair once.<br> InputInfo BuildJobsForAction(Compilation &C, const Action *A,<br> const ToolChain *TC, const char *BoundArch,<br> bool AtTopLevel, bool MultipleArchs,<br>- const char *LinkingOutput) const;<br>+ const char *LinkingOutput,<br>+ std::map<std::pair<const Action *, std::string>,<br>+ InputInfo> &CachedResults) const;<br><br> /// Returns the default name for linked images (e.g., "a.out").<br> const char *getDefaultImageName() const;<br>@@ -441,6 +445,16 @@ private:<br> /// the driver mode.<br> std::pair<unsigned, unsigned> getIncludeExcludeOptionFlagMasks() const;<br><br>+ /// Helper used in BuildJobsForAction. Doesn't use the cache when building<br>+ /// jobs specifically for the given action, but will use the cache when<br>+ /// building jobs for the Action's inputs.<br>+ InputInfo BuildJobsForActionNoCache(<br>+ Compilation &C, const Action *A, const ToolChain *TC,<br>+ const char *BoundArch, bool AtTopLevel, bool MultipleArchs,<br>+ const char *LinkingOutput,<br>+ std::map<std::pair<const Action *, std::string>, InputInfo><br>+ &CachedResults) const;<br>+<br> public:<br> /// GetReleaseVersion - Parse (([0-9]+)(.([0-9]+)(.([0-9]+)?))?)? and<br> /// return the grouped values as integers. Numbers which are not<br><br>Modified: cfe/trunk/lib/Driver/Driver.cpp<br>URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Driver.cpp?rev=257808&r1=257807&r2=257808&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Driver.cpp?rev=257808&r1=257807&r2=257808&view=diff</a><br>==============================================================================<br>--- cfe/trunk/lib/Driver/Driver.cpp (original)<br>+++ cfe/trunk/lib/Driver/Driver.cpp Thu Jan 14 15:41:21 2016<br>@@ -1632,6 +1632,8 @@ void Driver::BuildJobs(Compilation &C) c<br> if (A->getOption().matches(options::OPT_arch))<br> ArchNames.insert(A->getValue());<br><br>+ // Set of (Action, canonical ToolChain triple) pairs we've built jobs for.<br>+ std::map<std::pair<const Action *, std::string>, InputInfo> CachedResults;<br> for (Action *A : C.getActions()) {<br> // If we are linking an image for multiple archs then the linker wants<br> // -arch_multiple and -final_output <final image name>. Unfortunately, this<br>@@ -1651,7 +1653,7 @@ void Driver::BuildJobs(Compilation &C) c<br> /*BoundArch*/ nullptr,<br> /*AtTopLevel*/ true,<br> /*MultipleArchs*/ ArchNames.size() > 1,<br>- /*LinkingOutput*/ LinkingOutput);<br>+ /*LinkingOutput*/ LinkingOutput, CachedResults);<br> }<br><br> // If the user passed -Qunused-arguments or there were errors, don't warn<br>@@ -1779,19 +1781,38 @@ static const Tool *selectToolForJob(Comp<br> return ToolForJob;<br> }<br><br>-InputInfo Driver::BuildJobsForAction(Compilation &C, const Action *A,<br>- const ToolChain *TC, const char *BoundArch,<br>- bool AtTopLevel, bool MultipleArchs,<br>- const char *LinkingOutput) const {<br>+InputInfo Driver::BuildJobsForAction(<br>+ Compilation &C, const Action *A, const ToolChain *TC, const char *BoundArch,<br>+ bool AtTopLevel, bool MultipleArchs, const char *LinkingOutput,<br>+ std::map<std::pair<const Action *, std::string>, InputInfo> &CachedResults)<br>+ const {<br>+ std::pair<const Action *, std::string> ActionTC = {<br>+ A, TC->getTriple().normalize()};<br>+ auto CachedResult = CachedResults.find(ActionTC);<br>+ if (CachedResult != CachedResults.end()) {<br>+ return CachedResult->second;<br>+ }<br>+ InputInfo Result =<br>+ BuildJobsForActionNoCache(C, A, TC, BoundArch, AtTopLevel, MultipleArchs,<br>+ LinkingOutput, CachedResults);<br>+ CachedResults[ActionTC] = Result;<br>+ return Result;<br>+}<br>+<br>+InputInfo Driver::BuildJobsForActionNoCache(<br>+ Compilation &C, const Action *A, const ToolChain *TC, const char *BoundArch,<br>+ bool AtTopLevel, bool MultipleArchs, const char *LinkingOutput,<br>+ std::map<std::pair<const Action *, std::string>, InputInfo> &CachedResults)<br>+ const {<br> llvm::PrettyStackTraceString CrashInfo("Building compilation jobs");<br><br> InputInfoList CudaDeviceInputInfos;<br> if (const CudaHostAction *CHA = dyn_cast<CudaHostAction>(A)) {<br> // Append outputs of device jobs to the input list.<br> for (const Action *DA : CHA->getDeviceActions()) {<br>- CudaDeviceInputInfos.push_back(<br>- BuildJobsForAction(C, DA, TC, nullptr, AtTopLevel,<br>- /*MultipleArchs*/ false, LinkingOutput));<br>+ CudaDeviceInputInfos.push_back(BuildJobsForAction(<br>+ C, DA, TC, nullptr, AtTopLevel,<br>+ /*MultipleArchs*/ false, LinkingOutput, CachedResults));<br> }<br> // Override current action with a real host compile action and continue<br> // processing it.<br>@@ -1822,7 +1843,7 @@ InputInfo Driver::BuildJobsForAction(Com<br> TC = &C.getDefaultToolChain();<br><br> return BuildJobsForAction(C, *BAA->begin(), TC, ArchName, AtTopLevel,<br>- MultipleArchs, LinkingOutput);<br>+ MultipleArchs, LinkingOutput, CachedResults);<br> }<br><br> if (const CudaDeviceAction *CDA = dyn_cast<CudaDeviceAction>(A)) {<br>@@ -1831,7 +1852,8 @@ InputInfo Driver::BuildJobsForAction(Com<br> assert(CDA->getGpuArchName() && "No GPU name in device action.");<br> return BuildJobsForAction(C, *CDA->begin(), C.getCudaDeviceToolChain(),<br> CDA->getGpuArchName(), CDA->isAtTopLevel(),<br>- /*MultipleArchs*/ true, LinkingOutput);<br>+ /*MultipleArchs*/ true, LinkingOutput,<br>+ CachedResults);<br> }<br><br> const ActionList *Inputs = &A->getInputs();<br>@@ -1847,9 +1869,9 @@ InputInfo Driver::BuildJobsForAction(Com<br> // need to build jobs for device-side inputs it may have held.<br> if (CollapsedCHA) {<br> for (const Action *DA : CollapsedCHA->getDeviceActions()) {<br>- CudaDeviceInputInfos.push_back(<br>- BuildJobsForAction(C, DA, TC, "", AtTopLevel,<br>- /*MultipleArchs*/ false, LinkingOutput));<br>+ CudaDeviceInputInfos.push_back(BuildJobsForAction(<br>+ C, DA, TC, "", AtTopLevel,<br>+ /*MultipleArchs*/ false, LinkingOutput, CachedResults));<br> }<br> }<br><br>@@ -1863,7 +1885,7 @@ InputInfo Driver::BuildJobsForAction(Com<br> AtTopLevel && (isa<DsymutilJobAction>(A) || isa<VerifyJobAction>(A));<br> InputInfos.push_back(BuildJobsForAction(C, Input, TC, BoundArch,<br> SubJobAtTopLevel, MultipleArchs,<br>- LinkingOutput));<br>+ LinkingOutput, CachedResults));<br> }<br><br> // Always use the first input as the base input.<br><br><br>_______________________________________________<br>cfe-commits mailing list<br><a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br><a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br></div></div></blockquote></div><br></div></div></div>_______________________________________________<br>cfe-commits mailing list<br><a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br><a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br></div></blockquote></div><br></div></div></div></blockquote></div>