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

[Digger Lin via Phabricator via llvm-commits]

DiggerLin marked 3 inline comments as done.
DiggerLin added inline comments.


================
Comment at: llvm/test/tools/llvm-nm/XCOFF/export-symbols.test:72
+Symbols:
+  - Name:            export_protected_var
+    Section:         .data
----------------
jhenderson wrote:
> It's probably worth adding a comment to the start of each symbol block in this YAML explaining what case(s) that symbol covers.
I think most the symbol name can express what we want to test. I only added some visibility comment on the  YAML . and symbol name "var_extern"  which test the 
    if (SecIter == XCOFFObj->section_end())
      continue;


================
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
----------------
jhenderson wrote:
> 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).
thanks a lot


================
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:
> 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.
thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112735