[PATCH] D112735: export unique symbol list for xcoff with llvm-nm new option "--export-symbols"

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

Sorry for the belated response.



================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:243
 
+static constexpr uint16_t VISIBILITY_MASK = 0x7000;
+
----------------
`constexpr uint16_t VISIBILITY_MASK = 0x7000;`


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:270
+static bool compareSymbolNameVisibility(const NMSymbol &A, const NMSymbol &B) {
+  return std::make_tuple(A.Name, A.Visibility) <
+         std::make_tuple(B.Name, B.Visibility);
----------------
Can two symbols with the same name have different visibilities?

In ELF, it can't. If you want to make the result stable on all platforms, use stable_sort.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:688
+static void removeAdjacentDuplicates() {
+  if (SymbolList.size() == 0)
+    return;
----------------
The whole function can be replaced with `llvm::unique` (see std::unique).

You can create an `operator==` comparator from the existing `operator<` comparator.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:722
+    if (!Sym.Visibility.empty())
+      outs() << " " << Sym.Visibility;
+    outs() << "\n";
----------------



================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:723
+      outs() << " " << Sym.Visibility;
+    outs() << "\n";
   }
----------------
char is more efficient and makes the linked output smaller


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1694
+  // Skip Shared object file.
+  if (Obj->getFlags() & XCOFF::F_SHROBJ)
+    return;
----------------
Don't shared objects have exported symbols as well? Why are them skipped?


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1725
+
+    XCOFFObjectFile *XCOFFObj = dyn_cast<XCOFFObjectFile>(Obj);
+    assert(XCOFFObj && "Not an XCOFFObjectFile in exportSymbolsForXCOFF().");
----------------
Use `cast<` to assert it has the intended type. Drop assert


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1755
+
+    Regex r("^__[0-9]+__");
+    if (SymName.startswith("__sinit") || SymName.startswith("__sterm") ||
----------------
Regex library is large and slow. Try avoiding it.

The pattern is simple. You may just open coding the pattern.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1760
+
+    if (SymName.startswith("__tf1")) {
+      SymName = SymName.substr(6);
----------------
https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112735/new/

https://reviews.llvm.org/D112735