[llvm-objdump] Use symbol index+symbol name + storage mapping class as label for -D

[DiggerLin]

Summary

For the llvm-objdump -D, the symbol name is used as a label in the disassembly for the specific address (when a symbol address is equal to the virtual address in the dump).

In XCOFF, multiple symbols may have the same name, being differentiated by their storage mapping class. It is helpful to print the QualName and not just the name when forming the output label for a csect symbol. The symbol index further removes any ambiguity caused by duplicate names.

To maintain compatibility with the binutils objdump, the XCOFF-specific --symbol-description option is added to enable the enhanced format.

[DiggerLin]

address comment

[jhenderson]

llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h

51

Are you talking about XCOFF here? For ELF, it is possible for this to happen. Take two source files as follows:

// bar.c
static int bar() { return 42; }
int foo();
int _start() { return bar() + foo(); }

// foo.c
static int bar() { return 42; }
int foo() { return bar(); }

Compile using -ffunction-sections, and link them together using --icf=all, and you end up with two local symbols called bar, with the same address and type.

54

we do not want to change the elf related test case .so we use use Addr, Name, Type to compare.

As noted above, two ELF symbols can be identical, but be distinct symbols. Do you care about this situation?

when you look into the Optional class data structure, I think we compare storage mapping class actually.

But you don't compare using the XCOFFSymInfo member. I don't think you can assume anything about the layout of the union, so cannot assume that comparing using Type will work if it has been initialised the other way.

llvm/lib/BinaryFormat/XCOFF.cpp

17–40

Please don't use report_fatal_error if at all possible. This produces error messages that imply that it is an internal error (something similar to an assert, but works if assertions are disabled). You can change getMappingClassString to return either an empty StringRef() or an Expected<StringRef> and report an error properly.

If somebody has defined their SMC field to not be one of the recognised values, you want the dumping code to handle it nicely, without hitting report_fatal_error or a crash...

Please also add a test case for this situation.

llvm/test/tools/llvm-objdump/xcoff-disassemble-all.test

5

For ELF, --disassemble disassembles all sections with the SHF_EXECINSTR flag, i.e. things like .text etc, whereas --disassemble-all disassembles non executable sections too. I suppose it is okay to add it here. However, the biggest benefit would be to combine the two sets of CHECKs using --check-prefixes:

# RUN: <regular -D> | FileCheck %s --check-prefixes=COMMON,PLAIN
# RUN: <with --symbol-description> | FileCheck %s --check-prefixes=COMMON,DESC

# COMMON:      Inputs/xcoff-section-headers.o:  file format aixcoff-rs6000
# COMMON:      Disassembly of section .text:
# PLAIN:       00000000 .text:
# DESC:        00000000 (idx: 4) .text:
# COMMON-NEXT:         0: 80 62 00 04                    lwz 3, 4(2)
...

You haven't addressed my indentation comment either.

llvm/tools/llvm-objdump/XCOFFDump.cpp

65

StringRef is intended to be used instead of const std::string & as it supports both const char * and const std::string & with less heap allocation. See the Programmer's Manual.

llvm/tools/llvm-objdump/llvm-objdump.cpp

149

This still hasn't been addressed. Do you need any assistance?

149–150

This hasn't been properly addressed. ',' -> '.' before the "This".

Also, please remove the trailing full stop in the help description, to match other options.

1176

I think you can use None instead of Optional<uint32_t>().

llvm/tools/llvm-objdump/llvm-objdump.h

149–156

Those should be refactored into separate files too. Let's not compound bad form by blindly following what was done before.

[DiggerLin]

llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h

51

for xcoff disassembly, if there are symbol have the same address. I think we do not care about which symbol to show when disassembly.

54

please checked the patch. https://reviews.llvm.org/D74240

it compare the std::tuple<uint64_t, StringRef, uint8_t> and sort the symbol in the SectionSymbolsTy

the default less function of std:tuple is tie uint64_t, StringRef, uint8_t together and compare. we do not want to change the no xcoff test case for llvm-objdum -D, we keep to compare P1.Addr, P1.Name, P1.Type

llvm/lib/BinaryFormat/XCOFF.cpp

17–40

There are several place to use getMappingClassString in the llvm(not only the patch). I change the interface of the function, We need to change several place(not related to this patch) in llvm. I will return a StringRef("Unknown"); here.

llvm/test/tools/llvm-objdump/xcoff-disassemble-all.test

5

thanks let me know the usage. I will change it.

llvm/tools/llvm-objdump/llvm-objdump.cpp

1176

thanks

llvm/tools/llvm-objdump/llvm-objdump.h

149–156

I agree with you. But I think we maybe need to create separate NFC patch to address the problem.

[DiggerLin]

address comment

[DiggerLin]

add option "symbol-description" information for llvm-objdump.rst

[jhenderson]

llvm/docs/CommandGuide/llvm-objdump.rst

330 ↗

