<div dir="ltr">Hi Tom,<br><br>Now I can reproduce the issue, and sorry for any inconvenience caused!<br><br>I just realized, for branch 3.4, integrated assemble was turned on for both AArch64 and X86/X86_64. I originally thought it was not, so this is why I thought we can simply remove method Generic_ELF::IsIntegratedAssemblerDefault(). I was wrong, and we shouldn't remove it.<br>
<br>With the commit 198940 applied to branch 3.4.1, the behavior of bare metal was changed, because method Target.isOSBinFormatELF() would reply true for bare metal and it checks OSes only rather than checking if format is ELF, and the bare metal would fall back to toolchains::Generic_ELF. Without this commit, originally bare metal should go to class toolchains::Generic_GCC.<br>
<br>Generic_GCC is the parent class of Generic_ELF, and the only difference between them is Generic_ELF enabled integrated assembler for AArch64 and X86/X86_64. With the original commit 198940, we would enable integrated assembler for bare metal, and two regression tests would fail on checking -no-integrated-as for x86 with unknown OS (bare metal).<br>
<br>The patch for commit 198940 intends to cover both elf format and bare metal, because there is a newly added RUN for target aarch64-none-none-eabi (bare metal). The relationship of Linux, Generic_ELF and Generic_GCC are,<div>
<br></div><div>Linux -> Generic_ELF -> Generic_GCC (-> points to parent class)<div><br></div><div>The patch 198940 originally tries to promote Linux::addClangTargetOptions to Generic_ELF::addClangTargetOptions. Considering current situation, the reasonable solution would be promoting the method Linux::addClangTargetOptions to even higher class, i.e. Generic_GCC::addClangTargetOptions. This way we could meet all requirements,</div>
<div><div><br>1) Don't change integrate assembler behavior for bare metal<br>2) Enable the fix around -fuse-init-array for both ELF and bare metal<br><br>With this new promotion, the change for Driver::getToolChain in Driver.cpp would be unnecessary.<br>
<br>At the moment of committing 198940 on trunk, we didn't really have this issue, because Generic_ELF implemented nothing at that moment, and integrated assembler wasn't really turned on yet.<br><br>For top of trunk right now, I think we needn't to promote Generic_ELF::addClangTargetOptions to Generic_GCC::addClangTargetOptions, because</div>
<div><br><div>1) The implementation of isOSBinFormatELF is changed by checking ELF format directly rather than OS.</div><div>2) Integrated assembler switch has been promoted to Generic_GCC::IsIntegratedAssemblerDefault.<br>
<br>Refer to attached patch, please! I tested for regression test and the LNT test MultiSource/Applications/sgefa, and I don't see any failures. Hopefully we don't have regressions any longer.<br><br>Thanks,<br>-Jiangning<br>
</div></div></div></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">2014-04-22 21:46 GMT+08:00 Tom Stellard <span dir="ltr"><<a href="mailto:tom@stellard.net" target="_blank">tom@stellard.net</a>></span>:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">On Tue, Apr 22, 2014 at 04:02:24PM +0800, Jiangning Liu wrote:<br>
> Hi Tom,<br>
><br>
> Looking at your code changes, I don't see any issue, and 198940 should only<br>
> affect the behavior of c++ program, while MultiSource/Applications/sgefa is<br>
> a pure c program.<br>
><br>
> I tried top of release 3.4.1 with MultiSource/Applications/sgefa, and I<br>
> can't reproduce the issue. My dump shows with/without the patch you<br>
> reverted, the test can always pass for AArch64 target.<br>
><br>
> So can you tell me on which target it failed? And can you show me the<br>
> failure log as well?<br>
<br>
</div>Attached is the failure log.  The failure has been reported on x86_64<br>
by several different testers.<br>
<span class="HOEnZb"><font color="#888888"><br>
-Tom<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
><br>
> Thanks,<br>
> -Jiangning<br>
><br>
><br>
> 2014-04-22 4:44 GMT+08:00 Tom Stellard <<a href="mailto:tom@stellard.net">tom@stellard.net</a>>:<br>
><br>
> > On Thu, Apr 17, 2014 at 09:57:32AM -0400, Tom Stellard wrote:<br>
> > > On Fri, Apr 11, 2014 at 03:02:21PM -0700, Tom Stellard wrote:<br>
> > > > Hi,<br>
> > > ><br>
> > > > I have just tagged the first release candidate for the<br>
> > > > 3.4.1 release, so testers may begin testing.  Please refer to<br>
> > > > <a href="http://llvm.org/docs/ReleaseProcess.html" target="_blank">http://llvm.org/docs/ReleaseProcess.html</a> for information on how to<br>
> > > > validate a release.  If you have any questions or need<br>
> > > > something clarified, just email the list.<br>
> > > ><br>
> > > > For the 3.4.1 release we want to compare test results against<br>
> > 3.4-final.<br>
> > > ><br>
> > > > I have added support to the test-release.sh script for dot releases and<br>
> > > > have done basic testing.  However, if you run into issues please send<br>
> > > > an email to the list and we can try to get it worked out.<br>
> > > ><br>
> > > > When you report your test results to me please remind me of what<br>
> > platform<br>
> > > > you are testing on.<br>
> > > ><br>
> > > > Thanks again to all the testers for your help.<br>
> > > ><br>
> > ><br>
> > > Test results are starting to come in, it looks like we have a few<br>
> > > regressions:<br>
> > ><br>
> > > x86_64:<br>
> > > MultiSource/Applications/sgefa<br>
> > ><br>
> > > ARMV7:<br>
> > > - new failure: ExecutionEngine/MCJIT/remote/simpletest-remote.ll<br>
> > > - new unexpected pass:<br>
> > >   ExecutionEngine/MCJIT/remote/cross-module-sm-pic-a.ll<br>
> > ><br>
> > ><br>
> > > Has anyone attempted to bisect to figure out which commit broke these<br>
> > > tests?<br>
> > ><br>
> ><br>
> > Hi Jiangning,<br>
> ><br>
> > This commit that you requested be merged into the 3.4 branch has caused a<br>
> > regression in<br>
> > the MultiSource/Applications/sgefa test:<br>
> ><br>
> > -----------------------------------------------------------------------<br>
> ><br>
> >  Merging r198940:<br>
> ><br>
> ><br>
> > ------------------------------------------------------------------------<br>
> >     r198940 | kristof.beyls | 2014-01-10 08:44:34 -0500 (Fri, 10 Jan 2014)<br>
> > | 2 lines<br>
> ><br>
> >     Enable -fuse-init-array for all AArch64 ELF targets by default, not<br>
> > just linux.<br>
> ><br>
> ><br>
> > ------------------------------------------------------------------------<br>
> > -----------------------------------------------------------------------<br>
> ><br>
> > I have reverted this in the 3.4 branch, I'm guessing I merged this wrong.<br>
> > Here was<br>
> > my original attempt:<br>
> > <a href="https://github.com/llvm-mirror/clang/commit/dce55aa1d529e1a07f41241c6a54b9570e0cec64" target="_blank">https://github.com/llvm-mirror/clang/commit/dce55aa1d529e1a07f41241c6a54b9570e0cec64</a><br>
> > Can you merge  r198940 into 3.4 and send me the correct patch.<br>
> ><br>
> > Thanks,<br>
> > Tom<br>
> ><br>
</div></div></blockquote></div><br></div>