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

[Digger Lin via Phabricator via llvm-commits]

DiggerLin marked 5 inline comments as done.
DiggerLin added a comment.

In D112735#3307226 <https://reviews.llvm.org/D112735#3307226>, @jhenderson wrote:

> 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.

Thanks, I can start the refactor from next week.

In D112735#3307226 <https://reviews.llvm.org/D112735#3307226>, @jhenderson wrote:

> 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.

  Thanks a lot , I will begin to refactor as soon as possible, I think I can begin from later of next week. 



================
Comment at: llvm/test/tools/llvm-nm/XCOFF/export-symbols-with-invalid-section-index.test:1
+## Test the behavior of the symbol reference section.
+
----------------
jhenderson wrote:
> 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?
thanks.


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


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:254
+
+  bool shouldSkipPrintingSymbol() const {
+    bool Undefined = SymFlags & SymbolRef::SF_Undefined;
----------------
jhenderson wrote:
> 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.
done ,thanks 


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1784-1788
+    if (ExportSymbols && Obj.isXCOFF()) {
+      XCOFFObjectFile *XCOFFObj = cast<XCOFFObjectFile>(&Obj);
+      exportSymbolsForXCOFF(XCOFFObj, ArchiveName);
+      return;
+    }
----------------
jhenderson wrote:
> 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.
thanks


================
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);
----------------
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  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112735