[NFC] Refactor llvm-nm symbol comparing and split sorting

[DiggerLin]

Summary

1. added a helper function isSymbolDefined().
2. Split out sorting code
3. refactor symbol comparing function

[MaskRay]

Mostly looks good

llvm/tools/llvm-nm/llvm-nm.cpp

226–231

230

https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code

Re-align the next line.

249

Is operator> used? If used, can the use site be refactored to only use operator<?

[DiggerLin]

llvm/tools/llvm-nm/llvm-nm.cpp

226–231

thanks

230

thanks

249

yes, the operator> is used to change

llvm::sort(SymbolList, [=](const NMSymbol &A, const NMSymbol &B) -> bool {
       return Cmp(B, A);
     });

---->

llvm::stable_sort(SymbolList, std::greater<NMSymbol>());

there is Jame's comment as
https://reviews.llvm.org/D112735#inline-1137095

[jhenderson]

Mostly looks fine to me.

Change the title/commit summary to "[NFC] Refactor llvm-nm symbol sorting" (will need the additional patch mentioned inline).

llvm/tools/llvm-nm/llvm-nm.cpp

234–235

Since the NMSymbol class has no private members, do you need the friend operator declarations?

249

Just noting that if @MaskRay prefers using the lambda and avoid the operator>, I'm okay with that.

658–660

I'm okay with this switch to stable_sort, but if I'm not mistaken, this means that under some cases, it might be a subtle behaviour change (in a good way) that could result in people seeing "identical" symbols in different orders before and after this patch.

To keep this patch as a pure refactor, I suggest you create a separate patch that handles this behaviour change, by simply swapping sort to stable_sort. Then, you can add the [NFC] tag to this patch.

[DiggerLin]

llvm/tools/llvm-nm/llvm-nm.cpp

234–235

thanks , good catch.

[jhenderson]

Change the title/commit summary to "[NFC] Refactor llvm-nm symbol sorting" (will need the additional patch mentioned inline).

Patch title still doesn't match the suggested correction.

[DiggerLin]

In D119028#3300831, @jhenderson wrote:

Change the title/commit summary to "[NFC] Refactor llvm-nm symbol sorting" (will need the additional patch mentioned inline).

Patch title still doesn't match the suggested correction.

In D119028#3300831, @jhenderson wrote:

Change the title/commit summary to "[NFC] Refactor llvm-nm symbol sorting" (will need the additional patch mentioned inline).

Patch title still doesn't match the suggested correction.

  I do not think we need to additional patch for it. using std::sort  is OK for export symbol list,  " export symbol list" do not need to change std::stable_sort (if the two symbol is equal at symbol name and visibility, and we will remove the duplication symbol finally). 
and for other functionality, the code is already using the std::sort , do we need to change it?

[jhenderson]

I'm good with this, but please wait for @MaskRay too.

[MaskRay]

Thanks!

llvm/tools/llvm-nm/llvm-nm.cpp

249

OK, operator> is fine since there is a std::greater<NMSymbol>().

658

Optional: this can be written as llvm::sort(SymbolList, std::greater<>()); to reduce boilerplate.

[MaskRay]

llvm/tools/llvm-nm/llvm-nm.cpp

249

Actually, I think it'd be better to do:

llvm::sort(Symbols);
if (ReverseSort)
  std::reverse(Symbols.begin(), Symbols.end());

NMSymbol is not trivially copyable, so llvm::sort expands to std::sort.
Having two instantiations (less and greater) increase the code size too much.
std::reverse can decrease the code size.

[DiggerLin]

llvm/tools/llvm-nm/llvm-nm.cpp

249

yes, it can reduce the code a little, to use the
But there is one sort, one reverse maybe less efficient than one std::greater<NMSymbol>(). sort.

658

thanks, change as suggestion when landed

llvm/tools/llvm-nm/llvm-nm.cpp

struct NMSymbol {
   // below are filled in to represent what would have been a Mach-O nlist
   // native symbol.
   uint32_t SymFlags;
   SectionRef Section;
   uint8_t NType;
   uint8_t NSect;
   uint16_t NDesc;
   std::string IndirectName;
-};
-} // anonymous namespace
 