(On Diff #249614)

I don't think you really need the second sentence. I think it's implied by the first saying "disassembly". However, if you still want it, see, e.g. --x86-asm-syntax for an example of how to reference options in rst, and remove the references to the short options.

llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h

54

Right, this is fine for non-XCOFF, but in an XCOFF object, you CANNOT reference P1.Type as it is not initialised.

llvm/lib/BinaryFormat/XCOFF.cpp

43

Test-case using this? More generally, you should have a test case for all the supported SMC values probably (i.e. showing how they're printed), rather than a subset. You could use a unit test instead of a lit test if it's not possible to hit all cases at this point, or consider a yaml2obj extension.

llvm/test/tools/llvm-objdump/xcoff-disassemble-all.test

1–6

Indentation comment STILL hasn't been addressed:

# RUN: llvm-objdump -D %p/Inputs/xcoff-section-headers.o | \
# RUN:   FileCheck --check-prefixes=COMMON,PLAIN %s

# RUN: llvm-objdump -D --symbol-description %p/Inputs/xcoff-section-headers.o | \
# RUN:   FileCheck --check-prefixes=COMMON,DESC %s

llvm/tools/llvm-objdump/XCOFFDump.cpp

53

No need to take StringRef as const &. Just use StringRef SymbolName.

llvm/tools/llvm-objdump/llvm-objdump.cpp

149–150

Also, please remove the trailing full stop in the help description, to match other options.

This hasn't been addressed. Please remember to address ALL points in inline comments.

llvm/tools/llvm-objdump/llvm-objdump.h

149–156

We should do XCOFF one in this patch I think, as it is new code. Others can be addressed later.

[DiggerLin]

address comment

llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h

54

in xcoff , we only sort the symbols base on the address and name, that means we do not care about the order of the symbols which has the same address in xcoff the XCOFFSymbolInfo will cast to P1.Type here and compare.

llvm/lib/BinaryFormat/XCOFF.cpp

43

llvm yaml2obj do not support xcoff currently. We can add a test case for this when the llvm yaml2obj support xcoff.

llvm/tools/llvm-objdump/llvm-objdump.h

149–156

assume that I added a new header file llvm/tools/llvm-objdump/xcoffobjdump.h.
and then the header files will be included llvm/tools/llvm-objdump/llvm-objdump.cpp

as

#include "llvm-objdump.h"
#include "coffobjdump.h"
#include "elfpbjdump.h"
#include "xcoffobjdump.h"

I do not think it is a good idea.

[jhenderson]

llvm/docs/CommandGuide/llvm-objdump.rst

324 ↗

(On Diff #250639)

You probably don't want the "-O" in XCOFF...

329 ↗

(On Diff #250639)

for disassembly -> to disassembly output

llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h

54

I'm pretty sure referencing an unitialised part of a union (i.e. the Type member) is undefined behaviour in C++. See https://stackoverflow.com/a/11996970. Also "It's undefined behavior to read from the member of the union that wasn't most recently written" (see https://en.cppreference.com/w/cpp/language/union).

It doesn't matter if you have set the other member of the union or whether you care about that part of the comparison etc. So, if this code is touched by XCOFF, what you are doing is illegal.

llvm/lib/BinaryFormat/XCOFF.cpp

43

Are you planning on adding support to yaml2obj any time soon?

Please add a TODO to this code saying that it needs testing.

llvm/tools/llvm-objdump/llvm-objdump.h

149–156

I do not think it is a good idea.

Why not? I honestly do not understand this statement. You don't object to including headers for other things though?

Having the file format parts in separate files is a perfectly normal way of doing things. See llvm-readobj, which splits the ELF/COFF/XCOFF/MachO/wasm specific parts into separate files, with a uniform interface that they are accessed via.

You also don't need to include "objdump" in the name, if you don't want to. Simply XCOFFDump or whatever is perfectly reasonable.

[jasonliu]

llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h

54

I agree with what James said.
And add to that, even if you are XCOFF, this comparison could still happen. i.e. P1 and P2 have the same Address and Name. So You have to compare XCOFFSymbolInfo in some way, even if you don't really care about the result.

[DiggerLin]

address comment

llvm/lib/BinaryFormat/XCOFF.cpp

43

I do not think we are planning to adding XCOFF support to yaml2obj soon.
I added TODO here. thanks

[jhenderson]

llvm/docs/CommandGuide/llvm-objdump.rst

329 ↗

(On Diff #252569)

Please be a little more careful when addressing review comments, to avoid repeated churn. You haven't changed to my text as requested:

"Add symbol description to disassembly output."

llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h

41

IsXCoff might want to be IsXCOFF, since Coff is an acronym. Also, it probably wants to be a private member?

llvm/lib/BinaryFormat/XCOFF.cpp

44

Perhaps also "and other SMC cases", unless those can be easily tested now.

llvm/tools/llvm-objdump/llvm-objdump.h

11

Does this really need to be included in the header? Can it be in llvm-objdump.cpp only?

llvm/tools/llvm-objdump/llvm-xcoffdump.h

1 ↗

(On Diff #252569)

Licence header is incomplete (missing first line).

[DiggerLin]

llvm/docs/CommandGuide/llvm-objdump.rst

329 ↗

(On Diff #252569)

thanks.

llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h

41

OK, thanks

llvm/tools/llvm-objdump/llvm-objdump.h

11

yes, it can put in llvm-objdump.cpp only.

llvm/tools/llvm-objdump/llvm-xcoffdump.h

1 ↗

(On Diff #252569)

thanks

[DiggerLin]

address comment

[jhenderson]

LGTM, from a stylistic point of view. Not sure I can comment much on the behaviour parts that I haven't commented on. Please wait for other reviewers to approve too.

[hubert.reinterpretcast]

llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h

53

Should we assert that P1.IsXCOFF == P2.IsXCOFF?

[hubert.reinterpretcast]

llvm/include/llvm/Object/XCOFFObjectFile.h

150

My browser search function is telling me that this new function is unused in this patch.

llvm/test/tools/llvm-objdump/xcoff-disassemble-all.test

5

@jasonliu, what is the plan for the merge conflict between this and D75131? In particular, would the relocation printing be tested with both the plain and the descriptive output?

[jasonliu]

llvm/test/tools/llvm-objdump/xcoff-disassemble-all.test

5

I think ultimately we might want to have variations for
-D
-D --symbol-description
-D -r
-D -r --symbol-description

But the testing for -D -r --symbol-description is a bit tricky, as I think if we enable that, relocation printing's symbol should be the new format too, which is not implemented. So I'm Okay for that to be left out, or we just put it in and change later.

[hubert.reinterpretcast]

llvm/test/tools/llvm-objdump/xcoff-disassemble-all.test

5

What is the difference if effort between needing to rebase this patch anyway and adding the descriptive output for the relocation printing within this patch?

llvm/tools/llvm-objdump/XCOFFDump.cpp

17

I had mentioned this with another file on this patch; it is now documented: https://llvm.org/docs/CodingStandards.html#use-namespace-qualifiers-to-implement-previously-declared-functions.

llvm/tools/llvm-objdump/llvm-xcoffdump.h

26 ↗

(On Diff #252846)

I am having trouble accepting that llvm-objdump has any business injecting symbols directly into the llvm namespace. I understand that llvm-objdump.h does do so. @jhenderson, would you object if we introduced an llvm::objdump_impl namespace?

[jasonliu]

llvm/test/tools/llvm-objdump/xcoff-disassemble-all.test

5

We need to call printXCOFFSymbolDescription in the new getXCOFFRelocationValueString function. It shouldn't be a big effort.

[hubert.reinterpretcast]

llvm/tools/llvm-objdump/XCOFFDump.cpp

28

The assertion makes sense. Would it make more sense as part of hasCsectAuxEnt? That is, if hasCsectAuxEnt were to return true, then it should assert that there is at least one auxiliary entry (of which we would assume that the last is the csect auxiliary entry).

Also, minor nit: Use >= 1 if that is actually what is being tested for.

29

Please use one space only between words.

31

Can we avoid the cast entirely?

43

Seeing a second instance of the assert makes me want it moved even more.

45

Please don't create variables for expressions where expanding the expression at the single point of use reads just fine. Note that this probably applies to the other AuxEntPtr too if the cast could be removed.

[hubert.reinterpretcast]

llvm/tools/llvm-objdump/XCOFFDump.cpp

54

Please add a comment above this block explaining the circumstances when there is no index (say, in the case of a generated "dummy" value). Be exact if possible.

63

Please don't call .str() here.

llvm/tools/llvm-objdump/llvm-objdump.cpp

1155

SymbolType is unused on the descriptive-XCOFF path. Move it into the else (perhaps avoiding it entirely with Obj->isELF() ? getElfSymbolType(Obj, Symbol) : ELF::STT_NOTYPE.

1159

Given that we checked Obj->isXCOFF(), can we use cast instead of dyn_cast?

1162

Can't this just be uint32_t? I believe that the call to the SymbolInfoTy constructor would do implicit conversion.

llvm/tools/llvm-objdump/llvm-objdump.h

14

Moving the include directive is the only change in this file from the patch. We don't need to touch this file at all.

llvm/tools/llvm-objdump/llvm-xcoffdump.h

1 ↗

(On Diff #252846)

The original suggestion was for XCOFFDump.h as the header name. Is there a reason to invent llvm-xcoffdump.h?

[hubert.reinterpretcast]

llvm/tools/llvm-objdump/llvm-objdump.cpp

1205

With descriptive printing, we can print at least the index for a symbol with an empty name?

[hubert.reinterpretcast]

llvm/test/tools/llvm-objdump/xcoff-disassemble-all.test

4

The entire object file has only one label definition and it is currently not tested. The symbol being used for the address associated with the label definition is currently a C_STAT symbol. One way of salvaging the test coverage is to implement the sorting change I am requesting.

61

This has helped me understand something about what is happening.

We should change the sorting so that we prefer symbols with CSECT Auxiliary Entries (i.e., it has a storage mapping class--even if it is a label and we won't print it). We should also prefer labels before csects. In other words, the name should only be considered after the criteria I just listed. The rationale is that the more specific symbol would be selected for the address.

Yes, it does mean that we should generate the XCOFF form of the symbol info always.
@jasonliu @daltenty @sfertile, fyi.

This might be a separate patch; however, note that doing the above as part of this patch happens to fix a test coverage issue.

[jhenderson]

llvm/tools/llvm-objdump/llvm-xcoffdump.h

26 ↗

(On Diff #252846)

Seems like a reasonable idea, although I'd probably just call it llvm::objdump.

[DiggerLin]

address comment and rebased the code based on https://reviews.llvm.org/D75131 [llvm-objdump][XCOFF][AIX] Implement -r option

[DiggerLin]

address comment

llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h

53

thanks

llvm/test/tools/llvm-objdump/xcoff-disassemble-all.test

4

As we discuss on the scrum meeting, This comment will be addressed in another patch.

61

As we discuss on the scrum meeting, We will implement it in another patch.

llvm/tools/llvm-objdump/XCOFFDump.cpp

63

thanks

llvm/tools/llvm-objdump/llvm-objdump.cpp

1205

I am not sure there is a symbol with an empty name in symbol table ?

llvm/tools/llvm-objdump/llvm-objdump.h

14

when git clang format, it will move the include directive automatically .

llvm/tools/llvm-objdump/llvm-xcoffdump.h

26 ↗

(On Diff #252846)

do this in a new patch ?

[hubert.reinterpretcast]

llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h

54

Please clang-format as the linter indicated for all cases identified.

llvm/include/llvm/Object/XCOFFObjectFile.h

132

This can now be

static constexpr uint8_t SymbolTypeMask = 0x07u;

133

This is unused now.

llvm/lib/Object/XCOFFObjectFile.cpp

798 ↗

(On Diff #253668)

Sorry for the churn. Seeing it in this context makes me realize that this probably can't be an assert in the first place unless if know that the checking was already done. We will need a function without the assert to query whether or not we should expect to find a CSECT Auxiliary Entry (hasCsectAuxExt is not named well for that purpose). When we request a CSECT Auxiliary Entry, we can assert that we had a reason to expect that there is one and we can also pass an error back to the caller if there isn't one. Whether or not we can propagate said error without too much difficulty will depend on where the caller is.

llvm/test/tools/llvm-objdump/XCOFF/disassemble-symbol-description.test

20 ↗

(On Diff #253668)

Please address take the comments identified on the original test file in D75131 into account. For example, the REQUIRES should be moved and made consistent with the use of # on the other directive lines, the comment lines should use ##, etc.

llvm/tools/llvm-objdump/XCOFFDump.cpp

14

Please name the header XCOFFDump.h since the non-header source file is XCOFFDump.cpp.

43

As mentioned on the prior review, this variable is now not particularly helpful to have. I do note that this block of code is likely to change again anyway if getXCOFFCsectAuxEnt32 is changed such that it can indicate an error.

63

Suggestion
Dummy symbols have no symbol index.

llvm/tools/llvm-objdump/XCOFFdump.h

1 ↗

(On Diff #253668)

In addition to the naming fix requested, please adjust the length of this line.

llvm/tools/llvm-objdump/llvm-xcoffdump.h

26 ↗

(On Diff #252846)

Please do the XCOFF part in this patch.

[jhenderson]

llvm/tools/llvm-objdump/XCOFFDump.cpp

1

No need for the "C++" part of this line. That's only to help editors know that a header file is in C++ and to enable appropriate syntax highlighting. See https://llvm.org/docs/CodingStandards.html#file-headers.

llvm/tools/llvm-objdump/llvm-objdump.h

14

Only clang-format lines of code in the immediate area where you are modifying something. For example, it would be okay to modify this line if you are including a new header, but not otherwise.

[DiggerLin]

address comment

llvm/lib/Object/XCOFFObjectFile.cpp

798 ↗

(On Diff #253668)

I think assert(getNumberOfAuxEntries() >=1 is OK ,here , because the function is hasCsectAuxEnt() , The function names means it should at least has one Csect Aux entry.

we can return Expected<bool> for the function.

I am prefer to change it in NFC patch. because there are some place using the function too.(it is not in the scope the patch) . and the return type of function getXCOFFCsectAuxEnt32 maybe need to change to expect<const XCOFFCsectAuxEnt32 *>

Maybe we also need a new function in the NFC patch

bool XCOFFSymbolRef::withCsectAuxEnt() const {

XCOFF::StorageClass SC = getStorageClass();
return (SC == XCOFF::C_EXT || SC == XCOFF::C_WEAKEXT ||
        SC == XCOFF::C_HIDEXT);

}

llvm/tools/llvm-objdump/XCOFFDump.cpp

1

thanks

[DiggerLin]

simplify some code.

[jhenderson]

llvm/lib/Object/XCOFFObjectFile.cpp

799 ↗

(On Diff #254038)

clang-format is still complaining here.

llvm/test/tools/llvm-objdump/XCOFF/disassemble-symbol-description.test

1 ↗

(On Diff #254038)

Use '#' for all lit directives, not just the RUN lines.

[DiggerLin]

llvm/lib/Object/XCOFFObjectFile.cpp

799 ↗

(On Diff #254038)

thanks

llvm/test/tools/llvm-objdump/XCOFF/disassemble-symbol-description.test

1 ↗

(On Diff #254038)

thanks

[DiggerLin]

address comment

[hubert.reinterpretcast]

llvm/lib/Object/XCOFFObjectFile.cpp

799 ↗

(On Diff #254038)

As far as I know, the assert may fail due to the XCOFF file being invalid. Please remove the assert and add the TODO comments.

798 ↗

(On Diff #253668)

The way it is named, if there isn't a CSECT Auxiliary Entry, then the result of this function should just be false. That's not what we are looking for. This function is used (as you also noted) in other places. We can leave the function name and implementation unchanged and rename it in a separate patch. We can also change getXCOFFCsectAuxEnt32 to be able to express an error condition in that patch. In this patch, please add a TODO for each of these changes where the affected function is defined.

[hubert.reinterpretcast]

llvm/tools/llvm-objdump/XCOFFDump.cpp

63

Add a way to assert SymbolInfo.XCOFFSymInfo.isXCOFF. Maybe a getXCOFFSymInfo function that asserts isXCOFF.

llvm/tools/llvm-objdump/llvm-objdump.cpp

1158

We can use XCOFFObj instead of casting Obj again.

1205

extern const char *const str = "Hello, world!";

Compiled with:

IBM XL C/C++ for AIX, V16.1.0  (5725-C72, 5765-J12)
Version: 16.01.0000.0004

Has this:

                        ***Relocation Information***
             Vaddr      Symndx  Sign  Fixup     Len      Type  Name
.data:
        0x00000000  0x0000000e     0      0  0x001f   Pos_Rel  str
        0x00000004  0x0000000c     0      0  0x001f   Pos_Rel  **No Symbol**
        0x00000008  0x0000000c     0      0  0x001f   Pos_Rel  **No Symbol**

[ ... ]

[12]    m   0x0000000c     .data     1  unamex                    **No Symbol**
[13]    a4  0x0000000e       0    0     SD       RO    0    0

[hubert.reinterpretcast]

llvm/test/tools/llvm-objdump/XCOFF/disassemble-symbol-description.test

67 ↗

(On Diff #254179)

This needs to be updated to match recent changes to the tooling output. You may need to rebase your branch.

[DiggerLin]

modified the test case based on the latest llvm code.

[hubert.reinterpretcast]

It seems we are still expecting some more comments to be addressed? @DiggerLin, in the future please clearly acknowledge all comments that have not yet been addressed if you post an updated patch that does not address all comments.

llvm/tools/llvm-objdump/XCOFFDump.cpp

45

I would rather leave the asserts out since the TODO for getXCOFFCsectAuxEnt32 will cover it. In particular, I'm not convinced that assertions are the right tool here.

59

Same comment re: the assertion.

[DiggerLin]

llvm/tools/llvm-objdump/XCOFFDump.cpp

45

since we have not implement the TODO, I think it is better to put a assert here.
when we implement the "TODO for getXCOFFCsectAuxEnt32" , I will remove the assert.

59

since we have not implement the TODO, I think it is better to put a assert here.
when we implement the "TODO for getXCOFFCsectAuxEnt32" , I will remove the assert.

[DiggerLin]

llvm/tools/llvm-objdump/XCOFFDump.cpp

45

Agree with "assertions may be not the right tool here" . I delete the assert.

[hubert.reinterpretcast]

llvm/tools/llvm-objdump/XCOFFDump.h

12 ↗

(On Diff #254386)

What is this header being included from this header for?

13 ↗

(On Diff #254386)

The function declarations do not require definitions for most of the types. Class types can be declared as incomplete in this header. XCOFF::StorageMappingClass comes from llvm/BinaryFormat/XCOFF.h.

17 ↗

(On Diff #254386)

Please include an interface header that is expected to provide Optional as part of its interface.

24 ↗

(On Diff #254386)

Same comment about interface header but for StringRef.

28 ↗

(On Diff #254386)

Ditto SmallVectorImpl.

[jhenderson]

llvm/lib/Object/XCOFFObjectFile.cpp

769 ↗

(On Diff #254386)

need -> needs (same below)

Is there any reason you can't do this in a separate and prerequisite commit?

796–797 ↗

(On Diff #254386)

Is there any reason you can't do this in a separate and prerequisite commit?

[hubert.reinterpretcast]

llvm/tools/llvm-objdump/llvm-objdump.cpp

76

In D77285, there's a comment from MaskRay:

Add a new line before namespace llvm

[DiggerLin]

llvm/lib/Object/XCOFFObjectFile.cpp

769 ↗

(On Diff #254386)

yes, we can do it separate and prerequisite commit. but We prefer to have a NFC plan for it after this patch . @hubert.reinterpretcast

llvm/tools/llvm-objdump/XCOFFDump.h

28 ↗

(On Diff #254386)

for above several comment, I have different option.

1. the XCOFFDump.h not compile independently. it be included in the file llvm/tools/llvm-objdump/XCOFFDump.cpp and llvm/tools/llvm-objdump/llvm-objdump.cpp

if I do not include these two files
#include "llvm/MC/MCDisassembler/MCDisassembler.h"
#include "llvm/Object/XCOFFObjectFile.h"
in the llvm/tools/llvm-objdump/XCOFFDump.h
I have to include above two file into
llvm/tools/llvm-objdump/XCOFFDump.cpp and
llvm/tools/llvm-objdump/llvm-objdump.cpp

and from compiler view,when it compile the llvm/tools/llvm-objdump/XCOFFDump.cpp , If I change as you suggestion.
compiler has to open file llvm/tools/llvm-objdump/XCOFFDump.h (and all it depend header files ) and opened "llvm/MC/MCDisassembler/MCDisassembler.h"
and #include "llvm/Object/XCOFFObjectFile.h" later. I do not think it is more efficient. (header file StringRef will opened several times.)

and there are a several lines of something like
using object::XCOFFObjectFile;
using object::SymbolRef;
using object::RelocationRef;
........

I think it is not as simple as I include these two files here.

[hubert.reinterpretcast]

llvm/lib/Object/XCOFFObjectFile.cpp

769 ↗

(On Diff #254386)

The change indicated by the TODO might be easier to understand and tune with the context added by the additional uses from this patch. Adding it to this patch would go past the intended scope of this patch to include adjustments needed for some existing calls. I think a follow-on patch makes sense.

I do understand that there have been multiple things identified as improvements that we want through the review of this patch. In terms of improvements to existing code quality, we are working on D77285, and the subject of the TODO here is the other code quality (as opposed to, e.g., functional behaviour) follow-on.

[hubert.reinterpretcast]

llvm/lib/Object/XCOFFObjectFile.cpp

769 ↗

(On Diff #254386)

I expect the code comment to go away shortly, but note that our intended style for "csect" is to use all-lowercase when the word is used in a context where it would not be capitalized in English (and "CSECT" when it would be). "Csect" is only used for camelCasing purposes.

796–797 ↗

(On Diff #254386)

I would like to clarify that I believe this TODO could be done at the same time as change to getXCOFFCsectAuxEnt32.

llvm/tools/llvm-objdump/XCOFFDump.cpp

63

Please address: https://reviews.llvm.org/D72973?id=254179#inline-705870.

llvm/tools/llvm-objdump/llvm-objdump.cpp

1158

Please address: https://reviews.llvm.org/D72973?id=254179#inline-705885.

1205

Please address: https://reviews.llvm.org/D72973?id=252846#inline-705899.

[DiggerLin]

address comment

[hubert.reinterpretcast]

llvm/tools/llvm-objdump/XCOFFDump.h

17 ↗

(On Diff #254386)

The comment is not actually addressed. XCOFFObjectFile.h does not physically contain any reference to Optional.

[hubert.reinterpretcast]

LGTM with the one comment to include "llvm/ADT/Optional.h".

[jhenderson]

llvm/tools/llvm-objdump/XCOFFDump.h

17 ↗

(On Diff #254386)

I'm not sure this is necessary? According to https://llvm.org/docs/CodingStandards.html#include-as-little-as-possible it's okay to go via other headers to include things. The route to ADT/Optional.h is:

Object/XCOFFObjectFile.h
BinaryFormat/XCOFF.h
ADT/StringRef.h
ADT/STLExtras.h
ADT/Optional.h

(From a philosophical point of view, I don't actually agree with this part of the coding standards - when coding on other projects, I try to include the headers directly that define the things I need - but we should adhere to LLVM style).

24 ↗

(On Diff #254386)

Ditto response.

[hubert.reinterpretcast]

llvm/tools/llvm-objdump/XCOFFDump.h

17 ↗

(On Diff #254386)

Okay. Thanks.

24 ↗

(On Diff #254386)

Agreed.

[hubert.reinterpretcast]

llvm/tools/llvm-objdump/XCOFFDump.h

16 ↗

(On Diff #255331)

Issue with -Werror builds noted. Fix is on its way. Thanks @DiggerLin.

llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h

 //===- llvm/MC/MCDisassembler.h - Disassembler interface --------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_MC_MCDISASSEMBLER_MCDISASSEMBLER_H
 #define LLVM_MC_MCDISASSEMBLER_MCDISASSEMBLER_H
 
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/BinaryFormat/XCOFF.h"
 #include "llvm/MC/MCDisassembler/MCSymbolizer.h"
+
 #include <cstdint>
 #include <memory>
 #include <vector>
 
 namespace llvm {
 
+struct XCOFFSymbolInfo {
+  uint64_t Index;
+  Optional<XCOFF::StorageMappingClass> StorageMappingClass;
+  bool ShowSymDesc;
+  XCOFFSymbolInfo(uint64_t Idx, Optional<XCOFF::StorageMappingClass> Smc,
+                  bool Show)
+      : Index(Idx), StorageMappingClass(Smc), ShowSymDesc(Show){};
+};
+
 struct SymbolInfoTy {
   uint64_t  Addr;
   StringRef Name;
-  uint8_t   Type;
+
+        union {
+          uint8_t Type;
+          XCOFFSymbolInfo XCoffSymInfo;
+        };
+
+        SymbolInfoTy(uint64_t Addr, StringRef Name,
+                     Optional<XCOFF::StorageMappingClass> Smc, int64_t Idx,
+                     bool Show)
+            : Addr(Addr), Name(Name), XCoffSymInfo(Idx, Smc, Show){};
 
         SymbolInfoTy(uint64_t Addr, StringRef Name, uint8_t Type)
             : Addr(Addr), Name(Name), Type(Type){};
 
         friend bool operator<(const SymbolInfoTy &P1, const SymbolInfoTy &P2) {
           return std::tie(P1.Addr, P1.Name, P1.Type) <
                  std::tie(P2.Addr, P2.Name, P2.Type);
         }
 };
 
 using SectionSymbolsTy = std::vector<SymbolInfoTy>;
 
-
 template <typename T> class ArrayRef;
 class MCContext;
 class MCInst;
 class MCSubtargetInfo;
 class raw_ostream;
 
 /// Superclass for all disassemblers. Consumes a memory region and provides an
 /// array of assembly instructions.

llvm/include/llvm/Object/XCOFFObjectFile.h

 };
 
 struct XCOFFStringTable {
   uint32_t Size;
   const char *Data;
 };
 
 struct XCOFFCsectAuxEnt32 {
+  enum {
+    SymbolTypeMask = 0x07,
+    SymbolAlignmentMask = 0xF8,
+    SymbolAlignmentBitOffset = 3
+  };
+
   support::ubig32_t
       SectionOrLength; // If the symbol type is XTY_SD or XTY_CM, the csect
                        // length.
                        // If the symbol type is XTY_LD, the symbol table
                        // index of the containing csect.
                        // If the symbol type is XTY_ER, 0.
   support::ubig32_t ParameterHashIndex;
   support::ubig16_t TypeChkSectNum;
   uint8_t SymbolAlignmentAndType;
   XCOFF::StorageMappingClass StorageMappingClass;
   support::ubig32_t StabInfoIndex;
   support::ubig16_t StabSectNum;
+
+  uint16_t getAlignmentLog2() const {
+    return (SymbolAlignmentAndType & SymbolAlignmentMask) >>
+           SymbolAlignmentBitOffset;
+  };
+
+  uint8_t getSymbolType() const {
+    return SymbolAlignmentAndType & SymbolTypeMask;
+  };
+
+  bool isLabel() const { return getSymbolType() == XCOFF::XTY_LD; }
 };
 
 struct XCOFFFileAuxEnt {
   typedef struct {
     support::big32_t Magic; // Zero indicates name in string table.
     support::ubig32_t Offset;
     char NamePad[XCOFF::FileNamePadSize];
   } NameInStrTblType;

llvm/lib/BinaryFormat/XCOFF.cpp

 //===-- llvm/BinaryFormat/XCOFF.cpp - The XCOFF file format -----*- C++/-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/BinaryFormat/XCOFF.h"
 
 using namespace llvm;
 
+#define SMC_CASE(A)                                                            \
+  case XCOFF::XMC_##A:                                                         \
+    return #A;
 StringRef XCOFF::getMappingClassString(XCOFF::StorageMappingClass SMC) {
   switch (SMC) {
-  case XCOFF::XMC_DS:
-    return "DS";
-  case XCOFF::XMC_RW:
-    return "RW";
-  case XCOFF::XMC_PR:
-    return "PR";
-  case XCOFF::XMC_TC0:
-    return "TC0";
-  case XCOFF::XMC_BS:
-    return "BS";
-  case XCOFF::XMC_RO:
-    return "RO";
-  case XCOFF::XMC_UA:
-    return "UA";
-  case XCOFF::XMC_TC:
-    return "TC";
+    SMC_CASE(PR)
+    SMC_CASE(RO)
+    SMC_CASE(DB)
+    SMC_CASE(GL)
+    SMC_CASE(XO)
+    SMC_CASE(SV)
+    SMC_CASE(SV64)
+    SMC_CASE(SV3264)
+    SMC_CASE(TI)
+    SMC_CASE(TB)
+    SMC_CASE(RW)
+    SMC_CASE(TC0)
+    SMC_CASE(TC)
+    SMC_CASE(TD)
+    SMC_CASE(DS)
+    SMC_CASE(UA)
+    SMC_CASE(BS)
+    SMC_CASE(UC)
+    SMC_CASE(TL)
+    SMC_CASE(UL)
+    SMC_CASE(TE)
+#undef SMC_CASE
   default:
     report_fatal_error("Unhandled storage-mapping class.");
   }
 }

llvm/test/tools/llvm-objdump/xcoff-disassemble-all.test

-# RUN: llvm-objdump -D %p/Inputs/xcoff-section-headers.o | \
+# RUN: llvm-objdump -D --symbol-description %p/Inputs/xcoff-section-headers.o | \
 # RUN: FileCheck %s
 
+# RUN: llvm-objdump -D  %p/Inputs/xcoff-section-headers.o | \
+# RUN: FileCheck --check-prefix=CHECK-GNU %s
+
 # xcoff-section-headers.o Compiled with IBM XL C/C++ for AIX, V16.1.0
 # compiler command: xlc -qtls -o xcoff-section-headers.o -c test.c
 # test.c:
 # int a;
 # int b = 12345;
 # __thread int c;
 # __thread double d = 3.14159;
 #
 # int func(void)  {
 #   return a;
 # }
 ; REQUIRES: powerpc-registered-target
 CHECK: Inputs/xcoff-section-headers.o:  file format aixcoff-rs6000
 CHECK: Disassembly of section .text:
-CHECK: 00000000 .text:
+CHECK: 00000000 (idx: 4) .text:
 CHECK-NEXT:        0: 80 62 00 04                   lwz 3, 4(2)
 CHECK-NEXT:        4: 80 63 00 00                   lwz 3, 0(3)
 CHECK-NEXT:        8: 4e 80 00 20                   blr
 CHECK-NEXT:        c: 00 00 00 00                   <unknown>
 CHECK-NEXT:       10: 00 00 20 40                   <unknown>
 CHECK-NEXT:       14: 00 00 00 01                   <unknown>
 CHECK-NEXT:       18: 00 00 00 0c                   <unknown>
 CHECK-NEXT:       1c: 00 04 66 75                   <unknown>
 CHECK-NEXT:       20: 6e 63 00 00                   xoris 3, 19, 0
 CHECK-NEXT:     ...
 CHECK: Disassembly of section .data:
-CHECK: 00000080 func:
+CHECK: 00000080 (idx: 22) func[TC]:
 CHECK-NEXT:       80: 00 00 00 94                   <unknown>
-CHECK: 00000084 a:
+CHECK: 00000084 (idx: 26) a[TC]:
 CHECK-NEXT:       84: 00 00 00 a4                   <unknown>
-CHECK: 00000088 b:
+CHECK: 00000088 (idx: 30) b[TC]:
 CHECK-NEXT:       88: 00 00 00 a0                   <unknown>
-CHECK: 0000008c c:
+CHECK: 0000008c (idx: 34) c[TC]:
 CHECK-NEXT:       8c: 00 00 00 08                   <unknown>
-CHECK: 00000090 d:
+CHECK: 00000090 (idx: 38) d[TC]:
 CHECK-NEXT:       90: 00 00 00 00                   <unknown>
-CHECK: 00000094 func:
+CHECK: 00000094 (idx: 20) func[DS]:
 CHECK-NEXT:       94: 00 00 00 00                   <unknown>
 CHECK-NEXT:       98: 00 00 00 80                   <unknown>
 CHECK-NEXT:       9c: 00 00 00 00                   <unknown>
-CHECK: 000000a0 b:
+CHECK: 000000a0 (idx: 28) b[RW]:
 CHECK-NEXT:       a0: 00 00 30 39                   <unknown>
 CHECK: Disassembly of section .bss:
-CHECK: 000000a4 a:
+CHECK: 000000a4 (idx: 24) a[RW]:
 CHECK-NEXT: ...
 CHECK: Disassembly of section .tdata:
-CHECK: 00000000 d:
+CHECK: 00000000 (idx: 36) d[TL]:
 CHECK-NEXT:        0: 40 09 21 f9                   bdnzfl  9, .+8696
 CHECK-NEXT:        4: f0 1b 86 6e                   <unknown>
 CHECK: Disassembly of section .tbss:
-CHECK: 00000008 c:
+CHECK: 00000008 (idx: 32) c[UL]:
 CHECK-NEXT: ...
+
+
+
+CHECK-GNU: Inputs/xcoff-section-headers.o:      file format aixcoff-rs6000
+CHECK-GNU: Disassembly of section .text:
+CHECK-GNU: 00000000 .text:
+CHECK-GNU-NEXT:        0: 80 62 00 04                   lwz 3, 4(2)
+CHECK-GNU-NEXT:        4: 80 63 00 00                   lwz 3, 0(3)
+CHECK-GNU-NEXT:        8: 4e 80 00 20                   blr
+CHECK-GNU-NEXT:        c: 00 00 00 00                   <unknown>
+CHECK-GNU-NEXT:       10: 00 00 20 40                   <unknown>
+CHECK-GNU-NEXT:       14: 00 00 00 01                   <unknown>
+CHECK-GNU-NEXT:       18: 00 00 00 0c                   <unknown>
+CHECK-GNU-NEXT:       1c: 00 04 66 75                   <unknown>
+CHECK-GNU-NEXT:       20: 6e 63 00 00                   xoris 3, 19, 0
+CHECK-GNU-NEXT:                 ...
+CHECK-GNU: Disassembly of section .data:
+CHECK-GNU: 00000080 func:
+CHECK-GNU-NEXT:       80: 00 00 00 94                   <unknown>
+CHECK-GNU: 00000084 a:
+CHECK-GNU-NEXT:       84: 00 00 00 a4                   <unknown>
+CHECK-GNU: 00000088 b:
+CHECK-GNU-NEXT:       88: 00 00 00 a0                   <unknown>
+CHECK-GNU: 0000008c c:
+CHECK-GNU-NEXT:       8c: 00 00 00 08                   <unknown>
+CHECK-GNU: 00000090 d:
+CHECK-GNU-NEXT:       90: 00 00 00 00                   <unknown>
+CHECK-GNU: 00000094 func:
+CHECK-GNU-NEXT:       94: 00 00 00 00                   <unknown>
+CHECK-GNU-NEXT:       98: 00 00 00 80                   <unknown>
+CHECK-GNU-NEXT:       9c: 00 00 00 00                   <unknown>
+CHECK-GNU: 000000a0 b:
+CHECK-GNU-NEXT:       a0: 00 00 30 39                   <unknown>
+CHECK-GNU: Disassembly of section .bss:
+CHECK-GNU: 000000a4 a:
+CHECK-GNU-NEXT: ...
+CHECK-GNU: Disassembly of section .tdata:
+CHECK-GNU: 00000000 d:
+CHECK-GNU-NEXT:        0: 40 09 21 f9                   bdnzfl  9, .+8696
+CHECK-GNU-NEXT:        4: f0 1b 86 6e                   <unknown>
+CHECK-GNU: Disassembly of section .tbss:
+CHECK-GNU: 00000008 c:
+CHECK-GNU-NEXT: ...

llvm/tools/llvm-objdump/CMakeLists.txt

set(LLVM_LINK_COMPONENTS
   )
 
 add_llvm_tool(llvm-objdump
   llvm-objdump.cpp
   COFFDump.cpp
   ELFDump.cpp
   MachODump.cpp
   WasmDump.cpp
+  XCOFFDump.cpp
   )
 
 if(HAVE_LIBXAR)
   target_link_libraries(llvm-objdump PRIVATE ${XAR_LIB})
 endif()
 
 if(LLVM_INSTALL_BINUTILS_SYMLINKS)
   add_llvm_tool_symlink(objdump llvm-objdump)
 endif()

llvm/tools/llvm-objdump/XCOFFDump.cpp

+//===-- XCOFFDump.cpp - Object file dumping utility for llvm --------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file implements XCOFF object dump information.
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm-objdump.h"
+#include "llvm/Object/XCOFFObjectFile.h"
+
+namespace llvm {
+
+using namespace llvm::object;
+
+Optional<XCOFF::StorageMappingClass>
+getXCOFFSymbolCsectSMC(const ObjectFile *Obj, const SymbolRef &Sym) {
+  assert(Obj->isXCOFF() && "Not XCOFF object file.");
+  DataRefImpl SymbolDRI = Sym.getRawDataRefImpl();
+
+  XCOFFSymbolRef SymRef(SymbolDRI, static_cast<const XCOFFObjectFile *>(Obj));
+
+  if (SymRef.hasCsectAuxEnt()) {
+    assert(SymRef.getNumberOfAuxEntries() &&
+           "No CSECT  Auxiliary Entry is found.");
+    const XCOFFCsectAuxEnt32 *AuxEntPtr = SymRef.getXCOFFCsectAuxEnt32();
+    return Optional<XCOFF::StorageMappingClass>(AuxEntPtr->StorageMappingClass);
+  }
+  return Optional<XCOFF::StorageMappingClass>();
+}
+
+int64_t getSymbolIndex(const ObjectFile *Obj, const SymbolRef &Sym) {
+  if (!Obj->isXCOFF())
+    return No_Sym_Index;
+
+  const auto *XCOFFObj = dyn_cast<XCOFFObjectFile>(Obj);
+  DataRefImpl SymbolDRI = Sym.getRawDataRefImpl();
+  return XCOFFObj->getSymbolIndex(SymbolDRI.p);
+}
+
+bool isSymbolDescriptionDisplay(const ObjectFile *Obj, const SymbolRef &Sym) {
+  if (!Obj->isXCOFF())
+    return false;
+
+  DataRefImpl SymbolDRI = Sym.getRawDataRefImpl();
+
+  XCOFFSymbolRef SymRef(SymbolDRI, static_cast<const XCOFFObjectFile *>(Obj));
+
+  if (SymRef.hasCsectAuxEnt()) {
+    assert(SymRef.getNumberOfAuxEntries() &&
+           "No CSECT Auxiliary Entry is found.");
+
+    const XCOFFCsectAuxEnt32 *AuxEntPtr = SymRef.getXCOFFCsectAuxEnt32();
+    return !AuxEntPtr->isLabel();
+  }
+
+  return false;
+}
+
+void printXCOFFSymbolDescription(SymbolInfoTy &SymbolInfo,
+                                 std::string &SymbolName) {
+
+  const int64_t SymIndex = SymbolInfo.XCoffSymInfo.Index;
+  if (SymIndex >= 0)
+    outs() << "(idx: " << SymIndex << ") ";
+
+  outs() << SymbolName;
+
+  if (!SymbolInfo.XCoffSymInfo.StorageMappingClass)
+    return;
+
+  const XCOFF::StorageMappingClass Smc =
+      SymbolInfo.XCoffSymInfo.StorageMappingClass.getValue();
+  if (SymIndex >= 0 && SymbolInfo.XCoffSymInfo.ShowSymDesc)
+    outs() << "[" << XCOFF::getMappingClassString(Smc).str() << "]";
+}
+
+} // namespace llvm

llvm/tools/llvm-objdump/llvm-objdump.h

 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_TOOLS_LLVM_OBJDUMP_LLVM_OBJDUMP_H
 #define LLVM_TOOLS_LLVM_OBJDUMP_LLVM_OBJDUMP_H
 
+#include "llvm/ADT/Optional.h"
+#include "llvm/BinaryFormat/XCOFF.h"
 #include "llvm/DebugInfo/DIContext.h"
+#include "llvm/MC/MCDisassembler/MCDisassembler.h"
+#include "llvm/Object/Archive.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/DataTypes.h"
-#include "llvm/Object/Archive.h"
 
 namespace llvm {
 class StringRef;
 
 namespace object {
 class COFFObjectFile;
 class COFFImportFile;
 class ELFObjectFileBase;
 class ELFSectionRef;
 class MachOObjectFile;
 class MachOUniversalBinary;
 class RelocationRef;
 }
 
+static const int64_t No_Sym_Index = -1;
+
 extern cl::opt<bool> Demangle;
 
 typedef std::function<bool(llvm::object::SectionRef const &)> FilterPredicate;
 
 /// A filtered iterator for SectionRefs that skips sections based on some given
 /// predicate.
 class SectionFilterIterator {
 public:

 void printWeakBindTable(object::ObjectFile *O);
 void printRawClangAST(const object::ObjectFile *O);
 void printRelocations(const object::ObjectFile *O);
 void printDynamicRelocations(const object::ObjectFile *O);
 void printSectionHeaders(const object::ObjectFile *O);
 void printSectionContents(const object::ObjectFile *O);
 void printSymbolTable(const object::ObjectFile *O, StringRef ArchiveName,
                       StringRef ArchitectureName = StringRef());
+
+Optional<XCOFF::StorageMappingClass>
+getXCOFFSymbolCsectSMC(const object::ObjectFile *Obj,
+                       const object::SymbolRef &Sym);
+bool isSymbolDescriptionDisplay(const object::ObjectFile *Obj,
+                                const object::SymbolRef &Sym);
+int64_t getSymbolIndex(const object::ObjectFile *Obj,
+                       const object::SymbolRef &Sym);
+void printXCOFFSymbolDescription(SymbolInfoTy &SymbolInfo,
+                                 std::string &SymbolName);
+
 LLVM_ATTRIBUTE_NORETURN void reportError(StringRef File, Twine Message);
 LLVM_ATTRIBUTE_NORETURN void reportError(Error E, StringRef FileName,
                                          StringRef ArchiveName = "",
                                          StringRef ArchitectureName = "");
 void reportWarning(Twine Message, StringRef File);
 
 template <typename T, typename... Ts>
 T unwrapOrError(Expected<T> EO, Ts &&... Args) {

llvm/tools/llvm-objdump/llvm-objdump.cpp

 #include "llvm/Object/Archive.h"
 #include "llvm/Object/COFF.h"
 #include "llvm/Object/COFFImportFile.h"
 #include "llvm/Object/ELFObjectFile.h"
 #include "llvm/Object/MachO.h"
 #include "llvm/Object/MachOUniversal.h"
 #include "llvm/Object/ObjectFile.h"
 #include "llvm/Object/Wasm.h"
+#include "llvm/Object/XCOFFObjectFile.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/GraphWriter.h"

 #include <cctype>
 #include <cstring>
 #include <system_error>
 #include <unordered_map>
 #include <utility>
 
 using namespace llvm::object;
 
 namespace llvm {
 
 cl::OptionCategory ObjdumpCat("llvm-objdump Options");
 
 // MachO specific
 extern cl::OptionCategory MachOCat;
 extern cl::opt<bool> Bind;
 extern cl::opt<bool> DataInCode;
 extern cl::opt<bool> DylibsUsed;
cl::opt<bool> DisassembleAll(
     "disassemble-all",
     cl::desc("Display assembler mnemonics for the machine instructions"),
     cl::cat(ObjdumpCat));
 static cl::alias DisassembleAllShort("D",
                                      cl::desc("Alias for --disassemble-all"),
                                      cl::NotHidden, cl::Grouping,
                                      cl::aliasopt(DisassembleAll));
 
+cl::opt<bool>
+    SymbolDescription("symbol-description",
+                      cl::desc("Add symbol description for disassembly"),
+                      cl::init(false), cl::cat(ObjdumpCat));
+
 static cl::list<std::string>
     DisassembleFunctions("disassemble-functions", cl::CommaSeparated,
                          cl::desc("List of functions to disassemble. "
                                   "Accept demangled names when --demangle is "
                                   "specified, otherwise accept mangled names"),
                          cl::cat(ObjdumpCat));
 
 static cl::opt<bool> DisassembleZeroes(
     "disassemble-zeroes",

 static cl::alias WideShort("w", cl::Grouping, cl::aliasopt(Wide));
 
 static cl::extrahelp
     HelpResponse("\nPass @FILE as argument to read options from FILE.\n");
 
 static StringSet<> DisasmFuncsSet;
 StringSet<> FoundSectionSet;
 static StringRef ToolName;
 
 namespace {
 struct FilterResult {
   // True if the section should not be skipped.
   bool Keep;
 
   // True if the index counter should be incremented, even if the section should
   // be skipped. For example, sections may be skipped if they are not included
   // in the --section flag, but we still want those to count toward the section
   // count.
if (auto *Elf64LEObj = dyn_cast<ELF64LEObjectFile>(Obj))
     return Elf64LEObj->getSymbol(Sym.getRawDataRefImpl())->getType();
   if (auto *Elf32BEObj = dyn_cast<ELF32BEObjectFile>(Obj))
     return Elf32BEObj->getSymbol(Sym.getRawDataRefImpl())->getType();
   if (auto *Elf64BEObj = cast<ELF64BEObjectFile>(Obj))
     return Elf64BEObj->getSymbol(Sym.getRawDataRefImpl())->getType();
   llvm_unreachable("Unsupported binary format");
 }
 
 template <class ELFT> static void
 addDynamicElfSymbols(const ELFObjectFile<ELFT> *Obj,
                      std::map<SectionRef, SectionSymbolsTy> &AllSymbols) {
   for (auto Symbol : Obj->getDynamicSymbolIterators()) {
     uint8_t SymbolType = Symbol.getELFType();
     if (SymbolType == ELF::STT_SECTION)
       continue;
 
     uint64_t Address = unwrapOrError(Symbol.getAddress(), Obj->getFileName());
     // ELFSymbolRef::getAddress() returns size instead of value for common
     // symbols which is not desirable for disassembly output. Overriding.
     if (SymbolType == ELF::STT_COMMON)
       Address = Obj->getSymbol(Symbol.getRawDataRefImpl())->st_value;
 
     StringRef Name = unwrapOrError(Symbol.getName(), Obj->getFileName());
     if (Name.empty())
       continue;
 
     section_iterator SecI =
         unwrapOrError(Symbol.getSection(), Obj->getFileName());
     if (SecI == Obj->section_end())
       continue;
 
     AllSymbols[*SecI].emplace_back(Address, Name, SymbolType);
   }
 }
 
 static void
 addDynamicElfSymbols(const ObjectFile *Obj,
                      std::map<SectionRef, SectionSymbolsTy> &AllSymbols) {
   assert(Obj->isELF());
if (NumBytes == 8) {
       outs() << std::string(IndentOffset, ' ') << "         ";
       outs() << reinterpret_cast<char *>(AsciiData);
       outs() << '\n';
       NumBytes = 0;
     }
   }
 }
 
+SymbolInfoTy CreateSymbolInfoTy(const ObjectFile *Obj,
+                                const SymbolRef &Symbol) {
+  const StringRef FileName = Obj->getFileName();
+  const uint64_t Addr = unwrapOrError(Symbol.getAddress(), FileName);
+  const StringRef Name = unwrapOrError(Symbol.getName(), FileName);
+
+  uint8_t SymbolType = ELF::STT_NOTYPE;
+
+  if (Obj->isELF()) {
+    SymbolType = getElfSymbolType(Obj, Symbol);
+  }
+
+  if (Obj->isXCOFF()) {
+    const int64_t SymbolIndex = getSymbolIndex(Obj, Symbol);
+    const bool SymDescDis = isSymbolDescriptionDisplay(Obj, Symbol);
+    Optional<XCOFF::StorageMappingClass> Smc =
+        getXCOFFSymbolCsectSMC(Obj, Symbol);
+    return SymbolInfoTy(Addr, Name, Smc, SymbolIndex, SymDescDis);
+  } else
+    return SymbolInfoTy(Addr, Name, SymbolType);
+}
+
+SymbolInfoTy CreateSymbolInfoTy(const ObjectFile *Obj, const uint64_t Addr,
+                                StringRef &Name) {
+  if (Obj->isXCOFF())
+    return SymbolInfoTy(Addr, Name, Optional<XCOFF::StorageMappingClass>(),
+                        No_Sym_Index, false);
+  else
+    return SymbolInfoTy(Addr, Name, ELF::STT_NOTYPE);
+}
+
 static void disassembleObject(const Target *TheTarget, const ObjectFile *Obj,
                               MCContext &Ctx, MCDisassembler *PrimaryDisAsm,
                               MCDisassembler *SecondaryDisAsm,
                               const MCInstrAnalysis *MIA, MCInstPrinter *IP,
                               const MCSubtargetInfo *PrimarySTI,
                               const MCSubtargetInfo *SecondarySTI,
                               PrettyPrinter &PIP,
                               SourcePrinter &SP, bool InlineRelocs) {
static void disassembleObject(const Target *TheTarget, const ObjectFile *Obj,
 
   // Create a mapping from virtual address to symbol name.  This is used to
   // pretty print the symbols while disassembling.
   std::map<SectionRef, SectionSymbolsTy> AllSymbols;
   SectionSymbolsTy AbsoluteSymbols;
   const StringRef FileName = Obj->getFileName();
   const MachOObjectFile *MachO = dyn_cast<const MachOObjectFile>(Obj);
   for (const SymbolRef &Symbol : Obj->symbols()) {
-    uint64_t Address = unwrapOrError(Symbol.getAddress(), FileName);
-
     StringRef Name = unwrapOrError(Symbol.getName(), FileName);
     if (Name.empty())
       continue;
 
     uint8_t SymbolType = ELF::STT_NOTYPE;
     if (Obj->isELF()) {
       SymbolType = getElfSymbolType(Obj, Symbol);
       if (SymbolType == ELF::STT_SECTION)
         continue;
     }
 
-    // Don't ask a Mach-O STAB symbol for its section unless you know that 
+    // Don't ask a Mach-O STAB symbol for its section unless you know that
     // STAB symbol's section field refers to a valid section index. Otherwise
     // the symbol may error trying to load a section that does not exist.
     if (MachO) {
       DataRefImpl SymDRI = Symbol.getRawDataRefImpl();
       uint8_t NType = (MachO->is64Bit() ?
                        MachO->getSymbol64TableEntry(SymDRI).n_type:
                        MachO->getSymbolTableEntry(SymDRI).n_type);
       if (NType & MachO::N_STAB)
         continue;
     }
 
     section_iterator SecI = unwrapOrError(Symbol.getSection(), FileName);
     if (SecI != Obj->section_end())
-      AllSymbols[*SecI].emplace_back(Address, Name, SymbolType);
+      AllSymbols[*SecI].push_back(CreateSymbolInfoTy(Obj, Symbol));
     else
-      AbsoluteSymbols.emplace_back(Address, Name, SymbolType);
+      AbsoluteSymbols.push_back(CreateSymbolInfoTy(Obj, Symbol));
   }
+
   if (AllSymbols.empty() && Obj->isELF())
     addDynamicElfSymbols(Obj, AllSymbols);
 
   BumpPtrAllocator A;
   StringSaver Saver(A);
   addPltEntries(Obj, AllSymbols, Saver);
 
   // Create a mapping from virtual address to section.
for (const auto &ExportEntry : COFFObj->export_directories()) {
 
       uint64_t VA = COFFObj->getImageBase() + RVA;
       auto Sec = partition_point(
           SectionAddresses, [VA](const std::pair<uint64_t, SectionRef> &O) {
             return O.first <= VA;
           });
       if (Sec != SectionAddresses.begin()) {
         --Sec;
         AllSymbols[Sec->second].emplace_back(VA, Name, ELF::STT_NOTYPE);
       } else
         AbsoluteSymbols.emplace_back(VA, Name, ELF::STT_NOTYPE);
     }
   }
 
   // Sort all the symbols, this allows us to use a simple binary search to find
   // a symbol near an address.
   StringSet<> FoundDisasmFuncsSet;
for (const SectionRef &Section : ToolSectionFilter(*Obj)) {
     if (MachO) {
       DataRefImpl DR = Section.getRawDataRefImpl();
       SegmentName = MachO->getSectionFinalSegmentName(DR);
     }
 
     StringRef SectionName = unwrapOrError(Section.getName(), Obj->getFileName());
     // If the section has no symbol at the start, just insert a dummy one.
     if (Symbols.empty() || Symbols[0].Addr != 0) {
+      if (Obj->isXCOFF())
+        Symbols.insert(Symbols.begin(),
+                       CreateSymbolInfoTy(Obj, SectionAddr, SectionName));
+      else
         Symbols.insert(
             Symbols.begin(),
             SymbolInfoTy(SectionAddr, SectionName,
-                          Section.isText() ? ELF::STT_FUNC : ELF::STT_OBJECT));
     }
 
     SmallString<40> Comments;
     raw_svector_ostream CommentStream(Comments);
 
     ArrayRef<uint8_t> Bytes = arrayRefFromStringRef(
         unwrapOrError(Section.getContents(), Obj->getFileName()));
 
for (unsigned SI = 0, SE = Symbols.size(); SI != SE; ++SI) {
             End -= 4;
         }
       }
 
       outs() << '\n';
       if (!NoLeadingAddr)
         outs() << format(Is64Bits ? "%016" PRIx64 " " : "%08" PRIx64 " ",
                          SectionAddr + Start + VMAAdjustment);
-
+      if (Obj->isXCOFF() && SymbolDescription) {
+        printXCOFFSymbolDescription(Symbols[SI], SymbolName);
+        outs() << ":\n";
+      } else
         outs() << SymbolName << ":\n";
 
       // Don't print raw contents of a virtual section. A virtual section
       // doesn't have any contents in the file.
       if (Section.isVirtual()) {
         outs() << "...\n";
         continue;
       }