Page MenuHomePhabricator

[llvm-readobj][XCOFF] dump auxiliary symbols.
ClosedPublic

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

Details

Summary

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

Diff Detail

Event Timeline

Esme created this revision.Nov 12 2021, 10:31 PM
Esme requested review of this revision.Nov 12 2021, 10:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 12 2021, 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.Nov 23 2021, 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.Nov 24 2021, 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.Nov 25 2021, 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.Nov 29 2021, 10:05 PM
Esme marked 7 inline comments as done.

Address comments.

jhenderson added inline comments.Nov 30 2021, 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?

Esme updated this revision to Diff 390899.Nov 30 2021, 9:21 PM

Address comments.

jhenderson added inline comments.Dec 2 2021, 12:55 AM
llvm/lib/Object/XCOFFObjectFile.cpp
1116–1117

Is "should" appropriate here? Is it never a function? Or is it potentially a function that we can't identify? If the former, delete the word. If the latter, replace the phrase "then this symbol should not be a function" with "then treat this symbol as if it isn't a function".

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

No need for else after continue

572–574

There are several checks identical to this one. I think it would be useful to pull them into a helper lambda that takes a C_* enum and reports the message.

Esme updated this revision to Diff 391995.Dec 6 2021, 1:45 AM

Address comments.

jhenderson accepted this revision.Dec 9 2021, 6:05 AM

LGTM, but please wait for @Higuoxing too.

This revision is now accepted and ready to land.Dec 9 2021, 6:05 AM
Higuoxing accepted this revision.Dec 9 2021, 7:02 AM
This revision was landed with ongoing or failed builds.Tue, Jan 11, 11:19 PM
This revision was automatically updated to reflect the committed changes.

This breaks tests on windows: http://45.33.8.238/win/52389/step_11.txt

Please take a look and revert for now if it takes a while to fix.

jhenderson added inline comments.Wed, Jan 12, 3:55 AM
llvm/test/tools/llvm-readobj/XCOFF/symbols-invalid.test
6

The problem is a missing {{(.exe)?}} after the tool name. Usually, we don't include tool names in the check, and just check from "warning:" onwards.

Applies throughout this test.

thakis added inline comments.Wed, Jan 12, 6:16 AM
llvm/test/tools/llvm-readobj/XCOFF/symbols-invalid.test
6

Hm, normally we don't print "toolname.exe: " in diags on Windows, but just "toolname: ". We usually also don't print the full path to the tool but just the basename.

Maybe the Right Fix is to fix the diag output in llvm-readobj instead?

6

(…but that seems admittedly mostly independent of this patch here. So for relanding, just fixing the test expectations is fine too.)

(Reverted for now.)