-static bool compareSymbolAddress(const NMSymbol &A, const NMSymbol &B) {
-  bool ADefined;
-  // Symbol flags have been checked in the caller.
-  if (A.Sym.getRawDataRefImpl().p) {
-    uint32_t AFlags = cantFail(A.Sym.getFlags());
-    ADefined = !(AFlags & SymbolRef::SF_Undefined);
-  } else {
-    ADefined = A.TypeChar != 'U';
+  bool isDefined() const {
+    if (Sym.getRawDataRefImpl().p) {
+      uint32_t Flags = cantFail(Sym.getFlags());
+      return !(Flags & SymbolRef::SF_Undefined);
     }
-  bool BDefined;
-  // Symbol flags have been checked in the caller.
-  if (B.Sym.getRawDataRefImpl().p) {
-    uint32_t BFlags = cantFail(B.Sym.getFlags());
-    BDefined = !(BFlags & SymbolRef::SF_Undefined);
-  } else {
-    BDefined = B.TypeChar != 'U';
-  }
-  return std::make_tuple(ADefined, A.Address, A.Name, A.Size) <
-         std::make_tuple(BDefined, B.Address, B.Name, B.Size);
+    return TypeChar != 'U';
   }
 
-static bool compareSymbolSize(const NMSymbol &A, const NMSymbol &B) {
+  friend bool operator<(const NMSymbol &A, const NMSymbol &B);
+  friend bool operator>(const NMSymbol &A, const NMSymbol &B);
+};
+
+bool operator<(const NMSymbol &A, const NMSymbol &B) {
+  if (NumericSort)
+    return std::make_tuple(A.isDefined(), A.Address, A.Name, A.Size) <
+           std::make_tuple(B.isDefined(), B.Address, B.Name, B.Size);
+  if (SizeSort)
     return std::make_tuple(A.Size, A.Name, A.Address) <
            std::make_tuple(B.Size, B.Name, B.Address);
-}
-
-static bool compareSymbolName(const NMSymbol &A, const NMSymbol &B) {
   return std::make_tuple(A.Name, A.Size, A.Address) <
          std::make_tuple(B.Name, B.Size, B.Address);
 }
 
+bool operator>(const NMSymbol &A, const NMSymbol &B) { return B < A; }
+} // anonymous namespace
+
 static char isSymbolList64Bit(SymbolicFile &Obj) {
   if (auto *IRObj = dyn_cast<IRObjectFile>(&Obj))
     return Triple(IRObj->getTargetTriple()).isArch64Bit();
   if (isa<COFFObjectFile>(Obj) || isa<COFFImportFile>(Obj))
     return false;
   if (XCOFFObjectFile *XCOFFObj = dyn_cast<XCOFFObjectFile>(&Obj))
     return XCOFFObj->is64Bit();
-
   if (isa<WasmObjectFile>(Obj))
     return false;
   if (TapiFile *Tapi = dyn_cast<TapiFile>(&Obj))
     return Tapi->is64Bit();
   if (MachOObjectFile *MachO = dyn_cast<MachOObjectFile>(&Obj))
     return MachO->is64Bit();
   return cast<ELFObjectFileBase>(Obj).getBytesInAddress() == 8;
 }
if (OutputFormat == posix && !ArchiveName.empty())
     S << ArchiveName << "[" << CurrentFilename << "]: ";
   else {
     if (!ArchiveName.empty())
       S << ArchiveName << ":";
     S << CurrentFilename << ": ";
   }
 }
 
