This is an archive of the discontinued LLVM Phabricator instance.

[lld][ELF][test] Expand testing of symbols in mergeable sections
ClosedPublic

Authored by jhenderson on Aug 25 2020, 8:03 AM.

Details

Summary

Whilst reviewing some internal testing, I noticed a couple of holes in coverage of mergeable sections containing symbols. This patch addresses these holes:

  1. Show that mid-piece symbols have their values updated properly when pieces are merged.
  2. Show the behaviour of symbols in mergeable pieces when --gc-sections is enabled.

Also make a test more robust to address variations.

Diff Detail

Event Timeline

jhenderson created this revision.Aug 25 2020, 8:03 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: emaste. · View Herald Transcript
jhenderson requested review of this revision.Aug 25 2020, 8:04 AM
MaskRay added inline comments.Aug 25 2020, 8:32 AM
lld/test/ELF/merge-gc-sym.s
1 ↗(On Diff #287668)

Thanks for adding coverage. There are gc-sections-merge*.s. Can you check whether these tests can be reorganized?

grimar accepted this revision.Aug 26 2020, 1:49 AM

This change LG. merge-gc-sym.s name is consistent with merge-sym.s.

I'd do any futher reorganising/cleanup of gc-sections-merge*.s separatelly.

This revision is now accepted and ready to land.Aug 26 2020, 1:49 AM
jhenderson added inline comments.Aug 26 2020, 2:00 AM
lld/test/ELF/merge-gc-sym.s
1 ↗(On Diff #287668)

The gc-sections-merge*.s tests all are testing how difference reference variations cause mergeable pieces to be discarded. The three tests essentially translate into using R_X86_64_64 with no addend, R_X86_64_64 with an addend that could be confused with referencing the next piece, and R_X86_64_PC32, where the implicit addend could again cause the same confusion. These are all useful behaviours to test, and are independent of the impact on symbols in mergeable pieces. I think they belong as separate tests consequently (although perhaps renaming them/adding comments to explain things more explicitly might be worthwhile?).

What sort of reorganization did you have in mind?

jhenderson edited the summary of this revision. (Show Details)
  • Rename merge-gc-sym.s to merge-sym-gc.s since it is a parallel test to merge-sym.s.
  • Fixed merge-sym.s. The bar and foo symbols seem to get reordered. Their order doesn't matter, so to make the test robust, I have switched to use llvm-readelf (for single-line symbol descriptions) and CHECK-DAG.
  • Whilst updating merge-sym.s, I also took the chance to remove the hard-coded address values, making the test more robust.

Upload correct diff.

Latest version LGTM too.