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

[Digger Lin via Phabricator via llvm-commits]

DiggerLin added inline comments.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1668
+           ArchiveName);
+      return;
+    }
----------------
jhenderson wrote:
> 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?
thanks


================
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;
----------------
jhenderson wrote:
> 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
> 
> 
I added all test cases you mention above except the "12 Symbol with name look-up failure"

from the source code in the XCOFFObjectFile.cpp

```
Expected<StringRef> XCOFFSymbolRef::getName() const {
  // A storage class value with the high-order bit on indicates that the name is
  // a symbolic debugger stabstring.
  if (getStorageClass() & 0x80)
    return StringRef("Unimplemented Debug Name");

  if (Entry32) {
    if (Entry32->NameInStrTbl.Magic != XCOFFSymbolRef::NAME_IN_STR_TBL_MAGIC)
      return generateXCOFFFixedNameStringRef(Entry32->SymbolName);

    return OwningObjectPtr->getStringTableEntry(Entry32->NameInStrTbl.Offset);
  }

  return OwningObjectPtr->getStringTableEntry(Entry64->Offset);
}
```
It never return  an Error.

 The code in the function exportSymbolsForXCOFF

   
```
 Expected<StringRef> NameOrErr = Sym.getName();
    if (!NameOrErr) {
      warn(NameOrErr.takeError(), XCOFFObj->getFileName(),
           "for symbol with index " +
               Twine(XCOFFObj->getSymbolIndex(Sym.getRawDataRefImpl().p)),
           ArchiveName);
      continue;
    }
    StringRef SymName = *NameOrErr;
```

can be modified to 


```
StringRef SymName = cantFail(Sym.getName());
```

but  I prefer to keep as it is. The reason as:

if  Expected<StringRef> XCOFFSymbolRef::getName() const is changed to return with Error in some day.

using StringRef SymName = cantFail(Sym.getName()); will cause an llvm-unreable.



================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1720-1721
+
+    if (SymName == "__rsrc" && NoRsrc)
+      continue;
+
----------------
jhenderson wrote:
> 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.
good catch, thanks , the __tf1___rsrc should be export out as "__rsrc" , I changed the code and add test case for it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112735