Page MenuHomePhabricator

implement the --syms and using "symbol index and qualname" for --sym --symbol--description for llvm-objdump for xcoff
ClosedPublic

Authored by DiggerLin on Sep 8 2021, 10:05 AM.

Details

Summary

for xcoff :

  1. implement the getSymbolFlag and getSymbolType() for option --syms.
  2. llvm-objdump --sym , if the symbol is label, print the containing section for the symbol too.
  3. when using llvm-objdump --sym --symbol--description, print the symbol index and qualname for symbol.

for example:
--symbol-description
00000000000000c0 l .text (csect: (idx: 2) .foov[PR]) (idx: 3) .foov

and without --symbol-description
00000000000000c0 l .text (csect: .foov) .foov

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

I'd like other XCOFF people to look at this and decide if the output is a good style for them.

llvm/test/tools/llvm-objdump/XCOFF/symbol-table.test
2

Please add a brief comment explaining what this test is intended to test.

Also, this test won't succeed if the appropriate target is not configured for the person running the test. You need a REQUIRES statement.

I don't know much about llc, but I suspect it's likely that, based purely on the name, the -verify-machineinstrs option isn't needed?

3

When continuing run lines onto the next one, add spacing after the RUN: on the subsequent lines to indent the arguments a bit. This helps readability by tying it to the previous line.

28–29

Should there be a space after the first .text on each of these lines, before the parenthesis?

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

