This is an archive of the discontinued LLVM Phabricator instance.

Fix llvm-symbolizer to correctly sort a symbol array and calculate symbol sizes
ClosedPublic

Authored by kubamracek on Nov 10 2016, 6:27 PM.

Details

Summary

I noticed that llvm-symbolizer gives wrong results sometimes, this was due to incorrect sizes of some symbols. The reason for that was an incorrectly sorted array in computeSymbolSizes. The comparison function used subtraction of unsigned types, which is incorrect. Let's change this to return explicit -1 or 1.

Diff Detail

Repository
rL LLVM

Event Timeline

kubamracek updated this revision to Diff 77587.Nov 10 2016, 6:27 PM
kubamracek retitled this revision from to Fix llvm-symbolizer to correctly sort a symbol array and calculate symbol sizes.
kubamracek updated this object.
kubamracek added reviewers: aprantl, dvyukov, kcc, rafael.
kubamracek set the repository for this revision to rL LLVM.
kubamracek added subscribers: llvm-commits, zaks.anna.
kubamracek removed rL LLVM as the repository for this revision.

testcase?

Unfortunately, it's hard to come up with a reasonable testcase because the issue is hard to reproduce on small inputs (because the sorting usually works and only edge cases sort incorrectly). One of compiler-rt tests fail due to this, however. So with this patch, I can enable the test again. Is that enough?

Is it possible to manufacture a test in unittests/Object/ ?

I spend some time trying to, and I'm giving up on writing a subclass of ObjectFile that I could give to computeSymbolSizes. The best I could come up with was to pull SymEntry and compareAddress into a header file and just test that array_pod_sort sorts correctly. Will that do?

kubamracek updated this revision to Diff 77699.Nov 11 2016, 4:27 PM

Updating patch with a unittest.

dberris added inline comments.
unittests/Object/SymbolSizeTest.cpp
17–25 ↗(On Diff #77699)

This looks like you can use an initialiser list instead:

auto it = symbol_iterator(SymbolRef());
std::vector<SymEntry> Syms{
  SymEntry{it, ...},
  SymEntry{it, ...}};
29–31 ↗(On Diff #77699)

How about using something like std::is_sorted(...)?

33 ↗(On Diff #77699)

Do you even need this?

kubamracek added inline comments.Nov 14 2016, 8:55 AM
unittests/Object/SymbolSizeTest.cpp
17–25 ↗(On Diff #77699)

Right.

29–31 ↗(On Diff #77699)

The test is trying to make sure the sorting predicate is correct. Using is_sorted with the same predicate that sorted the array will not test that.

33 ↗(On Diff #77699)

Right, this shouldn't be here.

dberris accepted this revision.Nov 14 2016, 5:00 PM
dberris added a reviewer: dberris.
dberris added inline comments.
lib/Object/SymbolSize.cpp
19–25 ↗(On Diff #77835)

It would be good to document what the ordering guarantees are at the interface level for this function -- for instance, is this a strict weak ordering on (section id, address)?

This revision is now accepted and ready to land.Nov 14 2016, 5:00 PM
This revision was automatically updated to reflect the committed changes.

LLVM: r287028
compiler-rt: r287029