[llvm-nm]add helper function to print out the object file name, archive name, architecture name

[DiggerLin]

Summary

1. added helper function printObjectNamesInfo() to print out object file name, archive name, architecture name.
2. One small behaviors change.
   
   in the function dumpMachOUniversalBinaryArchAll , delete the functionality:

if (moreThanOneArch)
               outs() << "\n";

[MaskRay]

llvm/tools/llvm-nm/llvm-nm.cpp

1863–1893

The default arguments are unused

1865

1872

remove braces for single-line simple statements

[DiggerLin]

llvm/tools/llvm-nm/llvm-nm.cpp

1863–1893

thanks.

[MaskRay]

llvm/tools/llvm-nm/llvm-nm.cpp

1865

Not fixed.

1876

Try const SymbolicFile &Obj and fix some non-const references in the body.

[jhenderson]

Please don't mark a patch as a refactor when there are behaviour changes: by definition "refactor" means a change with no change in behaviour.

Please don't change the output of llvm-nm, at least for regular ELF output and probably for all foramts, unless GNU nm output has changed. The LLVM tools like llvm-nm, llvm-readelf, llvm-objdump are supposed to produce pretty much identical output to the GNU utilities, so that they can be used as drop-in replacements. The version of GNU nm on my machine prints archive member names as member.o: not archive.a(member.o): or similar. I do agree that the proposed output is better, but we can't break this requirement, unless there's buy-in from the GNU community, as lots of people rely on automated scripts to parse the tool output.

llvm/tools/llvm-nm/llvm-nm.cpp

1877–1878

It's not clear what the difference is between PrintName and PrintObjectNameInfo from just the variable names. I suggest renaming PrintName to be more specific (I think it's PrintSymbolName right?). Also, I'd drop "Info" from the PrintObjectNameInfo variable name.

2231–2232

Label these booleans with comments, i.e. as per the inline edit.

Same goes at the other call sites.

[DiggerLin]

llvm/tools/llvm-nm/llvm-nm.cpp

1876

Agree with you , but if I change the to const SymbolicFile &Obj, a lot of functions's parameter need to "const SymbolicFile &Obj" ,
for example, printSymbolList() ,isSymbolList64Bit() , I am prefer to keep as it is now. and another separate patch for it. if you strong suggest that we need to change it the patch. I can do it.

1877–1878

it is not printing symbol name. it print object file name relate info. I thin PrintName is too generic. I used PrintObjectName?

[DiggerLin]

llvm/tools/llvm-nm/llvm-nm.cpp

1877–1878

sorry for misunderstand comment, please ignore above comment.

1. I changed PrintName to PrintSymObjectName
2. changed PrintObjectNameInfo to PrintObjectName

[jhenderson]

in the function dumpMachOUniversalBinaryArchAll , delete the functionality:

Your commit message indicates that there's a functional change, but there's no corresponding test change. That implies there's either a missing test, or you've broken an existing test. I suggest adding a test to cover the output difference.

llvm/tools/llvm-nm/llvm-nm.cpp

1863–1893

Please remember UpperCamelCase for variable names.

That being said, I'm not convinced IsArchive is especially meaningful or even correct, given that the false case is also an archive member sometimes. I suggest just passing in an empty string for ArchiveName if the archive name shouldn't be printed.

You're also only printing one object, so I recommend against calling it printObjectNames (plural). Even though you are printing multiple names, there is only one name for the object; the rest just provide additional context.

1876

+1 to leaving it for now, but fixing in a separate patch, since the situation is no worse than it was before.

1877–1878

I'm still not a massive fan of the variable names, as they are still somewhat unclear. How about PrintSymbolObject and PrintObjectLabel respectively?

I use "Label" in this context, because it's similar to assembly or C labels, so should be familiar enough.

2216–2217

Here and throughout, there's no need to label the parameters with comments if the input makes it somewhat obvious what they may represent. I personally use it only for literals like true, false, 0, 1 and {} etc. In other words, remove the last two labels here and anywhere where a variable is being passed in, rather than a literal.

[DiggerLin]

This comment was removed by DiggerLin.

llvm/tools/llvm-nm/llvm-nm.cpp

1863–1893

That being said, I'm not convinced IsArchive is especially meaningful or even correct, given that the false case is also an archive member sometimes.

I changed to IsArchiveFile.

I suggest just passing in an empty string for ArchiveName if the archive name shouldn't be printed.

I do not think we can set the ArchiveName to empty if archive name should not print in the printObjectName , for the function writeFileName(errs(), ArchiveName, ArchitectureName); and printSymbolList(Obj, PrintSymObjectName, ArchiveName, ArchitectureName) still need it .
in the dumpSymbolNamesFromObject.

1877–1878

OK.

2126

I think it is bug of the code,

if (moreThanOneArch)

outs() << "\n";

if the MachOUniversalBinary has more than one object , it print as "

"
(there is one empty line here)
a.o:

XXXXXX

b.o:

XXXXXX"

if only have one object file , it printed as

"a.o: //(there is no empty before the a.o)
XXXXXXX
"