Don't use auto if the type isn't clear (i.e. it isn't mentioned on the line already, in e.g. a cast or constructor). See the style guide.

72

It seems that if a case is necessary here, there's a problem with the function you're calling. At a guess, it should just be two functions, one returning a section, the other an index, with errors or assertions reported if using the wrong one for the type.

If a case is really necessary, use static_cast.

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

This is a problem throughout this header file (and its cpp) already, so please make a small commit to fix it: SymbolRef is designed to be a lightweight struct and passed by value, not by reference (just like StringRef).

llvm/tools/llvm-objdump/llvm-objdump.cpp
2078
2136–2141
2139

Seems to me like you're changing the name to add the description. But then the variable is no longer just the name, which could cause issues in the future.

How about you just print the name before this check, then print the description (inside the if), and finally the new line?

I'd like other XCOFF people to look at this and decide if the output is a good style for them.

Generally, yes. Having the containing csect information with the section information prevents confusion for developers making used of the GNU __section__ attribute in C/C++. The ability to differentiate symbols with the same "unqualified" name by means of adding the storage mapping class qualification in the style of the assembly syntax is also very helpful. Lastly, the index information is useful for cross-referencing purposes.

llvm/test/tools/llvm-objdump/XCOFF/symbol-table.test
37

In keeping with "idx" being not capitalized, "csect" should not be either. Note also that the style used for prose text in the IBM documentation has the capitalized form of "csect" as "CSECT".

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

The containing csect presents as an annotation "within" the section field. It may be friendlier if the delimiters of the section field were more explicit:

00000000000000c0 l [.text (csect: (idx: 2) .foov[PR])] (idx: 3) .foov

and without --symbol-description

00000000000000c0 l [.text (csect: .foov)] .foov
2085–2086

@DiggerLin, at least some XCOFF symbols have size fields. This could be handled in a separate patch if that is your preference (in which case, please add a TODO comment to the code).

@jhenderson, at least for XCOFF, I would prefer to always have the field (even if empty). I am not convinced that having it present only for "common" symbols for non-ELF formats is good.

2139

Maybe if the index was not prefixed, that would work: .foov[PR](idx: 3). --symbol-description already presents with the current ordering in conjunction with, e.g., -Dr.

jhenderson added inline comments.Sep 10 2021, 12:51 AM
llvm/tools/llvm-objdump/llvm-objdump.cpp
2085–2086

At least for ELF format, we are tied by the need to match GNU objdump output as closely as practical. I've no issue it being different for other formats that don't have a GNU objdump implementation they are trying to mirror.

DiggerLin marked 8 inline comments as done.Sep 10 2021, 8:48 AM
DiggerLin added inline comments.
llvm/tools/llvm-objdump/XCOFFDump.cpp
72

there is a description in the XCOFFObjectFile.h

// For getSectionOrLength(),
// 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.
uint64_t getSectionOrLength() const {
  return Entry32 ? getSectionOrLength32() : getSectionOrLength64();
}

for the number of symbols in aix xcoff object file should not large than the max of uint32_t. so cast to unit32_t is safe.

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

thanks

2078

thanks

2085–2086

@hubert.reinterpretcast , the size we mention here is section size?
in the https://www.ibm.com/docs/en/aix/7.2?topic=formats-xcoff-object-file-format

XTY_SD x_scnlen contains the csect length.
XTY_CM x_scnlen contains the csect length.

so we want to display the size of the symbols of the XTY_SD symbols and XTY_CM symbols or only XTY_CM symbols?

2136–2141

thanks

llvm/tools/llvm-objdump/llvm-objdump.cpp
2085–2086

For ELF, the common symbols are displayed with the alignment and the other symbols are displayed with the size. We could do the same as ELF (except sometimes we don't have a size or alignment).

DiggerLin marked 6 inline comments as done.Sep 13 2021, 1:31 PM
DiggerLin added inline comments.
llvm/test/tools/llvm-objdump/XCOFF/symbol-table.test
2

llc is cross compile, I think llc can compiler success in other platform , we do not need REQUIRES.

llvm/tools/llvm-objdump/llvm-objdump.cpp
2085–2086

for the "char c" in global variable, in the common, the x_scnlen of csect auxiliary symbol entry is 1, but the alignment will be 4, so we will show 4 bytes?

llvm/tools/llvm-objdump/llvm-objdump.cpp
2085–2086

Just because the alignment given by the various compilers is surprising doesn't mean that we should avoid indicating the alignment.

jhenderson added inline comments.Sep 14 2021, 12:21 AM
llvm/test/tools/llvm-objdump/XCOFF/symbol-table.test
2

llc may be able to cross compile, but it requires to be built with the specified target enabled in LLVM_TARGETS_TO_BUILD, otherwise it doesn't know how to generate the machine code for the appropriate target.

Try regenerating your LLVM build tree with LLVM_TARGETS_TO_BUILD=X86 (and nothing else), and I bet you this test will fail, due to llc not knowing about the powerpc target in this case. You need the REQUIRES.

Also, don't forget my other comments in this test.

3

This is marked as done, but has not been fixed. Please don't mark items as done until you upload a patch that addresses them. Same goes throughout.

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

If I hand-edited (or crafted with yaml2obj) an object file, could I end up with an invalid index that is larger than uint32_t, or is the field size only uint32_t?

Regardless, just because it's safe doesn't mean you need to do it. As far as I can tell, you could change getSymbolByIndex to take a uint64_t without impact, and you wouldn't need the cast.

DiggerLin marked 9 inline comments as done.Sep 15 2021, 9:49 AM
DiggerLin added inline comments.
llvm/test/tools/llvm-objdump/XCOFF/symbol-table.test
2

there is a file

llvm/test/tools/llvm-objdump/XCOFF/lit.local.cfg

which has following content

if not 'PowerPC' in config.root.targets:

config.unsupported = True

it protect the test case in the llvm/test/tools/llvm-objdump/XCOFF not run for no PowerPC.

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

the field size only uint32_t

there is
support::ubig32_t NumberOfSymTableEntries;
in both "struct XCOFFFileHeader64" and "struct XCOFFFileHeader32"

if the hand-edited (or crafted with yaml2obj) an object file , the high order of 32bit will be ignored.

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

I think we should make a separate patch do deal the problem after the patch upload.

DiggerLin marked 3 inline comments as done.

address comment and implement function getSymbolFlag() and getSymbolType() ,getSymbolAlignment() etc

This patch seems to have gained a lot of functionality that sounds like it belongs in separate patches? The patch description and summary talk about reporting the symbol index, the qualname, and the symbol section. Other fields should be done in another patch, I'd think. There's no need for a single patch to implement all necessary functionality needed for complete symbol table printing.

llvm/lib/Object/XCOFFObjectFile.cpp
230

consumeError is often a code smell. You should either 1) use cantFail instead, 2) add a TODO comment saying to report the message up the stack, 3) report the error somehow, or 4) add a comment explaining why the consumeError is justified here (e.g. "we don't care about invalid files for this purpose"). I suspect at this time 2) is the appropriate response.

Same comment applies for the other consumeError instances.

llvm/test/tools/llvm-objdump/XCOFF/symbol-table.test
2

Thanks. Didn't realise that.

2

We usually describe what the test does (and why where necessary - not applicable here) not what the test is: all the files in this directory are for testing llvm-objdump, so the comment can be simplified somewhat.

8
llvm/tools/llvm-objdump/XCOFFDump.cpp
72

Thanks. Please clang-format.

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

I'm happy as long as it gets down.

DiggerLin retitled this revision from using symbol index and qualname for --sym --symbol--description for llvm-objdump for xcoff to implement the --syms and using "symbol index and qualname" for --sym --symbol--description for llvm-objdump for xcoff.Sep 16 2021, 7:13 AM
DiggerLin edited the summary of this revision. (Show Details)
DiggerLin marked 5 inline comments as done.Sep 16 2021, 8:28 AM

This patch seems to have gained a lot of functionality that sounds like it belongs in separate patches? The patch description and summary talk about reporting the symbol index, the qualname, and the symbol section. Other fields should be done in another patch, I'd think. There's no need for a single patch to implement all necessary functionality needed for complete symbol table printing.

