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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-nm/XCOFF/export-symbols.test:5
+# RUN: yaml2obj --docnum=1 -DFLAG=0x0002 %s -o %t1.o
+# RUN: yaml2obj --docnum=2 -DFLAG=0x0002 -DSECT=2 %s -o %t2.o
+# RUN: yaml2obj --docnum=2 -DFLAG=0x0002 -DSECT=26 %s -o %t2_invalid.o
----------------
Tip: you can avoid the need for `-DSECT=2` in the "regular" case, by using a default value in the YAML. I can't remember if the syntax is `[[SECT=2]]` or `[[SECT:2]]`, but one of them should work (look for examples in other tests).


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:701
+      outs() << Sym.Name;
+      if (!Sym.Visibility.empty())
+        outs() << ' ' << Sym.Visibility;
----------------
Nit: clang-format is complaining.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1668
+           ArchiveName);
+      return;
+    }
----------------
Should this be a `continue` rather than a return? Also, should it be closer to the rest of the name checks further down in this loop?


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1677-1713
+    if (HasVisibilityAttr) {
+      XCOFFSymbolRef XCOFFSym = XCOFFObj->toSymbolRef(Sym.getRawDataRefImpl());
+      uint16_t SymType = XCOFFSym.getSymbolType();
+      if ((SymType & XCOFF::VISIBILITY_MASK) == XCOFF::SYM_V_INTERNAL)
+        continue;
+      if ((SymType & XCOFF::VISIBILITY_MASK) == XCOFF::SYM_V_HIDDEN)
+        continue;
----------------
I haven't got the time right now to check this myself, but you should ensure that you have symbols that trigger every possible code path that lead to them being skipped/not skipped.

A quick skim suggests at least the following:
# INTERNAL visibility
# HIDDEN visibility
# No visibility attribute
# Visibility attribute that isn't internal or hidden.
# Symbol with error when looking up section
# Symbol that is not in a section (`SecIter == XCOFFObj->section_end()`)
# Text symbol
# Data symbol
# BSS symbol
# Symbol that is not one of the three previous types
# Symbol with empty name
# Symbol with name look-up failure
# __sinit prefixed symbol
# __sterm prefixed symbol
# . prefixed symbol
# ( prefixed symbol
# `____` named symbol (i.e. four underscores, nothing between prefix and suffix)
# `__<digits>__` symbol
# `__<digits>` (no suffix)
# `<digits>__` (no prefix)
# `__<digits and something non digit>__` symbol




================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1720-1721
+
+    if (SymName == "__rsrc" && NoRsrc)
+      continue;
+
----------------
This block appears after the name adjustments for `__tf1` and `__tf9`. If the intent is that `__tf1__rsrc` and `__tf9_rsrc` are skipped with --no-rsrc, you need to check them. In fact, it probably doens't hurt to have that test case even if they should be kept.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1773-1777
+  if (ExportSymbols && Obj.isXCOFF()) {
+    XCOFFObjectFile *XCOFFObj = cast<XCOFFObjectFile>(&Obj);
+    getXCOFFExports(XCOFFObj, ArchiveName);
+    return;
+  }
----------------
I'd be tempted to move this block right to the start of the function, since the other bits are completely irrelevant if we trigger this block.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1849
       S.Sym = Sym;
-      SymbolList.push_back(S);
+      if (S.setSymFlag(Obj))
+        SymbolList.push_back(S);
----------------
DiggerLin wrote:
> jhenderson wrote:
> > DiggerLin wrote:
> > > jhenderson wrote:
> > > > I don't understand the need for this addition.
> > > without setSymFlag(Obj).  the S do not have a value SymFlags, it will crash at function
> > >  bool shouldSkipPrinting()
> > But the code didn't have this before, so why does it need it now? Is this a result of some of the refactoring work you've already done? I take it there's a concrete reproducible you could craft that would cause the crash without this change?
> the function  shouldSkipPrinting() is called
> in the printExportSymbolList() 
> but the printExportSymbolList are called after dumpSymbolNamesFromFile , The object binary file maybe be freed from memory when there are  several files inputs from command line. in the function dumpSymbolNamesFromFile() ,
> 
> 
> ```
> Expected<std::unique_ptr<Binary>> BinaryOrErr =
>       createBinary(BufferOrErr.get()->getMemBufferRef(), ContextPtr);
> ```
> 
>  it generate a binary and freed it at the end of the function . so  if we still try to get the  SymFlags  of symbol from bit code object file , it will be crashed.
> 
> so I set the SymFlag for getSymbolNamesFromObject() 
> as 
> 
> 
> ```
>       if (S.initializeFlags(Obj))
>         SymbolList.push_back(S);
> ```
> 
> 
> dumpSymbolsFromDLInfoMachO（） initiate the SymFlag  for SymFlags . but  getSymbolNamesFromObject do not set it,  for consistency， I set the SymFlag   for the NMSymbol in getSymbolNamesFromObject  
Thanks. It would be nice we can avoid this issue with the proposed refactoring (and therefore delete this new code), but if it's needed for now, so be it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112735