[LLVMdev] 3.4.1 Regression caused by merging r198940

Tue Apr 22 23:03:49 PDT 2014

Hi Tom,

Now I can reproduce the issue, and sorry for any inconvenience caused!

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.

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.

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).

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,

Linux -> Generic_ELF -> Generic_GCC (-> points to parent class)

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,

1) Don't change integrate assembler behavior for bare metal
2) Enable the fix around -fuse-init-array for both ELF and bare metal

With this new promotion, the change for Driver::getToolChain in Driver.cpp
would be unnecessary.

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.

For top of trunk right now, I think we needn't to
promote Generic_ELF::addClangTargetOptions
to Generic_GCC::addClangTargetOptions, because

1) The implementation of isOSBinFormatELF is changed by checking ELF format
directly rather than OS.
2) Integrated assembler switch has been promoted to
Generic_GCC::IsIntegratedAssemblerDefault.

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.

Thanks,
-Jiangning

2014-04-22 21:46 GMT+08:00 Tom Stellard <tom at stellard.net>:

> On Tue, Apr 22, 2014 at 04:02:24PM +0800, Jiangning Liu wrote:
> > Hi Tom,
> >
> > Looking at your code changes, I don't see any issue, and 198940 should
> only
> > affect the behavior of c++ program, while MultiSource/Applications/sgefa
> is
> > a pure c program.
> >
> > I tried top of release 3.4.1 with MultiSource/Applications/sgefa, and I
> > can't reproduce the issue. My dump shows with/without the patch you
> > reverted, the test can always pass for AArch64 target.
> >
> > So can you tell me on which target it failed? And can you show me the
> > failure log as well?
>
> Attached is the failure log.  The failure has been reported on x86_64
> by several different testers.
>
> -Tom
>
> >
> > Thanks,
> > -Jiangning
> >
> >
> > 2014-04-22 4:44 GMT+08:00 Tom Stellard <tom at stellard.net>:
> >
> > > On Thu, Apr 17, 2014 at 09:57:32AM -0400, Tom Stellard wrote:
> > > > On Fri, Apr 11, 2014 at 03:02:21PM -0700, Tom Stellard wrote:
> > > > > Hi,
> > > > >
> > > > > I have just tagged the first release candidate for the
> > > > > 3.4.1 release, so testers may begin testing.  Please refer to
> > > > > http://llvm.org/docs/ReleaseProcess.html for information on how to
> > > > > validate a release.  If you have any questions or need
> > > > > something clarified, just email the list.
> > > > >
> > > > > For the 3.4.1 release we want to compare test results against
> > > 3.4-final.
> > > > >
> > > > > I have added support to the test-release.sh script for dot
> releases and
> > > > > have done basic testing.  However, if you run into issues please
> send
> > > > > an email to the list and we can try to get it worked out.
> > > > >
> > > > > When you report your test results to me please remind me of what
> > > platform
> > > > > you are testing on.
> > > > >
> > > > > Thanks again to all the testers for your help.
> > > > >
> > > >
> > > > Test results are starting to come in, it looks like we have a few
> > > > regressions:
> > > >
> > > > x86_64:
> > > > MultiSource/Applications/sgefa
> > > >
> > > > ARMV7:
> > > > - new failure: ExecutionEngine/MCJIT/remote/simpletest-remote.ll
> > > > - new unexpected pass:
> > > >   ExecutionEngine/MCJIT/remote/cross-module-sm-pic-a.ll
> > > >
> > > >
> > > > Has anyone attempted to bisect to figure out which commit broke these
> > > > tests?
> > > >
> > >
> > > Hi Jiangning,
> > >
> > > This commit that you requested be merged into the 3.4 branch has
> caused a
> > > regression in
> > > the MultiSource/Applications/sgefa test:
> > >
> > > -----------------------------------------------------------------------
> > >
> > >  Merging r198940:
> > >
> > >
> > >
> ------------------------------------------------------------------------
> > >     r198940 | kristof.beyls | 2014-01-10 08:44:34 -0500 (Fri, 10 Jan
> 2014)
> > > | 2 lines
> > >
> > >     Enable -fuse-init-array for all AArch64 ELF targets by default, not
> > > just linux.
> > >
> > >
> > >
> ------------------------------------------------------------------------
> > > -----------------------------------------------------------------------
> > >
> > > I have reverted this in the 3.4 branch, I'm guessing I merged this
> wrong.
> > > Here was
> > > my original attempt:
> > >
> https://github.com/llvm-mirror/clang/commit/dce55aa1d529e1a07f41241c6a54b9570e0cec64
> > > Can you merge  r198940 into 3.4 and send me the correct patch.
> > >
> > > Thanks,
> > > Tom
> > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140423/cdc08334/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 198940_backport_to_release_34_clang.patch
Type: text/x-patch
Size: 3634 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140423/cdc08334/attachment.bin>