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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Okay, I'm willing to accept delaying further refactoring, as long as it is definitely going to happen sooner rather than later. Just a few nits to tidy up before I think you can land this patch.



================
Comment at: llvm/test/tools/llvm-nm/XCOFF/export-symbols-with-invalid-section-index.test:1
+## Test the behavior of the symbol reference section.
+
----------------
Could you fold this test case into the positive test case file? Would that allow you to reuse the YAML object, using -D to provide the invalid section index?


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1658
 
+static void exportSymbolsForXCOFF(XCOFFObjectFile *XCOFFObj,
+                                  StringRef ArchiveName) {
----------------
Let's rename this function now: `getXCOFFExports` seems like a nice concise name.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:254
+
+  bool shouldSkipPrintingSymbol() const {
+    bool Undefined = SymFlags & SymbolRef::SF_Undefined;
----------------
DiggerLin wrote:
> jhenderson wrote:
> > It's easier to work with positives, so I'd suggest renaming this `shouldPrint`, and invert the logic.
> thanks
Just noting this hasn't been done, although you can defer to another patch if you prefer.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1784-1788
+    if (ExportSymbols && Obj.isXCOFF()) {
+      XCOFFObjectFile *XCOFFObj = cast<XCOFFObjectFile>(&Obj);
+      exportSymbolsForXCOFF(XCOFFObj, ArchiveName);
+      return;
+    }
----------------
DiggerLin wrote:
> jhenderson wrote:
> > This code doesn't really belong here, as it's just filtering out the export symbols. Later code will already retrieve XCOFf symbols without needing to do anything extra - we should filter that set of symbols after that point.
> > 
> > This block here is also a good example of why the current implementation in this patch is not a good structure. It looks like none of the code before this block is relevant. The `if (DynamicSyms)` block isn't relevant, becuase that's only for ELF. The `if (!SegSect.empty() && MachO)` block is also irrelevant, since an object cannot be both MachO and XCOFF. That also implies that `if (!(MachO && DyldInfoOnly))` is always true, if we are XCOFF. Nothing after this block is relevant either, since it returns unconditionally. This shows that this function is not the place for this piece of code.
> agree with you , but I do not think there is better choice in current structure.
Let's at least in this patch pull the block outside of the `if (!(MachO && DyldInfoOnly))` clause, since it doesn't need to be inside it.


================
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:
> > 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112735