Page MenuHomePhabricator

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

Authored by DiggerLin on Jan 17 2020, 7:16 PM.

Details

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.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
DiggerLin marked 21 inline comments as done.Mon, Mar 30, 12:38 PM

address comment

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

thanks

llvm/test/tools/llvm-objdump/xcoff-disassemble-all.test
4 ↗(On Diff #252846)

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

24 ↗(On Diff #252846)

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

llvm/tools/llvm-objdump/XCOFFDump.cpp
62

thanks

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

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

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

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 ?

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

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

llvm/include/llvm/Object/XCOFFObjectFile.h
133

This can now be

static constexpr uint8_t SymbolTypeMask = 0x07u;
134

This is unused now.

llvm/lib/Object/XCOFFObjectFile.cpp
803

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
21

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–15

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

44

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.

64

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 added inline comments.Tue, Mar 31, 12:33 AM
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
12

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 updated this revision to Diff 254017.Tue, Mar 31, 3:15 PM
DiggerLin marked 13 inline comments as done.

address comment

llvm/lib/Object/XCOFFObjectFile.cpp
803

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 updated this revision to Diff 254038.Tue, Mar 31, 4:27 PM

simplify some code.

jhenderson added inline comments.Wed, Apr 1, 12:30 AM
llvm/lib/Object/XCOFFObjectFile.cpp
804

clang-format is still complaining here.

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

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

DiggerLin marked 4 inline comments as done.Wed, Apr 1, 6:19 AM
DiggerLin added inline comments.
llvm/lib/Object/XCOFFObjectFile.cpp
804

thanks

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

thanks

DiggerLin updated this revision to Diff 254179.Wed, Apr 1, 6:29 AM
DiggerLin marked 2 inline comments as done.

address comment

llvm/lib/Object/XCOFFObjectFile.cpp
803

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.

804

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.

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
1178

We can use XCOFFObj instead of casting Obj again.

1222
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
llvm/test/tools/llvm-objdump/XCOFF/disassemble-symbol-description.test
68

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

hubert.reinterpretcast edited the summary of this revision. (Show Details)Wed, Apr 1, 10:04 AM
DiggerLin updated this revision to Diff 254264.Wed, Apr 1, 12:00 PM
DiggerLin updated this revision to Diff 254319.Wed, Apr 1, 2:54 PM
DiggerLin marked an inline comment as done.

modified the test case based on the latest llvm code.

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 marked 4 inline comments as done.Wed, Apr 1, 6:03 PM
DiggerLin added inline comments.
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 marked 3 inline comments as done.Wed, Apr 1, 6:18 PM
DiggerLin added inline comments.
llvm/tools/llvm-objdump/XCOFFDump.cpp
45

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

DiggerLin updated this revision to Diff 254386.Wed, Apr 1, 6:32 PM
llvm/tools/llvm-objdump/XCOFFDump.h
13

What is this header being included from this header for?

14

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.

18

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

25

Same comment about interface header but for StringRef.

29

Ditto SmallVectorImpl.

jhenderson added inline comments.Thu, Apr 2, 12:37 AM
llvm/lib/Object/XCOFFObjectFile.cpp
769

need -> needs (same below)

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

796–797

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

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

In D77285, there's a comment from MaskRay:

Add a new line before namespace llvm

DiggerLin marked 8 inline comments as done.Thu, Apr 2, 6:35 AM
DiggerLin added inline comments.
llvm/lib/Object/XCOFFObjectFile.cpp
769

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
29

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.

llvm/lib/Object/XCOFFObjectFile.cpp
769

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 marked an inline comment as done.Thu, Apr 2, 7:38 AM
hubert.reinterpretcast added inline comments.
llvm/lib/Object/XCOFFObjectFile.cpp
769

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

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
llvm/tools/llvm-objdump/llvm-objdump.cpp
1178
1222
DiggerLin marked 18 inline comments as done.Thu, Apr 2, 9:11 AM
DiggerLin updated this revision to Diff 254544.Thu, Apr 2, 9:25 AM

address comment

llvm/tools/llvm-objdump/XCOFFDump.h
18

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

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

jhenderson added inline comments.Fri, Apr 3, 12:28 AM
llvm/tools/llvm-objdump/XCOFFDump.h
18

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

25

Ditto response.

hubert.reinterpretcast marked 2 inline comments as done.Fri, Apr 3, 6:18 AM
hubert.reinterpretcast added inline comments.
llvm/tools/llvm-objdump/XCOFFDump.h
18

Okay. Thanks.

25

Agreed.

This revision was not accepted when it landed; it landed in state Needs Review.Mon, Apr 6, 7:33 AM
This revision was automatically updated to reflect the committed changes.
llvm/tools/llvm-objdump/XCOFFDump.h
16

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