Re: [PATCH v4 5/7] MIPS: Fix some ld testcases with compiler

[Alan Modra <amodra@gmail.com>]

Hi YunQiang Su,
I thought I'd take a detailed look at your patch modifying the generic
ELF tests for mips, despite not really being familiar enough with mips
to properly review mips changes.  Most of this review is not about
details of the patch but rather questioning whether you have done the
necessary analysis before xfailing tests.  It is reasonable to xfail a
test if for some reason a problem cannot be fixed, or even if a
problem is too difficult to be fixed.  On the other hand we don't want
a clean testsuite result if there are problems that should be fixed.

On Fri, Jun 16, 2023 at 02:34:10PM +0800, YunQiang Su wrote:
> 1. config/default.exp:
> 	use -mabi=32 not for -gnuabi64;
> 	xfail_from_runlist: remove an element and mark it xfail.

I dislike the use of xfail here.  xfail should only be used after
running a test, to record an expected fail.  Don't use it when a test
is not run.

> 2. ld-elf/indirect.exp: xfail
> 	indirect5a, indirect5b, indirect6a, indirect6b,
> 	indirect5c, indirect5d, indirect6c, indirect6d.

Can you explain why is it correct to xfail these for mips?  The first
four pass for me on mips-linux.  Is there a reason why the last four
must make "bar" dynamic on mips?

> 3. ld-elf/pr23658-2: mips output is not common.

The purpose of this test is to check the placement of orphan loaded
note sections.  These ought to be placed near the start of the first
PT_LOAD segment, so that they can be accessed by the kernel without
reading many disk pages.  Ideally they would be placed in the same
page as the file header.  This isn't happening for mips64, and I see
the standard scripts don't even mention .interp, a section that also
should be placed near the start of the first PT_LOAD.  Why not?!

> 4. ld-elf/shared.exp: non-run on mips: Build libpr16496b.so.

OK, seems like we will actually generate a correct .so here for mips,
so not much is lost by not running the testcase for mips.

> 5. ld-elfvers/vers.exp:
> 	xfail vers4, vers4b;

I see a ld segfault in _bfd_mips_elf_generic_reloc due to
symbol->section->output_section being NULL prior to this patch series.
I'd hope that someone making changes to the testsuite would have seen
this, and not ignored it.  No one expects you to make linking
non-native object files work, but not segfaulting it fairly important.

> 	no-run on mips: vers24a, vers24b, vers24c.

OK, fair enough.  Mips is different enough that it isn't expected to
pass these tests.

> 6. ld-gc/gc.exp: add -KPIC into asflags for pr13683, pr14265, pr19161.

OK.

> 7. ld-mips-elf/mips-elf.exp:
> 	use noarch for mips16-local-stubs-1, since it use -mips4.

I'll leave this for the mips maintainers to OK.

> 8. ld-plugin/lto.exp:
> 	no-run on mips/linux: PR ld/12982;

Please add a comment to lto.exp as to why mips requires an executable
stack.

> 	add -KPIC into asflags for lto-3r, lto-5r, PR ld/19317 (2);

OK.

> 	xfail PR ld/15323 (4), PR ld/19317 (3).

Please explain why the lto plugin won't handle the objects.

> 9. ld-plugin/plugin.exp: xfail
> 	plugin claimfile lost symbol,
> 	plugin claimfile replace symbol,
> 	plugin claimfile replace symbol,
> 	plugin claimfile lost symbol with source,
> 	plugin claimfile replace symbol with source,
> 	plugin claimfile resolve symbol with source,
> 	plugin 2 with source lib,
> 	load plugin 2 with source,
> 	plugin 3 with source lib,
> 	load plugin 3 with source.

These all pass on mips-linux, and on mips64-linux.  Why disable them?
Yes, I see fails on mips64-linux-gnuabi64 but that looks like a gcc
problem to me.

> 11. ld-selective/selective.exp: add -fno-PIC, which is needed for -mno-abicalls.

OK.

> 12. ld-shared/shared.exp: xfail shared (non PIC), shared (PIC main, non PIC so).

OK, and thanks for tweaking the xcoff test message.

-- 
Alan Modra
Australia Development Lab, IBM