yes, this is a big patch. Since I have implemented all the functionality in one patch, I do not think it is a good ideal to separate the patch to two patches now. I changed the title of the patch and the summary of the patch as your suggestion. thanks.

llvm/lib/Object/XCOFFObjectFile.cpp
230

thanks

llvm/test/tools/llvm-objdump/XCOFF/symbol-table.test
2

thanks

8

thanks

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

thanks

DiggerLin updated this revision to Diff 372957.Sep 16 2021, 8:30 AM
DiggerLin marked 4 inline comments as done.
jhenderson added inline comments.Sep 17 2021, 12:29 AM
llvm/test/tools/llvm-objdump/XCOFF/symbol-table.test
3

Sorry, should have said this before.

112

Nit, don't have a completely empty blank line like this here (do keep the single '\n' at the end of the last line with text to avoid the "no new line at end of file" note).

DiggerLin updated this revision to Diff 373209.Sep 17 2021, 6:47 AM
DiggerLin marked 2 inline comments as done.
DiggerLin added inline comments.
llvm/test/tools/llvm-objdump/XCOFF/symbol-table.test
3

thanks

112

thanks

Some mostly stylistic comments. Beyond that, I've got not much else to add. @hubert.reinterpretcast will need to review the actual XCOFF implementation and corresponding code coverage.

llvm/lib/Object/XCOFFObjectFile.cpp
280

"considerated" isn't a word. I assume you mean "considered", but I think "treated" would be better. Same applies before.

284
806

consumeError usage needs explanation/TODO etc, as before.

llvm/test/tools/llvm-objdump/XCOFF/print-linenumber.test
21

Mildly surprising there's a test change here. Why is there this change?

llvm/tools/llvm-objdump/llvm-objdump.cpp
2054
  1. Be consistent in your capitalization.
  2. If an abbreviation is needed, "Sym" is the usual abbreviation, not "Symb".

Consequently, I'd rename this variable "XCOFFSym" or, if possible "XCOFFSymbol", depending on whether the latter clashes with a type somewhere or not.

2074–2075

This will result in a hard error, and stopping of any further dumping of the file, if there's a problem getting the name. It seems to me like this should actually be reported as a warning, and just keep going, using some placeholder string instead (e.g. "<?>") for the invalid name.

DiggerLin marked 6 inline comments as done.Sep 20 2021, 6:59 AM
DiggerLin added inline comments.
llvm/lib/Object/XCOFFObjectFile.cpp
284

thanks

806

thanks

llvm/test/tools/llvm-objdump/XCOFF/print-linenumber.test
21

for we implement the getSymbolFlag() and getSymbolType() in the patch. after implement the getSymbolType() , the symbol ".main" is marked as function (it is correct to mark .main as function, the symbol "main" is function description in aix). so the here change to .main(), @Esme , can you help to confirm it ?

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

thanks

2074–2075

I think we need to deal with the error as same as section name in the code

"

  if (!SegmentName.empty())
    outs() << SegmentName << ",";
  StringRef SectionName = unwrapOrError(Section->getName(), FileName);
  outs() << SectionName;
}

"

DiggerLin updated this revision to Diff 373576.Sep 20 2021, 7:05 AM
DiggerLin marked 5 inline comments as done.
jhenderson added inline comments.Sep 21 2021, 12:31 AM
llvm/tools/llvm-objdump/llvm-objdump.cpp
2054

Please make the same change throughout your changed code (there are several other places with the old variable name).

2074–2075

I'd suggest the SectionName is in a similar boat - a failure there shouldn't prevent dumping the rest, and should instead probably be printed as a warning. You could do something similar to what I've suggested for the symbol. You can change the section name error handling in a different patch.

DiggerLin marked 3 inline comments as done.Sep 21 2021, 9:57 AM
DiggerLin added inline comments.
llvm/tools/llvm-objdump/llvm-objdump.cpp
2054

done, thanks

DiggerLin updated this revision to Diff 373973.Sep 21 2021, 9:57 AM
DiggerLin marked an inline comment as done.

I've got nothing else to add at this point. It needs an XCOFF developer to review the functional behaviour for correctness.

jsji added a reviewer: Restricted Project.Wed, Sep 29, 7:48 PM
Esme accepted this revision.Thu, Sep 30, 2:07 AM

LGTM. Thanks for doing this.

llvm/test/tools/llvm-objdump/XCOFF/print-linenumber.test
21

Yes, I think the change is reasonable.
I'm trying to figure out why the output function name changed. By tracing the output of line ;.main():, I found that after applying this patch, we override the default LineInfo.FunctionName with the name from symbol table (see function SymbolizableObjectFile::symbolizeCode() ). And I believe it was correct to do so.

This revision is now accepted and ready to land.Thu, Sep 30, 2:07 AM
This revision was landed with ongoing or failed builds.Fri, Oct 1, 9:38 AM
This revision was automatically updated to reflect the committed changes.