Re: [PATCH 2/2] MIPS: Compressed PLT/stubs support test cases

[Richard Sandiford <rdsandiford at googlemail dot com>]

[removing gdb-patches from cc: since I doubt they care]

"Maciej W. Rozycki" <macro@codesourcery.com> writes:
>  Not much to say here, except that it's a huge pile of stuff. ;)
>
>  I believe that the selection of samples I made is representative for the 
> functionality covered.  It certainly proved itself in that it revealed a 
> number of corner-case problems that I have either addressed with separate 
> patches already posted to cover individual issues or by adjusting the 
> compressed PLT/stubs support change itself.  Which is also why I'm making 
> this submission now rather than a fortnight ago as I originally expected.
>
>  I realise this piece is extensive, which is why I have designed this test 
> case subset with long-term maintainability in mind.  Therefore the 
> mips-elf.exp part is relatively simple (for a reasonable definition of 
> "simple"), in that it's a loop over 16 cases tested for various o32
> configurations.  The selection of sources chosen for these tests is meant 
> to cover various internal paths of code being examined, it is meant to be 
> exhaustive.  There is some redundancy however, for example for lazy 
> binding stubs some of these individual tests add no value.
>
>  I have decided however, that it will be easier to maintain this uniform 
> set of selections rather than having more individual tests prepared for 
> each of the individual configuration covered.  This is also why I decided 
> there's less harm from some sources being assembled multiple times (i.e. 
> some performance hit with testsuite runs) than gain from having the 
> individual test cases self-contained.
>
>  Similarly, there's a smaller loop for NewABI tests that just have 2 cases 
> to verify compressed code does not sneak into PLT there and check lazy 
> stubs at the same time.  The tests may be merged back into the loop used 
> for o32 configurations if support for compressed PLT is ever added for 
> NewABIs, except that currently we have some restrictions on what MIPS16 
> code we can produce for the NewABIs due to missing relocations.  I had to 
> address it already in one of the MIPS16 cases where the corresponding 
> standard MIPS and microMIPS code uses the DLA macro, but there's no 
> equivalent, perhaps using the %higher and %highest operators, in the 
> MIPS16 mode.
>
>  Now the good side of this approach is the dumps are easy to maintain.  
> Assuming a legitimate regression occurs in the future because of a change 
> in functionality, then the testsuite can simply be run to produce all the 
> binaries required and failures ignored.  Then dumps can be easily updated 
> with a script and differences examined for correctness.
>
>  I can post the script I used (although it's a bit rough) -- I don't 
> suppose anybody might have even briefly thought I produced these dumps 
> manually. ;)  I did review the results to match expectations though (and 
> actually adjusted code accordingly in a couple cases till they matched).
>
>  Comments are welcome, as usually, and I hope this is OK once the change 
> proper goes in; this part isn't supposed to change as the test patterns 
> are meant to serve as a conformance check.

Looks good, although automatically-generated stuff is hard to review.

However, rather than quote the full symbol table, e.g.:

+Symbol table '\.dynsym' contains 6 entries:
+   Num:    Value  Size Type    Bind   Vis      Ndx Name
+     0: 00000000     0 NOTYPE  LOCAL  DEFAULT  UND 
+     1: 12340000     0 SECTION LOCAL  DEFAULT    1 
+     2: 12340020     8 FUNC    GLOBAL DEFAULT    1 fun3
+     3: 12340010     8 FUNC    GLOBAL DEFAULT    1 fun2
+     4: 12340030     0 OBJECT  GLOBAL DEFAULT  ABS _GLOBAL_OFFSET_TABLE_
+     5: 12340000     8 FUNC    GLOBAL DEFAULT    1 fun1
+
+Symbol table '\.symtab' contains 15 entries:
+   Num:    Value  Size Type    Bind   Vis      Ndx Name
+     0: 00000000     0 NOTYPE  LOCAL  DEFAULT  UND 
+     1: 12340000     0 SECTION LOCAL  DEFAULT    1 
+     2: 12340030     0 SECTION LOCAL  DEFAULT    2 
+     3: 12340038     0 SECTION LOCAL  DEFAULT    3 
+     4: 12340050     0 SECTION LOCAL  DEFAULT    4 
+     5: 123400e8     0 SECTION LOCAL  DEFAULT    5 
+     6: 12340148     0 SECTION LOCAL  DEFAULT    6 
+     7: 12340170     0 SECTION LOCAL  DEFAULT    7 
+     8: 1234019c     0 SECTION LOCAL  DEFAULT    8 
+     9: 12340050     0 OBJECT  LOCAL  DEFAULT  ABS _DYNAMIC
+    10: 12348020     0 NOTYPE  LOCAL  DEFAULT  ABS _gp
+    11: 12340020     8 FUNC    GLOBAL DEFAULT    1 fun3
+    12: 12340010     8 FUNC    GLOBAL DEFAULT    1 fun2
+    13: 12340030     0 OBJECT  GLOBAL DEFAULT  ABS _GLOBAL_OFFSET_TABLE_
+    14: 12340000     8 FUNC    GLOBAL DEFAULT    1 fun1

