This is an archive of the discontinued LLVM Phabricator instance.

[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

address comment

jhenderson added inline comments.Mar 4 2020, 2:03 AM
llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h
50–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.

53

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

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 ↗(On Diff #247074)

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
64

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
151

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

151–152

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

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

1196

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

llvm/tools/llvm-objdump/llvm-objdump.h
135–142

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

DiggerLin marked 18 inline comments as done.Mar 10 2020, 11:01 AM
DiggerLin added inline comments.
llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h
50–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.

53

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

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 ↗(On Diff #247074)

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

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

thanks

llvm/tools/llvm-objdump/llvm-objdump.h
135–142

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

DiggerLin marked 6 inline comments as done.

address comment

DiggerLin updated this revision to Diff 249614.Mar 11 2020, 7:26 AM

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

jhenderson added inline comments.Mar 13 2020, 2:59 AM
llvm/docs/CommandGuide/llvm-objdump.rst
334

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
53

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
41

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–5 ↗(On Diff #249614)

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
52

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

llvm/tools/llvm-objdump/llvm-objdump.cpp
151–152

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
135–142

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

DiggerLin updated this revision to Diff 250639.Mar 16 2020, 3:24 PM
DiggerLin marked 8 inline comments as done.

address comment

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

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
41

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
135–142

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 added inline comments.Mar 20 2020, 2:03 AM
llvm/docs/CommandGuide/llvm-objdump.rst
328

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

333

for disassembly -> to disassembly output

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

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
41

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
135–142

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 added inline comments.Mar 20 2020, 1:11 PM
llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h
53

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 updated this revision to Diff 252569.Mar 25 2020, 7:35 AM
DiggerLin marked 9 inline comments as done.

address comment

llvm/lib/BinaryFormat/XCOFF.cpp
41

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

MaskRay retitled this revision from using symbol index+symbol name + storage mapping class as label for llvm-objdump -D to [llvm-objdump] Use symbol index+symbol name + storage mapping class as label for -D.Mar 25 2020, 2:59 PM
jhenderson added inline comments.Mar 26 2020, 2:10 AM
llvm/docs/CommandGuide/llvm-objdump.rst
333

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
38

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
42

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 marked 9 inline comments as done.Mar 26 2020, 7:39 AM
DiggerLin added inline comments.
llvm/docs/CommandGuide/llvm-objdump.rst
333

thanks.

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

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 updated this revision to Diff 252846.Mar 26 2020, 7:51 AM
DiggerLin marked 4 inline comments as done.

address comment

DiggerLin marked 12 inline comments as done.Mar 26 2020, 7:58 AM
jhenderson accepted this revision.Mar 27 2020, 3:43 AM

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.

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

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

llvm/include/llvm/Object/XCOFFObjectFile.h
147

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 ↗(On Diff #252846)

@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 added inline comments.Mar 27 2020, 9:15 AM
llvm/test/tools/llvm-objdump/xcoff-disassemble-all.test
5 ↗(On Diff #252846)

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.

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

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
16

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 added inline comments.Mar 27 2020, 12:08 PM
llvm/test/tools/llvm-objdump/xcoff-disassemble-all.test
5 ↗(On Diff #252846)

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

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

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.

28

Please use one space only between words.

30

Can we avoid the cast entirely?

42

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

44

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.

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

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.

62

Please don't call .str() here.

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

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.

1179

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

1182

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
12

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?

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

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

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

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.

24 ↗(On Diff #252846)

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 added inline comments.Mar 30 2020, 12:07 AM
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 updated this revision to Diff 253600.Mar 30 2020, 8:21 AM
DiggerLin marked 3 inline comments as done.

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

DiggerLin marked 21 inline comments as done.Mar 30 2020, 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.Mar 31 2020, 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.Mar 31 2020, 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.Mar 31 2020, 4:27 PM

simplify some code.

jhenderson added inline comments.Apr 1 2020, 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.Apr 1 2020, 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.Apr 1 2020, 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)Apr 1 2020, 10:04 AM
DiggerLin updated this revision to Diff 254264.Apr 1 2020, 12:00 PM
DiggerLin updated this revision to Diff 254319.Apr 1 2020, 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.Apr 1 2020, 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.Apr 1 2020, 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.Apr 1 2020, 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.Apr 2 2020, 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.Apr 2 2020, 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.Apr 2 2020, 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.Apr 2 2020, 9:11 AM
DiggerLin updated this revision to Diff 254544.Apr 2 2020, 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.Apr 3 2020, 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.Apr 3 2020, 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.Apr 6 2020, 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.