Page MenuHomePhabricator

[llvm-readobj][XCOFF] dump auxiliary symbols.
Needs ReviewPublic

Authored by Esme on Fri, Nov 12, 10:31 PM.

Details

Reviewers
shchenz
qiucf
jhenderson
jsji
Higuoxing
Group Reviewers
Restricted Project
Summary

The patch adds support for dumping auxiliary symbols in llvm-readobj for XCOFF.

Diff Detail

Event Timeline

Esme created this revision.Fri, Nov 12, 10:31 PM
Esme requested review of this revision.Fri, Nov 12, 10:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Nov 12, 10:31 PM

I recommend separating the migration to YAML from binaries into a separate successor patch, to avoid confusion.

llvm/test/tools/llvm-readobj/XCOFF/symbols-invalid.test
6

How about "a non-function C_EXT/C_WEAKEXT/C_HIDEXT symbol should have only..."?

You probably should have this test case for each of the three different types. If viable, I'd also dynamically change the warning to explicitly state the type of the symbol (i.e. something like "a non-function C_EXT symbol ..." and "a non-function C_WEAKEXT symbol ..." etc).

Finally, ideally you'd incldue the symbol's name if possible in this warning, since it's not necessarily clear out-of-context which symbol this warning refers to. Applies to each of the other warnings too.

20

Nit: double blank line.

30

Same as above: test both cases, adn consider being explicity about which particular type of symbol this is.

32

Why specifically the file auxiliary entry and not others?

llvm/tools/llvm-readobj/XCOFFDumper.cpp
298
436

Why do we only check this in the debug case? It seems like it's something that's applicable for all cases?

441

Unless you're explicitly calling printf (which you aren't) don't call a function based on that name.

511–514

Skip doing what?

Esme updated this revision to Diff 389158.Tue, Nov 23, 4:10 AM
Esme marked 6 inline comments as done.
Esme edited the summary of this revision. (Show Details)

Move the migration to YAML from binaries into a separate patch D114434.

llvm/test/tools/llvm-readobj/XCOFF/symbols-invalid.test
6

Thanks!

32

This is a case from the deleted file llvm/test/tools/llvm-readobj/XCOFF/file-aux-wrong64.test

Higuoxing added inline comments.Wed, Nov 24, 3:18 AM
llvm/include/llvm/Object/XCOFFObjectFile.h
375–379

It looks that the variables' name are not consistent across this file? Is there any particular reason for that? e.g., Why use ReservedZeros_1 instead of ReservedZeros1 and LineNum_Hi instead of LineNumLo?

llvm/test/tools/llvm-readobj/XCOFF/symbols-invalid.test
63–66

Looks that the indention is broken here?

llvm/tools/llvm-readobj/XCOFFDumper.cpp
342–343

Nit: insert a blank space between the field name and the parenthesis.

359

Nit: blank line.

jhenderson added inline comments.Thu, Nov 25, 1:10 AM
llvm/test/tools/llvm-readobj/XCOFF/symbols-invalid.test
6

It looks like you haven't addressed the first part of this comment?

llvm/tools/llvm-readobj/XCOFFDumper.cpp
513

Should have spotted this earlier, but it should be "at index N" not "with index N". Same goes for the other message(s).

594

I wonder if a simple function that converts enum values to strings would be useful here and elsewhere that you do similar things? I believe there are similar things in ELF.

Esme updated this revision to Diff 390567.Mon, Nov 29, 10:05 PM
Esme marked 7 inline comments as done.

Address comments.

jhenderson added inline comments.Tue, Nov 30, 12:37 AM
llvm/test/tools/llvm-readobj/XCOFF/symbols-invalid.test
52

You can leave the # in the right place, and just add spaces to pad.

llvm/tools/llvm-readobj/XCOFFDumper.cpp
67

I'm slightly surprised this doesn't already exist in our code? Have you copied it from somewhere? If so, where, and could you just move such a function declaration into a common header for reuse?