it is not consistency for first object file a.o (whether there is a empty line before the first object file name(or Lable)"

[DiggerLin]

In D120357#3348530, @jhenderson wrote:

in the function dumpMachOUniversalBinaryArchAll , delete the functionality:

Your commit message indicates that there's a functional change, but there's no corresponding test change. That implies there's either a missing test, or you've broken an existing test. I suggest adding a test to cover the output difference.

in the original code,

if (moreThanOneArch)

outs() << "\n";

if the MachOUniversalBinary has more than one object , it print as

"
(there is one empty line here)
a.o:

XXXXXX

b.o:

XXXXXX"

if only have one object file , it printed as

"a.o: //(there is no empty before the a.o)
XXXXXXX
"
it is not consistency for first object file a.o (whether there is a empty line before the first object file name(or Lable)" for the MachOUniversalBinary , and other file format of the code
there do test there is no empty line for the first object file where is only one object file in the MachOUniversalBinary .

I change to always print out the

"
(there is one empty line here)
a.o:

XXXXXX
"
even there is only object file in the MachOUniversalBinary.

I do not think we need test for a empty line of the first object file of the MachOUniversalBinary.

[jhenderson]

I do not think we need test for a empty line of the first object file of the MachOUniversalBinary.

It is important to test whitespace as much as non-whitespace, as we don't want regressions in how things are printed (e.g. double blank lines, no blank lines when expected etc). This can make the output look messy, apart from anything else.

Having looked at the output, I agree that having a blank line at the start makes sense, but I want to be sure we enshrine this in a test.

Tip: when posting comments including code blocks or tool output etc, use the "Code Block" formatting option (left of the quote button in the Phabricator comment toolbar). The markup is three "`" characters for a multi-line output, or just one for inline content, at each end of the block.

llvm/tools/llvm-nm/llvm-nm.cpp

1863–1893

IsArchiveFile is no better: it has the same problems:

1. The input file for this function is not an archvie. It's an object (which may be part of an archive).
2. The code path for !IsArchiveFile is only relevant when the input file IS a member of an archive, which is contradictory to the name of the parameter.

How about PrintArchiveName (and flipping its logic).

1997

As already requested...

1999

2029

2062

2114

2147

2169

[DiggerLin]

This comment was removed by DiggerLin.

llvm/tools/llvm-nm/llvm-nm.cpp

1863–1893

OK

1997

from the '!PrintFileName" , we can not know it is the value of parameter of "PrintObjectLabel" at first glance , so I keep the comment here on purpose. I delete it anyway.

[jhenderson]

llvm/test/Object/nm-universal-binary.test

37–38

Adding {{[[:space:]].*}} does literally nothing useful, as the whitespace at the start of a line is ignored unless using both --strict-whitespace and --match-full-lines in the FileCheck command. I believe, if you want to check the first line is empty, that you can use:

CHECK-AR:      {{^$}}
CHECK-AR-NEXT: macho-universal-archive.x86_64.i386(hello.o) (for architecture x86_64):

For later blank lines, you can do CHECK-EMPTY: immediately following a regular CHECK. CHECK-EMPTY works like CHECK-NEXT but ensures the line is completely blank instead of checking against some pattern. Since it works like CHECK-NEXT it cannot be used as the first CHECK pattern however. Conversely, {{^$}} will not always work if used after the first CHECK pattern. This is because FileCheck consumes the string it has matched up to before running the next CHECK. For example, imagine the output was:

foo
bar

And we have the following CHECKs:

CHECK: foo
CHECK-NEXT: {{^$}}
CHECK-NEXT: bar

to test that there is a blank line between foo and bar. In this case, after matching foo, the output will look like a blank line, followed by a line containing bar. The attempt to check for an empty line matches the first line (at the end of foo), which is not the line after the previous match, so the CHECK-NEXT fails.

(Aside, not something for you to fix, but this test is full of holes, and could fail to catch symbols in the output that shouldn't be present, or other additional lines).

48–49

See above.

llvm/tools/llvm-nm/llvm-nm.cpp

1916

Why have you just introduced this change? How is it better than the previous code?

[DiggerLin]

llvm/tools/llvm-nm/llvm-nm.cpp

1916

yes, in the original code of function getSymbolNamesFromObject( )

auto Symbols = Obj.symbols();
.....
 
if (DynamicSyms) {
  const auto *E = dyn_cast<ELFObjectFileBase>(&Obj);
  if (!E) {
    error("File format has no dynamic symbol table", Obj.getFileName());
    return;
  }
Symbols = E->getDynamicSymbolIterators();

The value of Symbols be changed by the line
Symbols = E->getDynamicSymbolIterators();
Symbols maybe not the equal to Obj.symbols()

so we can not use the
if ( Obj.symbols().empty() && SymbolList.empty() && !Quiet)

[jhenderson]

llvm/tools/llvm-nm/llvm-nm.cpp

1916

Thanks, I see. I think I'd prefer it if you pulled the logic on whether or not an object has symbols into a separate function (maybe hasSymbols) which takes Obj and returns the value based on the appropriate set of symbols for whether DynamicSyms is specified or not. You probably would then benefit from a separate helper function used by this new function and getSymbolNamesFromObject that returns the dynamic symbols list (or an empty list if there was an error). Something like the following:

static bool hasSymbols(SymbolicFile &Obj) {
  if (DynamicSyms)
    return !getDynamicSyms(Obj).empty();
  return !Obj.symbols().empty();
}

static <insert return type here> getDynamicSyms(SymbolFile &Obj) {
  const auto *E = dyn_cast<ELFObjectFileBase>(&Obj);
  if (!E) {
    error("File format has no dynamic symbol table", Obj.getFileName());
    return {};
  }
  return E->getDynamicSymbolIterators();
}

[DiggerLin]

llvm/test/Object/nm-universal-binary.test

37–38

thanks a lot for your detail explanation.

llvm/tools/llvm-nm/llvm-nm.cpp

1916

as your suggestion, the function getDynamicSyms() will be call twice .

1. One is in the function

getSymbolNamesFromObject

2 . the other is in hasSymbols() which will be called in the function
dumpSymbolNamesFromObject()

if there is error.

if (!E) {

  error("File format has no dynamic symbol table", Obj.getFileName());
  return {};
}

the error info will be print out twice.

if I keep the code in the
as

static bool getSymbolNamesFromObject(SymbolicFile &Obj, bool &IsSymbolsEmpty) {
  auto Symbols = Obj.symbols();
  std::vector<VersionEntry> SymbolVersions;
  if (DynamicSyms) {
    const auto *E = dyn_cast<ELFObjectFileBase>(&Obj);
    if (!E) {
      error("File format has no dynamic symbol table", Obj.getFileName());
      return false;
    }
    Symbols = E->getDynamicSymbolIterators();

there is no advantage of adding a two helper function.

I do not think adding a new parameter "bool &IsSymbolsEmpty" is a big problem.

[jhenderson]

llvm/tools/llvm-nm/llvm-nm.cpp

1752–1754

Add a comment to the function explaining the meaning of the return value.

1916

the error info will be print out twice.

Fair point. My gut says the problem is that this error is reported at too low a level in the code, and that the right way to fix this would be to send errors back up the stack via Expected and Error, with it being reported near the top-level of the code, such that an error effectively aborts dumping of one object, but not all objects. I acknowledge that this is yet another change, although I think the cost of it wouuld be fairly small. If you don't want to go that far, I understand (I'd offer to do it myself, but don't really have the time). However, in that case, I think we still need a different solution, because the new input argument is yuck. A partial implementation of the above (using Expected/Error) might be to have getDynamicSyms return an Expected, and then in getSymbolNamesFromObject, if the Expected is in a failed state, call error there, while in hasSymbols simply do consumeError. If you do this, I'd add a TODO: comment in the latter case saying the error is already reported elsewhere, but really it should be propagated up the stack, to avoid duplicate errors.

I do not think adding a new parameter "bool &IsSymbolsEmpty" is a big problem.

Sorry, but I disagree. (Related aside: I'm not really all that happy about the return type change in this patch either, but I understand it's a step towards a later goal). With your change, you're making this function do at least four different things, 1) get all the object's symbols 2) filter out the ones you care about, 3) tell the caller whether the symbols could be loaded from the object, and 4) tell the caller whether the object has any symbols. Yes, some of these were there before, but adding more to that set is not ideal, when there are alternative approaches. In clean code, it is generally preferable for each function to try to do only one thing: this makes it easier to understand what any given function is doing, making it more maintanable and easier to use by other developers in the future.

Additionally, the use of a reference argument purely for setting a secondary return type is not great for various reasons: 1) it forces calling code to provide an input boolean that is set, even if the caller doesn't care about the parameter; 2) related to 1), the caller has to define a boolean variable before the function is called, preventing the function from being used cleanly within e.g. if clauses (the boolean input argument has to be defined before the if check); 3) it raises a question of "why is one returned and the other an argument?"; 4) it raises another question: "does my boolean need to be initialised or not?"

Another side-advantage with the extra helper function is that this reduces the size of getSymbolNamesFromObject. Again, the simpler a function is, the easier it is to understand.

[DiggerLin]

llvm/tools/llvm-nm/llvm-nm.cpp

1886

I use cantFail because if there is a error , it will happen at function getSymbolNamesFromObject first. it will not go to hasSymbols() which be called in the function dumpSymbolNamesFromObject. using cantFail let code simple here.

1916

Another side-advantage with the extra helper function is that this reduces the size of getSymbolNamesFromObject. Again, the simpler a function is, the easier it is to understand.

the helper function getDynamicSyms(SymbolicFile &Obj) can not be used in the getSymbolNamesFromObject.

for the following code still need E

if (Expected<std::vector<VersionEntry>> VersionsOrErr =
           E->readDynsymVersions())
     SymbolVersions = std::move(*VersionsOrErr);
   else
     WithColor::warning(errs(), ToolName)
         << "unable to read symbol versions: "
         << toString(VersionsOrErr.takeError()) << "\n";
 }

but the function getDynamicSyms

return E->getDynamicSymbolIterators()

So I keep the function getSymbolNamesFromObject without getDynamicSyms

[jhenderson]

Something very strange just happened and half my review comments were lost in my attempt to post. Sorry if anything ends up getting duplicated.

llvm/tools/llvm-nm/llvm-nm.cpp

1752–1754

1758–1775

Moving the conversation from below: I think you should still use getDynamicSyms here and just use another cast call to get the ELF object again, as in the inline edit. It's reasonably straightforward, and removes the duplicated error.

1784

Are you sure this should be return false? (I don't know either way, but we should preserve behaviour).

1886

I'm not particularly comfortable with this, because it would be trivially easy for someone in the future to add another hasSymbols call site, without having first called dumpSymbolNamesFromObject. I think it would be better to return an Expected<bool> from this function, and then use cantFail at the relevant call site(s), possibly with a comment at those sites explaining why the function cannot fail.

[DiggerLin]

llvm/tools/llvm-nm/llvm-nm.cpp

1784

the patch almost is NFC patch, I do not think it is a good idea to change the preserve
behaviour.

[jhenderson]

LGTM, with one question.

llvm/tools/llvm-nm/llvm-nm.cpp

1784

I said we should preserve behaviour. My question was: does this?

[DiggerLin]

llvm/tools/llvm-nm/llvm-nm.cpp

1784

not sure your question, would you like to explain more detail?

according to the comment

// If a "-s segname sectname" option was specified and this is a Mach-O
 // file get the section number for that section in this object file.
 unsigned int Nsect = 0;
 MachOObjectFile *MachO = dyn_cast<MachOObjectFile>(&Obj);
 if (!SegSect.empty() && MachO) {
   Nsect = getNsectForSegSect(MachO);
   // If this section is not in the object file no symbols are printed.
   if (Nsect == 0)
     return false;
 }

if the user specific a section name from llvm-nm command line
"-s segname sectname", but the section not in the Mach-O object, it can not get the symbol. it should return the false.

do I answer your question ?

[jhenderson]

llvm/tools/llvm-nm/llvm-nm.cpp

1784

Not exactly. Previously, the behaviour of dumpSymbolNamesFromObject was to not print anything if the section was not found. Importantly, it also skipped the no symbols printing and also didn't print an error.

This function's comment says that return false indicates an error. As not finding a section previously didn't report an error, return false here didn't quite line up with that comment. Consequently, I wondered whether it was correct to return false, and the important factor was whether this caused a behaviour change or not. Having gone through this patch again, I now think return false is correct amd preserves the existing behaviour.

llvm/test/Object/nm-universal-binary.test

 
 CHECK-OBJ-x86_64: 0000000100000000 T __mh_execute_header
 CHECK-OBJ-x86_64: 0000000100000f60 T _main
 CHECK-OBJ-x86_64:                  U dyld_stub_binder
 
 CHECK-OBJ-armv7m: error: file: {{.*}} does not contain architecture
 
 CHECK-OBJ-foobar: error: for the --arch option: Unknown architecture named 'foobar'
 
-CHECK-AR: macho-universal-archive.x86_64.i386(hello.o) (for architecture x86_64):
-CHECK-AR: 0000000000000068 s EH_frame0
-CHECK-AR: 000000000000003b s L_.str
-CHECK-AR: 0000000000000000 T _main
-CHECK-AR: 0000000000000080 S _main.eh
-CHECK-AR:                  U _printf
+CHECK-AR:       {{^$}}
+CHECK-AR-NEXT:  macho-universal-archive.x86_64.i386(hello.o) (for architecture x86_64):
+CHECK-AR:       0000000000000068 s EH_frame0
+CHECK-AR:       000000000000003b s L_.str
+CHECK-AR:       0000000000000000 T _main
+CHECK-AR:       0000000000000080 S _main.eh
+CHECK-AR:                        U _printf
 CHECK-AR: macho-universal-archive.x86_64.i386(foo.o) (for architecture i386):
 CHECK-AR: 00000008 D _bar
 CHECK-AR: 00000000 T _foo
 
-CHECK-64-AR: macho-universal64-archive.x86_64.i386(foo.o) (for architecture i386):
-CHECK-64-AR: 00000008 D _bar
-CHECK-64-AR: 00000000 T _foo
-CHECK-64-AR: macho-universal64-archive.x86_64.i386(hello.o) (for architecture x86_64):
-CHECK-64-AR: 0000000000000068 s EH_frame0
-CHECK-64-AR: 000000000000003b s L_.str
-CHECK-64-AR: 0000000000000000 T _main
-CHECK-64-AR: 0000000000000080 S _main.eh
-CHECK-64-AR:                  U _printf
+CHECK-64-AR:      {{^$}}
+CHECK-64-AR-NEXT: macho-universal64-archive.x86_64.i386(foo.o) (for architecture i386):
+CHECK-64-AR:      00000008 D _bar
+CHECK-64-AR:      00000000 T _foo
+CHECK-64-AR:      macho-universal64-archive.x86_64.i386(hello.o) (for architecture x86_64):
+CHECK-64-AR:      0000000000000068 s EH_frame0
+CHECK-64-AR:      000000000000003b s L_.str
+CHECK-64-AR:      0000000000000000 T _main
+CHECK-64-AR:      0000000000000080 S _main.eh
+CHECK-64-AR:                       U _printf
 
 CHECK-AR-i386: macho-universal-archive.x86_64.i386(foo.o):
 CHECK-AR-i386: 00000008 D _bar
 CHECK-AR-i386: 00000000 T _foo
 
 CHECK-AR-o: (for architecture x86_64):{{.*}}/macho-universal-archive.x86_64.i386:hello.o: 0000000000000068 s EH_frame0
 CHECK-AR-o: (for architecture x86_64):{{.*}}/macho-universal-archive.x86_64.i386:hello.o: 000000000000003b s L_.str
 CHECK-AR-o: (for architecture x86_64):{{.*}}/macho-universal-archive.x86_64.i386:hello.o: 0000000000000000 T _main
 CHECK-AR-o: (for architecture x86_64):{{.*}}/macho-universal-archive.x86_64.i386:hello.o: 0000000000000080 S _main.eh
 CHECK-AR-o: (for architecture x86_64):{{.*}}/macho-universal-archive.x86_64.i386:hello.o:                  U _printf
 CHECK-AR-o: (for architecture i386):{{.*}}/macho-universal-archive.x86_64.i386:foo.o: 00000008 D _bar
 CHECK-AR-o: (for architecture i386):{{.*}}/macho-universal-archive.x86_64.i386:foo.o: 00000000 T _foo

llvm/tools/llvm-nm/llvm-nm.cpp

static char getNMSectionTagAndName(SymbolicFile &Obj, basic_symbol_iterator I,
   if (!(Symflags & object::SymbolRef::SF_Global))
     return Ret;
 
   return toupper(Ret);
 }
 
 // getNsectForSegSect() is used to implement the Mach-O "-s segname sectname"
 // option to dump only those symbols from that section in a Mach-O file.
-// It is called once for each Mach-O file from dumpSymbolNamesFromObject()
+// It is called once for each Mach-O file from getSymbolNamesFromObject()
 // to get the section number for that named section from the command line
 // arguments. It returns the section number for that section in the Mach-O
 // file or zero it is not present.
 static unsigned getNsectForSegSect(MachOObjectFile *Obj) {
   unsigned Nsect = 1;
   for (auto &S : Obj->sections()) {
     DataRefImpl Ref = S.getRawDataRefImpl();
     StringRef SectionName;
     if (Expected<StringRef> NameOrErr = Obj->getSectionName(Ref))
       SectionName = *NameOrErr;
     StringRef SegmentName = Obj->getSectionFinalSegmentName(Ref);
     if (SegmentName == SegSect[0] && SectionName == SegSect[1])
       return Nsect;
     Nsect++;
   }
   return 0;
 }
 
 // getNsectInMachO() is used to implement the Mach-O "-s segname sectname"
 // option to dump only those symbols from that section in a Mach-O file.
 // It is called once for each symbol in a Mach-O file from
-// dumpSymbolNamesFromObject() and returns the section number for that symbol
+// getSymbolNamesFromObject() and returns the section number for that symbol
 // if it is in a section, else it returns 0.
 static unsigned getNsectInMachO(MachOObjectFile &Obj, BasicSymbolRef Sym) {
   DataRefImpl Symb = Sym.getRawDataRefImpl();
   if (Obj.is64Bit()) {
     MachO::nlist_64 STE = Obj.getSymbol64TableEntry(Symb);
     return (STE.n_type & MachO::N_TYPE) == MachO::N_SECT ? STE.n_sect : 0;
   }
   MachO::nlist STE = Obj.getSymbolTableEntry(Symb);
if (HasVisibilityAttr) {
       else if ((SymType & XCOFF::VISIBILITY_MASK) == XCOFF::SYM_V_EXPORTED)
         S.Visibility = "export";
     }
     if (S.initializeFlags(*XCOFFObj))
       SymbolList.push_back(S);
   }
 }
 
-static void dumpSymbolNamesFromObject(SymbolicFile &Obj, bool printName,
-                                      StringRef ArchiveName = {},
-                                      StringRef ArchitectureName = {}) {
-  if (!shouldDump(Obj))
-    return;
-
-  if (ExportSymbols && Obj.isXCOFF()) {
-    XCOFFObjectFile *XCOFFObj = cast<XCOFFObjectFile>(&Obj);
-    getXCOFFExports(XCOFFObj, ArchiveName);
-    return;
-  }
+static Expected<SymbolicFile::basic_symbol_iterator_range>
+getDynamicSyms(SymbolicFile &Obj) {
+  const auto *E = dyn_cast<ELFObjectFileBase>(&Obj);
+  if (!E)
+    return createError("File format has no dynamic symbol table");
+  return E->getDynamicSymbolIterators();
 
+// Returns false if an error is encounted or true otherwise.
+static bool getSymbolNamesFromObject(SymbolicFile &Obj) {
   auto Symbols = Obj.symbols();
   std::vector<VersionEntry> SymbolVersions;
+
   if (DynamicSyms) {
-    const auto *E = dyn_cast<ELFObjectFileBase>(&Obj);
-    if (!E) {
-      error("File format has no dynamic symbol table", Obj.getFileName());
-      return;
+    Expected<SymbolicFile::basic_symbol_iterator_range> SymbolsOrErr =
+        getDynamicSyms(Obj);
+    if (!SymbolsOrErr) {
+      error(SymbolsOrErr.takeError(), Obj.getFileName());
+      return false;
     }
-    Symbols = E->getDynamicSymbolIterators();
-
+    Symbols = *SymbolsOrErr;
+    if (const auto *E = dyn_cast<ELFObjectFileBase>(&Obj)) {
       if (Expected<std::vector<VersionEntry>> VersionsOrErr =
               E->readDynsymVersions())
         SymbolVersions = std::move(*VersionsOrErr);
       else
         WithColor::warning(errs(), ToolName)
             << "unable to read symbol versions: "
             << toString(VersionsOrErr.takeError()) << "\n";
     }
-
+  }
   // If a "-s segname sectname" option was specified and this is a Mach-O
   // file get the section number for that section in this object file.
   unsigned int Nsect = 0;
   MachOObjectFile *MachO = dyn_cast<MachOObjectFile>(&Obj);
   if (!SegSect.empty() && MachO) {
     Nsect = getNsectForSegSect(MachO);
     // If this section is not in the object file no symbols are printed.
     if (Nsect == 0)
-      return;
+      return false;
   }
 
   if (!(MachO && DyldInfoOnly)) {
     size_t I = -1;
     for (BasicSymbolRef Sym : Symbols) {
       ++I;
       Expected<uint32_t> SymFlagsOrErr = Sym.getFlags();
       if (!SymFlagsOrErr) {
         error(SymFlagsOrErr.takeError(), Obj.getFileName());
-        return;
+        return false;
       }
 
       // Don't drop format specifc symbols for ARM and AArch64 ELF targets, they
       // are used to repesent mapping symbols and needed to honor the
       // --special-syms option.
       auto *ELFObj = dyn_cast<ELFObjectFileBase>(&Obj);
       if ((!ELFObj || (ELFObj->getEMachine() != ELF::EM_ARM &&
                        ELFObj->getEMachine() != ELF::EM_AARCH64)) &&
static bool getSymbolNamesFromObject(SymbolicFile &Obj) {
   // If this is a Mach-O file where the nlist symbol table is out of sync
   // with the dyld export trie then look through exports and fake up symbols
   // for the ones that are missing (also done with the -add-dyldinfo flag).
   // This is needed if strip(1) -T is run on a binary containing swift
   // language symbols for example.  The option -only-dyldinfo will fake up
   // all symbols from the dyld export trie as well as the bind info.
   if (MachO && !NoDyldInfo)
     dumpSymbolsFromDLInfoMachO(*MachO);
+  return true;
+}
 
-  if (ExportSymbols)
+static void printObjectLabel(bool PrintArchiveName, StringRef ArchiveName,
+                             StringRef ArchitectureName,
+                             StringRef ObjectFileName) {
+  outs() << "\n";
+  if (ArchiveName.empty() || !PrintArchiveName)
+    outs() << ObjectFileName;
+  else
+    outs() << ArchiveName << "(" << ObjectFileName << ")";
+  if (!ArchitectureName.empty())
+    outs() << " (for architecture " << ArchitectureName << ")";
+  outs() << ":\n";
+}
+
+static Expected<bool> hasSymbols(SymbolicFile &Obj) {
+  if (DynamicSyms) {
+    Expected<SymbolicFile::basic_symbol_iterator_range> DynamicSymsOrErr =
+        getDynamicSyms(Obj);
+    if (!DynamicSymsOrErr)
+      return DynamicSymsOrErr.takeError();
+    return !DynamicSymsOrErr->empty();
+  }
+  return !Obj.symbols().empty();
+}
+
+static void dumpSymbolNamesFromObject(SymbolicFile &Obj, bool PrintSymbolObject,
+                                      bool PrintObjectLabel,
+                                      StringRef ArchiveName = {},
+                                      StringRef ArchitectureName = {},
+                                      StringRef ObjectName = {},
+                                      bool PrintArchiveName = true) {
+  if (!shouldDump(Obj))
     return;
 
+  if (ExportSymbols && Obj.isXCOFF()) {
+    XCOFFObjectFile *XCOFFObj = cast<XCOFFObjectFile>(&Obj);
+    getXCOFFExports(XCOFFObj, ArchiveName);
+    return;
+  }
+
+  if (PrintObjectLabel && !ExportSymbols)
+    printObjectLabel(PrintArchiveName, ArchiveName, ArchitectureName,
+                     ObjectName.empty() ? Obj.getFileName() : ObjectName);
+  if (!getSymbolNamesFromObject(Obj) || ExportSymbols)
+    return;
   CurrentFilename = Obj.getFileName();
 
-  if (Symbols.empty() && SymbolList.empty() && !Quiet) {
+  // If there is an error in hasSymbols(), the error should be encountered in
+  // function getSymbolNamesFromObject first.
+  if (!cantFail(hasSymbols(Obj)) && SymbolList.empty() && !Quiet) {
     writeFileName(errs(), ArchiveName, ArchitectureName);
     errs() << "no symbols\n";
   }
 
   sortSymbolList();
-  printSymbolList(Obj, printName, ArchiveName, ArchitectureName);
+  printSymbolList(Obj, PrintSymbolObject, ArchiveName, ArchitectureName);
 }
 
 // checkMachOAndArchFlags() checks to see if the SymbolicFile is a Mach-O file
 // and if it is and there is a list of architecture flags is specified then
 // check to make sure this Mach-O file is one of those architectures or all
 // architectures was specificed.  If not then an error is generated and this
 // routine returns false.  Else it returns true.
 static bool checkMachOAndArchFlags(SymbolicFile *O, std::string &Filename) {
for (auto &C : A->children(Err)) {
     if (SymbolicFile *O = dyn_cast<SymbolicFile>(&*ChildOrErr.get())) {
       if (!MachOPrintSizeWarning && PrintSize && isa<MachOObjectFile>(O)) {
         WithColor::warning(errs(), ToolName)
             << "sizes with -print-size for Mach-O files are always zero.\n";
         MachOPrintSizeWarning = true;
       }
       if (!checkMachOAndArchFlags(O, Filename))
         return;
-      if (!PrintFileName && shouldDump(*O) && !ExportSymbols) {
-        outs() << "\n";
-        if (isa<MachOObjectFile>(O)) {
-          outs() << Filename << "(" << O->getFileName() << ")";
-        } else
-          outs() << O->getFileName();
-        outs() << ":\n";
-      }
-      dumpSymbolNamesFromObject(*O, false, Filename);
+      dumpSymbolNamesFromObject(*O, /*PrintSymbolObject=*/false, !PrintFileName,
+                                Filename,
+                                /*ArchitectureName=*/{}, O->getFileName(),
+                                /*PrintArchiveName=*/false);
     }
   }
   if (Err)
     error(std::move(Err), A->getFileName());
 }
 
 static void dumpMachOUniversalBinaryMatchArchFlags(MachOUniversalBinary *UB,
                                                    std::string &Filename,
for (MachOUniversalBinary::object_iterator I = UB->begin_objects(),
         ArchFound = true;
         Expected<std::unique_ptr<ObjectFile>> ObjOrErr = I->getAsObjectFile();
         std::string ArchiveName;
         std::string ArchitectureName;
         ArchiveName.clear();
         ArchitectureName.clear();
         if (ObjOrErr) {
           ObjectFile &Obj = *ObjOrErr.get();
-          if (ArchFlags.size() > 1) {
-            if (PrintFileName)
-              ArchitectureName = I->getArchFlagName();
-            else
-              outs() << "\n"
-                     << Obj.getFileName() << " (for architecture "
-                     << I->getArchFlagName() << ")"
-                     << ":\n";
-          }
-          dumpSymbolNamesFromObject(Obj, false, ArchiveName, ArchitectureName);
+          if (ArchFlags.size() > 1)
+          dumpSymbolNamesFromObject(Obj, /*PrintSymbolObject=*/false,
+                                    (ArchFlags.size() > 1) && !PrintFileName,
+                                    ArchiveName, ArchitectureName);
         } else if (auto E =
                        isNotObjectErrorInvalidFileType(ObjOrErr.takeError())) {
           error(std::move(E), Filename,
                 ArchFlags.size() > 1 ? StringRef(I->getArchFlagName())
                                      : StringRef());
           continue;
         } else if (Expected<std::unique_ptr<Archive>> AOrErr =
                        I->getAsArchive()) {
           std::unique_ptr<Archive> &A = *AOrErr;
           Error Err = Error::success();
           for (auto &C : A->children(Err)) {
             Expected<std::unique_ptr<Binary>> ChildOrErr =
                 C.getAsBinary(ContextPtr);
             if (!ChildOrErr) {
               if (auto E =
                       isNotObjectErrorInvalidFileType(ChildOrErr.takeError())) {
                 error(std::move(E), Filename, C,
                       ArchFlags.size() > 1 ? StringRef(I->getArchFlagName())
                                            : StringRef());
               }
               continue;
             }
             if (SymbolicFile *O = dyn_cast<SymbolicFile>(&*ChildOrErr.get())) {
-              if (PrintFileName) {
-                ArchiveName = std::string(A->getFileName());
-                if (ArchFlags.size() > 1)
-                  ArchitectureName = I->getArchFlagName();
-              } else {
-                outs() << "\n" << A->getFileName();
-                outs() << "(" << O->getFileName() << ")";
-                if (ArchFlags.size() > 1) {
-                  outs() << " (for architecture " << I->getArchFlagName()
-                         << ")";
-                }
-                outs() << ":\n";
-              }
-              dumpSymbolNamesFromObject(*O, false, ArchiveName,
+              dumpSymbolNamesFromObject(*O, /*PrintSymbolObject=*/false,
+                                        !PrintFileName, ArchiveName,
                                         ArchitectureName);
             }
           }
           if (Err)
             error(std::move(Err), A->getFileName());
         } else {
           consumeError(AOrErr.takeError());
           error(Filename + " for architecture " +
                     StringRef(I->getArchFlagName()) +
                     " is not a Mach-O file or an archive file",
                 "Mach-O universal file");
         }
       }
static bool dumpMachOUniversalBinaryMatchHost(MachOUniversalBinary *UB,
   for (MachOUniversalBinary::object_iterator I = UB->begin_objects(),
                                              E = UB->end_objects();
        I != E; ++I) {
     if (HostArchName == I->getArchFlagName()) {
       Expected<std::unique_ptr<ObjectFile>> ObjOrErr = I->getAsObjectFile();
       std::string ArchiveName;
       if (ObjOrErr) {
         ObjectFile &Obj = *ObjOrErr.get();
-        dumpSymbolNamesFromObject(Obj, false);
+        dumpSymbolNamesFromObject(Obj, /*PrintSymbolObject=*/false,
+                                  /*PrintObjectLabel=*/false);
       } else if (auto E = isNotObjectErrorInvalidFileType(ObjOrErr.takeError()))
         error(std::move(E), Filename);
       else if (Expected<std::unique_ptr<Archive>> AOrErr = I->getAsArchive()) {
         std::unique_ptr<Archive> &A = *AOrErr;
         Error Err = Error::success();
         for (auto &C : A->children(Err)) {
           Expected<std::unique_ptr<Binary>> ChildOrErr =
               C.getAsBinary(ContextPtr);
           if (!ChildOrErr) {
             if (auto E =
                     isNotObjectErrorInvalidFileType(ChildOrErr.takeError()))
               error(std::move(E), Filename, C);
             continue;
           }
           if (SymbolicFile *O = dyn_cast<SymbolicFile>(&*ChildOrErr.get())) {
-            if (PrintFileName)
-              ArchiveName = std::string(A->getFileName());
-            else
-              outs() << "\n"
-                     << A->getFileName() << "(" << O->getFileName() << ")"
-                     << ":\n";
-            dumpSymbolNamesFromObject(*O, false, ArchiveName);
+            dumpSymbolNamesFromObject(*O, /*PrintSymbolObject=*/false,
+                                      !PrintFileName, ArchiveName);
           }
         }
         if (Err)
           error(std::move(Err), A->getFileName());
       } else {
         consumeError(AOrErr.takeError());
         error(Filename + " for architecture " +
                   StringRef(I->getArchFlagName()) +
static void dumpMachOUniversalBinaryArchAll(MachOUniversalBinary *UB,
   for (const MachOUniversalBinary::ObjectForArch &O : UB->objects()) {
     Expected<std::unique_ptr<ObjectFile>> ObjOrErr = O.getAsObjectFile();
     std::string ArchiveName;
     std::string ArchitectureName;
     ArchiveName.clear();
     ArchitectureName.clear();
     if (ObjOrErr) {
       ObjectFile &Obj = *ObjOrErr.get();
-      if (PrintFileName) {
-        if (isa<MachOObjectFile>(Obj) && moreThanOneArch)
-          ArchitectureName = O.getArchFlagName();
-      } else {
-        if (moreThanOneArch)
-          outs() << "\n";
-        outs() << Obj.getFileName();
-        if (isa<MachOObjectFile>(Obj) && moreThanOneArch)
-          outs() << " (for architecture " << O.getArchFlagName() << ")";
-        outs() << ":\n";
-      }
-      dumpSymbolNamesFromObject(Obj, false, ArchiveName, ArchitectureName);
+      dumpSymbolNamesFromObject(Obj, /*PrintSymbolObject=*/false,
+                                !PrintFileName, ArchiveName, ArchitectureName);
     } else if (auto E = isNotObjectErrorInvalidFileType(ObjOrErr.takeError())) {
       error(std::move(E), Filename,
             moreThanOneArch ? StringRef(O.getArchFlagName()) : StringRef());
       continue;
     } else if (Expected<std::unique_ptr<Archive>> AOrErr = O.getAsArchive()) {
       std::unique_ptr<Archive> &A = *AOrErr;
       Error Err = Error::success();
       for (auto &C : A->children(Err)) {
         Expected<std::unique_ptr<Binary>> ChildOrErr =
             C.getAsBinary(ContextPtr);
         if (!ChildOrErr) {
           if (auto E = isNotObjectErrorInvalidFileType(ChildOrErr.takeError()))
             error(std::move(E), Filename, C,
                   moreThanOneArch ? StringRef(ArchitectureName) : StringRef());
           continue;
         }
         if (SymbolicFile *F = dyn_cast<SymbolicFile>(&*ChildOrErr.get())) {
-          if (PrintFileName) {
-            ArchiveName = std::string(A->getFileName());
-            if (isa<MachOObjectFile>(F) && moreThanOneArch)
-              ArchitectureName = O.getArchFlagName();
-          } else {
-            outs() << "\n" << A->getFileName();
-            if (isa<MachOObjectFile>(F)) {
-              outs() << "(" << F->getFileName() << ")";
-              if (moreThanOneArch)
-                outs() << " (for architecture " << O.getArchFlagName() << ")";
-            } else
-              outs() << ":" << F->getFileName();
-            outs() << ":\n";
-          }
-          dumpSymbolNamesFromObject(*F, false, ArchiveName, ArchitectureName);
+          dumpSymbolNamesFromObject(*F, /*PrintSymbolObject=*/false,
+                                    !PrintFileName, ArchiveName,
+                                    ArchitectureName);
         }
       }
       if (Err)
         error(std::move(Err), A->getFileName());
     } else {
       consumeError(AOrErr.takeError());
       error(Filename + " for architecture " + StringRef(O.getArchFlagName()) +
                 " is not a Mach-O file or an archive file",
static void dumpTapiUniversal(TapiUniversal *TU, std::string &Filename) {
   for (const TapiUniversal::ObjectForArch &I : TU->objects()) {
     StringRef ArchName = I.getArchFlagName();
     const bool ShowArch =
         ArchFlags.empty() || llvm::is_contained(ArchFlags, ArchName);
     if (!ShowArch)
       continue;
     if (!AddInlinedInfo && !I.isTopLevelLib())
       continue;
-    if (auto ObjOrErr = I.getAsObjectFile()) {
-      outs() << "\n"
-             << I.getInstallName() << " (for architecture " << ArchName << ")"
-             << ":\n";
-      dumpSymbolNamesFromObject(*ObjOrErr.get(), false, {}, ArchName);
-    } else if (Error E =
-                   isNotObjectErrorInvalidFileType(ObjOrErr.takeError())) {
+    if (auto ObjOrErr = I.getAsObjectFile())
+      dumpSymbolNamesFromObject(*ObjOrErr.get(), /*PrintSymbolObject=*/false,
+                                /*PrintObjectLabel=*/true,
+                                /*ArchiveName=*/{}, ArchName,
+                                I.getInstallName());
+    else if (Error E = isNotObjectErrorInvalidFileType(ObjOrErr.takeError())) {
       error(std::move(E), Filename, ArchName);
     }
   }
 }
 
 static void dumpSymbolicFile(SymbolicFile *O, std::string &Filename) {
   if (!MachOPrintSizeWarning && PrintSize && isa<MachOObjectFile>(O)) {
     WithColor::warning(errs(), ToolName)
         << "sizes with --print-size for Mach-O files are always zero.\n";
     MachOPrintSizeWarning = true;
   }
   if (!checkMachOAndArchFlags(O, Filename))
     return;
-  dumpSymbolNamesFromObject(*O, true);
+  dumpSymbolNamesFromObject(*O, /*PrintSymbolObject=*/true,
+                            /*PrintObjectLabel=*/false);
 }
 
 static void dumpSymbolNamesFromFile(std::string &Filename) {
   ErrorOr<std::unique_ptr<MemoryBuffer>> BufferOrErr =
       MemoryBuffer::getFileOrSTDIN(Filename);
   if (error(BufferOrErr.getError(), Filename))
     return;