[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:72
+Symbols:
+  - Name:            export_protected_var
+    Section:         .data
----------------
It's probably worth adding a comment to the start of each symbol block in this YAML explaining what case(s) that symbol covers.


================
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;
----------------
DiggerLin wrote:
> 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.
> 
I think you should switch to `cantFail`, unless you know that you are going to change `getName` soon. The reasons are:
1) it simplifies the code dramatically, making it more readable.
2) you can't test the current code, so there's no guarantee that even if `getName` is changed that it is handled here appropriately.
3) if `getName` is changed to return an Error at some point, call sites should be audited for additional checks/tests that are needed. As such, this case should be picked up anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112735