-static void sortAndPrintSymbolList(SymbolicFile &Obj, bool printName,
-                                   StringRef ArchiveName,
-                                   StringRef ArchitectureName) {
-  if (!NoSort) {
-    using Comparator = bool (*)(const NMSymbol &, const NMSymbol &);
-    Comparator Cmp;
-    if (NumericSort)
-      Cmp = &compareSymbolAddress;
-    else if (SizeSort)
-      Cmp = &compareSymbolSize;
-    else
-      Cmp = &compareSymbolName;
+static void sortSymbolList() {
+  if (NoSort)
+    return;
 
-    if (ReverseSort)
-      llvm::sort(SymbolList, [=](const NMSymbol &A, const NMSymbol &B) -> bool {
-        return Cmp(B, A);
-      });
-    else
-      llvm::sort(SymbolList, Cmp);
-  }
+    llvm::stable_sort(SymbolList, std::greater<NMSymbol>());
+    llvm::stable_sort(SymbolList);
 
+static void printSymbolList(SymbolicFile &Obj, bool printName,
+                            StringRef ArchiveName, StringRef ArchitectureName) {
   if (!PrintFileName) {
     if ((OutputFormat == bsd || OutputFormat == posix ||
          OutputFormat == just_symbols) &&
         MultipleFiles && printName) {
       outs() << '\n' << CurrentFilename << ":\n";
     } else if (OutputFormat == sysv) {
       outs() << "\n\nSymbols from " << CurrentFilename << ":\n\n";
       if (isSymbolList64Bit(Obj))
case Radix::x:
       break;
     default:
       printFormat = OutputFormat == posix ? "%" PRId64 : "%08" PRId64;
     }
   }
 
   for (const NMSymbol &S : SymbolList) {
     uint32_t SymFlags;
-    std::string Name = S.Name;
-    MachOObjectFile *MachO = dyn_cast<MachOObjectFile>(&Obj);
-    if (Demangle) {
-      function_ref<Optional<std::string>(StringRef)> Fn = ::demangle;
-      if (Obj.isXCOFF())
-        Fn = demangleXCOFF;
-      if (Obj.isMachO())
-        Fn = demangleMachO;
-      if (Optional<std::string> Opt = Fn(S.Name))
-        Name = *Opt;
-    }
     if (S.Sym.getRawDataRefImpl().p) {
       Expected<uint32_t> SymFlagsOrErr = S.Sym.getFlags();
       if (!SymFlagsOrErr) {
         // TODO: Test this error.
         error(SymFlagsOrErr.takeError(), Obj.getFileName());
         return;
       }
       SymFlags = *SymFlagsOrErr;
     } else
       SymFlags = S.SymFlags;
 
     bool Undefined = SymFlags & SymbolRef::SF_Undefined;
     bool Global = SymFlags & SymbolRef::SF_Global;
     bool Weak = SymFlags & SymbolRef::SF_Weak;
     bool FormatSpecific = SymFlags & SymbolRef::SF_FormatSpecific;
     if ((!Undefined && UndefinedOnly) || (Undefined && DefinedOnly) ||
         (!Global && ExternalOnly) || (Weak && NoWeakSymbols) ||
         (FormatSpecific && !(SpecialSyms || DebugSyms)))
       continue;
+
+    std::string Name = S.Name;
+    MachOObjectFile *MachO = dyn_cast<MachOObjectFile>(&Obj);
+    if (Demangle) {
+      function_ref<Optional<std::string>(StringRef)> Fn = ::demangle;
+      if (Obj.isXCOFF())
+        Fn = demangleXCOFF;
+      if (Obj.isMachO())
+        Fn = demangleMachO;
+      if (Optional<std::string> Opt = Fn(S.Name))
+        Name = *Opt;
+    }
+
     if (PrintFileName)
       writeFileName(outs(), ArchiveName, ArchitectureName);
     if ((OutputFormat == just_symbols ||
          (UndefinedOnly && MachO && OutputFormat != darwin)) &&
         OutputFormat != posix) {
       outs() << Name << "\n";
       continue;
     }
static void dumpSymbolNamesFromObject(SymbolicFile &Obj, bool printName,
 
   CurrentFilename = Obj.getFileName();
 
   if (Symbols.empty() && SymbolList.empty() && !Quiet) {
     writeFileName(errs(), ArchiveName, ArchitectureName);
     errs() << "no symbols\n";
   }
 
-  sortAndPrintSymbolList(Obj, printName, ArchiveName, ArchitectureName);
+  sortSymbolList();
+  printSymbolList(Obj, printName, ArchiveName, ArchitectureName);
 }
 
 // checkMachOAndArchFlags() checks to see if the SymbolicFile is a Mach-O file
 // and if it is and there is a list of architecture flags is specified then
 // check to make sure this Mach-O file is one of those architectures or all
 // architectures was specificed.  If not then an error is generated and this
 // routine returns false.  Else it returns true.
 static bool checkMachOAndArchFlags(SymbolicFile *O, std::string &Filename) {