please instead match the bits we care about, with the symbol numbers
stubbed out.  E.g.:

Symbol table '\.dynsym' contains .* entries:
#...
.*: 12340020     8 FUNC    GLOBAL DEFAULT    1 fun3
.*: 12340010     8 FUNC    GLOBAL DEFAULT    1 fun2
.* _GLOBAL_OFFSET_TABLE_
.*: 12340000     8 FUNC    GLOBAL DEFAULT    1 fun1

Symbol table '\.symtab' contains .* entries:
#...
.*: 12340020     8 FUNC    GLOBAL DEFAULT    1 fun3
.*: 12340010     8 FUNC    GLOBAL DEFAULT    1 fun2
.* _GLOBAL_OFFSET_TABLE_
.*: 12340000     8 FUNC    GLOBAL DEFAULT    1 fun1

We don't care in this test how many section symbols there are, whether
section 6 (whatever that is) starts at address 0x12340148, whether the
section symbols have names, how many linker-generated local symbols
there are, etc.  Matching them means that incidental differences can
lead to large testsuite changes.

We don't really care whether there's a global _GLOBAL_OFFSET_TABLE_
either (raised recently), but at least stubbing out the symbol numbers
makes it easy to change things like that in future.  The stubbed-out
version also makes it easier to see what we're testing.

I think we should have direct tests of the .rel.plt contents, rather
than relying solely on the disassembly of .plt having the right synthetic
symbols.  We should also test the contents of .got.plt.  To avoid
file explosion, please use the same objdump and readelf dump files
if possible :-)

Also, you have things like ".word fun" in the source (good, thanks),
but don't match the result.  One of the things we want to know with this
test is what we do when we have ".word fun" and two PLT entries for "fun".
We want to make sure that the PLT entry with the standard encoding is
used both in the symbol table and in resolving the R_MIPS_32.

Using -Ttext goes some way to making the tests less sensitive to minor
address differences, but please also bung in a large alignment before
the sections that we match.  Something like an alignment of 0x1000
before .plt, .got.plt, .data and .rel.plt (I've probably missed some).
As well as making the test less sensitive, it also makes the addresses
easier to track.

E.g. in the MIPS16 tests we have things like:

+567800ac:	(?:5678 00c8|00c8 5678) 	\.word	0x567800c8

but the fact that 0x567800c8 is the address of this function's .got.plt
slot isn't obvious from the test source and depends indirectly on lots
of things.  This will probably become more of a problem with the -r
tests mentioned above.

Very minor, but since we want the real linker scripts to put .got.plt
at the beginning of the writable part of the data segment, it might be
worth reordering the test scripts:

+  .plt : { *(.plt) }
+  HIDDEN (_gp = ALIGN (16) + 0x7ff0);
+  .got : { *(.got) }
+  .got.plt : { *(.got.plt) }
+  .data : { *(.data) }

so that _gp and .got come after .got.plt.  This'll also test that
we don't accidentally rely on .got.plt being within range of _gp.

Thanks,
Richard