Page MenuHomePhabricator

[lld][test] Expand testing for dynamic-list and export-dynamic
ClosedPublic

Authored by gbreynoo on May 20 2020, 10:09 AM.

Details

Summary
  • Expanded testing for export-dynamic and dynamic list
  • Added cases that have no matching symbol
  • Added uses of fatal-warnings to ensure warnings are not output unexpectedly
  • Rename dynamic-list-wildcard.s to dynamic-list-patterns.s and expanded the test
  • Added export-symbol-types.s
  • Fixed invalid-dynamic-list, previously the same list was used in each case. When looking at this test on I found that the 5th case was failing to echo successfully leading to the previous case being tested again.
  • Allow invalid-dynamic-list to run on Windows after removing the use of echo with quoted strings.

Diff Detail

Event Timeline

gbreynoo created this revision.May 20 2020, 10:09 AM
Herald added a project: Restricted Project. · View Herald Transcript

Interesting. Oh, did you know that I have an ongoing discussion with binutils to refine the semantics of --export-dynamic-symbol --dynamic-symbol -Bsymbolic :) https://sourceware.org/pipermail/binutils/2020-May/111019.html

Generally LG. Few comments below.

lld/test/ELF/export-dynamic-symbol.s
8 ↗(On Diff #265273)
##
9 ↗(On Diff #265273)

A double space after --fatal-warnings (and below).

lld/test/ELF/export-symbol-types.s
1 ↗(On Diff #265273)

It probably missing a description of what this test case generally checks.

17 ↗(On Diff #265273)

export-symbol-types.s is short. Could you use echo instead?

21 ↗(On Diff #265273)
##

Also, full stop is missing.

23 ↗(On Diff #265273)

This empty line looks excessive.

gbreynoo updated this revision to Diff 265509.May 21 2020, 8:10 AM

Changes based on grimars comments:

  • Corrected white space and comments
  • Replaced the use of input file "Inputs/export-symbol-types.s" with use of echo
gbreynoo marked 6 inline comments as done.May 21 2020, 8:10 AM

Thanks MaskRay, I was not aware of that.

MaskRay added inline comments.May 21 2020, 11:31 PM
lld/test/ELF/dynamic-list-patterns.s
7 ↗(On Diff #265509)

-unknown-linux can be dropped (this tests generic ELF behavior)

9 ↗(On Diff #265509)

Use ' for outermost quotes

11 ↗(On Diff #265509)

--dyn-syms

You can also use llvm-nm -D

lld/test/ELF/export-symbol-types.s
31 ↗(On Diff #265509)

--dyn-syms

32 ↗(On Diff #265509)

Can you find existing tests which can be deleted after you add the more comprehensive test here?

gbreynoo updated this revision to Diff 265745.May 22 2020, 8:56 AM

Changes based on MaskRay's comments:

  • Replace use of --dyn-symbols with --dynsyms
  • Remove use of -unknown-linux
  • Correct use of quotes
gbreynoo marked 5 inline comments as done.May 22 2020, 9:04 AM
gbreynoo added inline comments.
lld/test/ELF/dynamic-list-patterns.s
11 ↗(On Diff #265509)

The other tests use llvm-readelf so thought it best to be consistent, also I think confirming the number of entries is easier with readelf. If llvm-nm is preferred I could update the other tests too?

lld/test/ELF/export-symbol-types.s
32 ↗(On Diff #265509)

I had a look at the other tests for --dynamic-list and --export-dynamic and believe they each add coverage e.g. they test for a particular error message or are case when multiple symbols of the same type are exported together etc. If there is a test you have in mind I could add it to the review.

Thanks MaskRay, I was not aware of that.

--dynamic-list is an obscure option. I just wondered how you get interested in it:)

There are fuzzy things about its semantics and relationship with -Bsymbolic, --export-dynamic, etc. That was the motivation behind my GNU ld patches.
I hope they can accept reasonable order-independent semantics.
https://sourceware.org/pipermail/binutils/2020-May/111211.html (I am happy with our our --dynamic-list semantics)
https://sourceware.org/pipermail/binutils/2020-May/111019.html (we may need to change --export-dynamic-symbol a bit)

MaskRay added inline comments.May 22 2020, 12:25 PM
lld/test/ELF/export-symbol-types.s
3 ↗(On Diff #265745)

For -shared, --dynamic-list does not affect the exported symbols.

MaskRay added inline comments.May 22 2020, 1:31 PM
lld/test/ELF/export-symbol-types.s
4 ↗(On Diff #265745)

I think the test should be named dynamic-list-* instead, instead of export-symbol

--dynamic-list work for both executables and shared libraries.
For an executable, export some symbols.
For a shared library, specify preemptible symbols.

We should check both executable and so cases.

Created D80487 for --export-dynamic-symbol

lld/test/ELF/Inputs/invalid-dynamic-list-1.list
1 ↗(On Diff #265745)

echo '.........' | llvm-mc -filetype=obj -triple=x86_64 - -o %t1.o for simple tests

lld/test/ELF/export-symbol-types.s
1 ↗(On Diff #265745)

REQUIRES: x86

The test duplicates partially with dynsym-pie.s, symbols.s and visibility.s

Check if they can be reorganized.

11 ↗(On Diff #265745)

imported symbols -> shared symbols

18 ↗(On Diff #265745)

echo '{ *; };' > %t.list

20 ↗(On Diff #265745)

write on one line and separate statements with ;

24 ↗(On Diff #265745)

Use the same basename for .o and .so if applicable

36 ↗(On Diff #265745)

Just use CHECK-NEXT:

The diagnostic will be better if things go off. This does rely on the symbol table order and will require extra efforts when the order does change, but I think the cost pays off.

38 ↗(On Diff #265745)

Use ABS for abs

43 ↗(On Diff #265745)

technically this is not referenced. You may name it undef_weak (yes, the order undefined weak (instead of weak undefined) is used in the ELF spec)

gbreynoo updated this revision to Diff 267660.Jun 1 2020, 10:42 AM
gbreynoo marked 2 inline comments as done.

Made the following changes:

  • Fixed up a comment
  • Added x86 requirement
  • Changed the creation of the dynamic list in export-symbol-types.s
  • Made file names more consistent
  • Use CHECK-NEXT
  • Use ABS for abs
  • rename symbol
gbreynoo marked 6 inline comments as done.Jun 1 2020, 10:43 AM
gbreynoo updated this revision to Diff 267671.Jun 1 2020, 10:55 AM

Fixed use of echo

gbreynoo marked 2 inline comments as done.Jun 1 2020, 10:55 AM

Thanks for the comments Maskray. I had some questions regarding the comments for expanding the test for use of -shared and the similarities with dynsym-pie.s, symbols.s and visibility.s. My intention with the test export-symbol-types.s was to confirm the symbols types that are placed in the dynamic symbol table with use of --export-dynamic and --dynamic-list. I considered the other tests mentioned as covering related but different functionality using similar methods. For example symbol.s being for correct symbol output as a whole, not specifically exports etc.

I can see there being a reason to expand the coverage of the existing test to cover -shared, this would require the rename and potentially splitting the existing test in two. It may also be worth creating one input assembly file that can be used for all these tests mentioned to ensure coverage. Could this wait for a future change however?

Also, with your comment were you suggesting removing some of the existing tests and folding them into the one added here? As they test different functionality I think it may be best to keep them separate.

MaskRay added inline comments.Jun 1 2020, 1:04 PM
lld/test/ELF/dynamic-list-patterns.s
1 ↗(On Diff #267671)

dynamic-list-glob.s may be better.

The recognized patterns are globs, not regex.

3 ↗(On Diff #267671)

Thanks for deleting dynamic-list-wildcard.s

Whether an entry is a glob depends on whether double quotes are used. It is not unconditional glob, so the sentence is not accurate.

Confirm --dynamic-list identifies symbols by patterns, including wildcards.

10 ↗(On Diff #267671)

s/%t.elf/%t/

lld/test/ELF/export-dynamic-symbol.s
3 ↗(On Diff #267671)

I committed D80487 which came with a bunch of tests. These cases should have been covered.

lld/test/ELF/export-symbol-types.s
9 ↗(On Diff #267671)

undefined symbols and shared symbols are exported.

--export-dynamic, --dynamic-list and --export-dynamic-symbol can export additional symbols: non-local STV_DEFAULT symbols

The list is not accurate because there is overlap between hidden symbols and weak symbols, for example.

weak symbols are not unconditionally exported.

Many symbols generated by the linker are not exported (hidden).

24 ↗(On Diff #267671)

Drop -shared. .so obviously identifies a shared object.

26 ↗(On Diff #267671)

Drop -elf. It carries no additional information. All object files discussed here are ELF.

26 ↗(On Diff #267671)

Delete llvm-mc -filetype=obj -triple=x86_64 %s -o %t-elf.o

the object file is the same

lld/test/ELF/invalid-dynamic-list.test
9–10

You can update the quotes by the way.

Thanks for the comments Maskray. I had some questions regarding the comments for expanding the test for use of -shared and the similarities with dynsym-pie.s, symbols.s and visibility.s. My intention with the test export-symbol-types.s was to confirm the symbols types that are placed in the dynamic symbol table with use of --export-dynamic and --dynamic-list. I considered the other tests mentioned as covering related but different functionality using similar methods. For example symbol.s being for correct symbol output as a whole, not specifically exports etc.

I carefully checked. export-symbol-types.s makes sense, but the name may be confusing. symbol types refers specifically to STT_*, while in the test we are checking other properties (visibility/binding/etc).

I can see there being a reason to expand the coverage of the existing test to cover -shared, this would require the rename and potentially splitting the existing test in two. It may also be worth creating one input assembly file that can be used for all these tests mentioned to ensure coverage. Could this wait for a future change however?

Also, with your comment were you suggesting removing some of the existing tests and folding them into the one added here? As they test different functionality I think it may be best to keep them separate.

-shared tests require to check preemptibility. This can usually tested by checking dynamic relocations. You can test them with movl foo@gotpcrel(%rip), %rax
They can be added in this patch. You can add -shared tests in a future change.

gbreynoo updated this revision to Diff 269549.Jun 9 2020, 8:32 AM
  • Corrected use of pattern with entry
  • Renamed dynamic-list-patterns.s
  • Removed changes to export-dynamic-symbol.s
  • Change input file names
  • Updated quotes
gbreynoo updated this revision to Diff 269557.Jun 9 2020, 8:40 AM
gbreynoo marked 3 inline comments as done.
gbreynoo marked 3 inline comments as done.
gbreynoo updated this revision to Diff 269560.Jun 9 2020, 8:47 AM
  • Fixed use of %t.elf
gbreynoo marked 6 inline comments as done.Jun 9 2020, 9:14 AM

Thanks again MaskRay, to avoid confusion surrounding the term "type" I have renamed the test export-symbols and removed the list of symbols, a number of new uses of weak symbols have also been added to the test. I have also split dynamic-list-cpp into a new test due to the dynamic-list-glob.s name change.

Regarding the testing of shared and preemption I have seen that export-dynamic-symbol.s has testing for this regarding --export-dynamic-symbol, whilst --dynamic-list has testing in dynamic-list-preempt-replace-symbol.s and others. Whilst looking into the behaviour of --dynamic-export I struggled to see the same behaviour. Should it also be affecting shared objects in the same way?

I think testing of shared and preemption could be better expanded in similar tests as I described above. I should better describe the initial reasoning for export-symbols.s as the test is a little vague in what exactly it is covering and the misuse of the term type did not help. The idea was that this would be a test for a number of different symbols to ensure they were being exported, this didn't seem to fit into the existing tests and was not exhaustive but could be good additional coverage. Although a shared object is created in the test, this was only used to check for a symbol that found it's definition elsewhere.

lld/test/ELF/export-symbol-types.s
9 ↗(On Diff #267671)

You are right that this list was more confusing than helpful, I have removed it.

26 ↗(On Diff #267671)

The object file created here is different to the one above, one uses the echo as input, the other this file.

MaskRay accepted this revision.Jun 9 2020, 9:27 AM

Looks great!

lld/test/ELF/export-symbols.s
30

Will be nice to make the table aligned.

This revision is now accepted and ready to land.Jun 9 2020, 9:27 AM

FWIW binutils's --dynamic-list and --export-dynamic-symbol are updated a bit this month. The new semantics should match us. https://sourceware.org/pipermail/binutils/2020-June/

This revision was automatically updated to reflect the committed changes.
gbreynoo marked 2 inline comments as done.