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

[kubamracek]

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.

[kubamracek]

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?

[kubamracek]

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]

Updating patch with a unittest.

[dberris]

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]

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]

lib/Object/SymbolSize.cpp

28–34

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)?

[kubamracek]

LLVM: r287028
compiler-rt: r287029

lib/Object/SymbolSize.cpp

 struct SymEntry {
   symbol_iterator I;
   uint64_t Address;
   unsigned Number;
   unsigned SectionID;
 };
 }
 
 static int compareAddress(const SymEntry *A, const SymEntry *B) {
   if (A->SectionID != B->SectionID)
-    return A->SectionID - B->SectionID;
-  return A->Address - B->Address;
+    return A->SectionID < B->SectionID ? -1 : 1;
+  if (A->Address != B->Address)
+    return A->Address < B->Address ? -1 : 1;
+  return 0;
 }
 
 static unsigned getSectionID(const ObjectFile &O, SectionRef Sec) {
   if (auto *M = dyn_cast<MachOObjectFile>(&O))
     return M->getSectionID(Sec);
   return cast<COFFObjectFile>(O).getSectionID(Sec);
 }
 
 static unsigned getSymbolSectionID(const ObjectFile &O, SymbolRef Sym) {

projects/compiler-rt/test/asan/TestCases/use-after-scope-types.cc

 // RUN: %clangxx_asan %stdcxx11 -O0 -fsanitize-address-use-after-scope %s -o %t
 // RUN: not %run %t 0 2>&1 | FileCheck %s
 // RUN: not %run %t 1 2>&1 | FileCheck %s
 // RUN: not %run %t 2 2>&1 | FileCheck %s
 // RUN: not %run %t 3 2>&1 | FileCheck %s
 // RUN: not %run %t 4 2>&1 | FileCheck %s
 // RUN: not %run %t 5 2>&1 | FileCheck %s
 // RUN: not %run %t 6 2>&1 | FileCheck %s
 // RUN: not %run %t 7 2>&1 | FileCheck %s
 // RUN: not %run %t 8 2>&1 | FileCheck %s
 // RUN: not %run %t 9 2>&1 | FileCheck %s
 // RUN: not %run %t 10 2>&1 | FileCheck %s
 
-// Temporarily disable to fix the bot.
-// UNSUPPORTED: darwin
-
 #include <stdlib.h>
 #include <string>
 #include <vector>
 
 template <class T> struct Ptr {
   void Store(T *ptr) { t = ptr; }
 
   void Access() { *t = {}; }