Page MenuHomePhabricator

[AIX][XCOFF] print out the traceback info
Needs ReviewPublic

Authored by DiggerLin on Oct 8 2020, 8:05 AM.

Details

Summary

1 .print out the traceback info without vector info

  1. if there is need of original object file of xcoff-invalid-traceback-table.o and xcoff-traceback-table.o when review, I send the the object file through email.

Diff Detail

Unit TestsFailed

TimeTest
40 msx64 debian > LLVM.tools/llvm-objdump/XCOFF::disassemble-invalid-traceback-table.test
Script: -- : 'RUN: at line 4'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/llvm-objdump -D --traceback-table --symbol-description /var/lib/buildkite-agent/builds/llvm-project/llvm/test/tools/llvm-objdump/XCOFF/Inputs/xcoff-invalid-traceback-table.o 2>&1 | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck --implicit-check-not warning: --check-prefixes=WARN /var/lib/buildkite-agent/builds/llvm-project/llvm/test/tools/llvm-objdump/XCOFF/disassemble-invalid-traceback-table.test
70 msx64 debian > LLVM.tools/llvm-objdump/XCOFF::disassemble-traceback-table.test
Script: -- : 'RUN: at line 3'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/llvm-objdump -D --traceback-table --symbol-description /var/lib/buildkite-agent/builds/llvm-project/llvm/test/tools/llvm-objdump/XCOFF/Inputs/xcoff-traceback-table.o | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table.test
40 msx64 windows > LLVM.tools/llvm-objdump/XCOFF::disassemble-invalid-traceback-table.test
Script: -- : 'RUN: at line 4'; c:\ws\w7\llvm-project\premerge-checks\build\bin\llvm-objdump.exe -D --traceback-table --symbol-description C:\ws\w7\llvm-project\premerge-checks\llvm\test\tools\llvm-objdump\XCOFF/Inputs/xcoff-invalid-traceback-table.o 2>&1 | c:\ws\w7\llvm-project\premerge-checks\build\bin\filecheck.exe --implicit-check-not warning: --check-prefixes=WARN C:\ws\w7\llvm-project\premerge-checks\llvm\test\tools\llvm-objdump\XCOFF\disassemble-invalid-traceback-table.test
50 msx64 windows > LLVM.tools/llvm-objdump/XCOFF::disassemble-traceback-table.test
Script: -- : 'RUN: at line 3'; c:\ws\w7\llvm-project\premerge-checks\build\bin\llvm-objdump.exe -D --traceback-table --symbol-description C:\ws\w7\llvm-project\premerge-checks\llvm\test\tools\llvm-objdump\XCOFF/Inputs/xcoff-traceback-table.o | c:\ws\w7\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w7\llvm-project\premerge-checks\llvm\test\tools\llvm-objdump\XCOFF\disassemble-traceback-table.test

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
DiggerLin added inline comments.Dec 2 2020, 2:11 PM
llvm/tools/llvm-objdump/XCOFFDump.cpp
173–178

There is filename in the reportWarning in the llvm-objdump, I do not want the file name show in the warning message . I added a new function "AddOffsetToValueInString" to convert the error message with the address from begin of object file.

DiggerLin updated this revision to Diff 309062.Dec 2 2020, 2:31 PM
DiggerLin marked 5 inline comments as done.

address comment

DiggerLin updated this revision to Diff 309247.Dec 3 2020, 6:53 AM

Please make sure you've run clang-format and fix all clang-tidy warnings in your new code.

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

Can this function be static too?

133

Please address these clang-tidy warnings by following LLVM coding standards.

173–178

If for some reason, you want to deliberately differ to what llvm-objdump does normally when printing warnings and not print the tool name, you could add an optional argument to reportWarning to suppress it.

You can follow this same technique to not print the file name too. Why don't you want the file name in the message though? If the error stream is being redirected to a file, you won't know where the warning is coming from if you are working with multiple files or archives.

Also, please fix the other whitespace and grammar issues, as I requested in my previous comment.

211

raw_ostream &operator<<(unsigned char C); exists. Take a look at raw_ostream.h. However, it may print the value as a character, not as an integer. How about using the format function to format your strings, using printf formatting, and PRIx8 to ensure the right type is being used without the need for casting? DWARFDebugLine.cpp has some good examples of that sort of thing.

DiggerLin marked 4 inline comments as done.Dec 22 2020, 9:19 AM
DiggerLin added inline comments.
llvm/tools/llvm-objdump/XCOFFDump.cpp
121

I still use the const std::string &StrMsg as parameter here, for int Num = std::stoi(NumString, 0, 16) and Res += std::string(StrMsg, PrePos, Pos - PrePos); we still need to convert the StringRef to std:string.

DiggerLin updated this revision to Diff 313375.Dec 22 2020, 9:38 AM
DiggerLin marked an inline comment as done.

addressed comment and rebased the patch on the https://reviews.llvm.org/D93659 [AIX][XCOFF] emit vector info of traceback table.

Xiangling_L added inline comments.
llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table.test
4

I noticed that, on AIX, we can do:
objdump --private=traceback xcoff-traceback-table.o to print out traceback table alone.
But with llvm-objdump, we are not possible to do that since we emit traceback table info alone with each byte.
I am wondering are we intentional to not support using --private=traceback alone with llvm-objdump?

8

Should we use xlclang here?

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

minor: Please use ++Pos to avoid extra copy of variable value.

171

minor:

210

I am wondering what's the point of getting a uint8_t and cast it to TracebackTable::LanguageID? Can we adjust the interface to the following?

TracebackTable::LanguageID getLanguageID() const;
212

minor: also add a blank line above this line

283

minor:

298

Is that intentional that we omit printing out function name after the first no more than 4 bytes?

329

This line is causing build failure when I build. Should it be getVectorParmsInfoString?

336

The same question as Jason already mentioned? Please use c++ style cast static_cast if you have to. And also is that possible to adjust the return value of getExtensionTable() to ExtendedTBTableFlag enum type?

345

minor: s/padding/paddings

358

Can we add testcase to get line 351 - line 359 get tested?

llvm/tools/llvm-objdump/llvm-objdump.h
172

May I ask why we don't put these two functions in objdump namespace as well?

DiggerLin marked 12 inline comments as done.Mar 9 2021, 10:18 AM
DiggerLin added inline comments.
llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table.test
4

llvm-objdump do not support --private= now.

8

yes, of course, we can use xlclang here, but I think using xlc is ok too.

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

of course , we can do as your suggestion. but I prefer use raw data type uint8_t as return type of getLanguageID() , and if we change the interface , there still need static_cast<TracebackTable::LanguageID> in the getLanguageID() , if you strong suggest it, I can change the interface.

298

We do not omit printing out the function name after the first no more 4 bytes.
what we do here is:
For hex, we print every four bytes in a line for function name . we print out whole function name in string in the first line of hex format as comment.
for example:

CHECK-NEXT: da: 66 6f 6f 6f # FunctionName = fooooo
CHECK-NEXT: de: 6f 6f
CHECK-NEXT: e0: 1f # AllocaRegister = 31

329

the patch has parent patch. https://reviews.llvm.org/D93659 , I think need to apply the parent patch before compile this patch.

llvm/tools/llvm-objdump/llvm-objdump.h
172

the call stack as
PrettyPrinter::printInst() ->printRawData() ->getInstStartColumn()

the class PrettyPrinter is defined in anonymous namespace and these two new function only used by the class PrettyPrinter. So I define two function as the same namespace as PrettyPrinter .

DiggerLin marked 6 inline comments as done.Mar 9 2021, 1:11 PM
DiggerLin added inline comments.
llvm/tools/llvm-objdump/llvm-objdump.h
172

sorry . I change the above comment
these two new function only used by the class PrettyPrinter
to
these two new function is used by the class PrettyPrinter

DiggerLin updated this revision to Diff 329453.Mar 9 2021, 1:12 PM

address comment

DiggerLin updated this revision to Diff 370034.Sep 1 2021, 1:16 PM
  1. rebased the patch.
  2. add eh_info decode of traceback table.
  3. since add two additional padding bytes after ext vector info when encode the traceback table, adjust the decode based on the new encode.
jhenderson added inline comments.Sep 2 2021, 1:37 AM
llvm/lib/Object/XCOFFObjectFile.cpp
1203
1204–1206

I think you should be able to use the alignTo function to do this alignment calculation. However, it looks like the Cursor and DataExtractor classes are missing any form of seek function to set the position directly, without needing to calculate from the current location. I think it would be useful to add this function to one or the other. You could then do something like one of the following:

Cur.seek(alignTo(Cur.tell(), 4));
DE.seek(Cur, alignTo(Cur.tell(), 4));

which I'm sure you'll agree is much clearer and less likely to have any issues.

llvm/test/CodeGen/PowerPC/aix-emit-tracebacktable-clobber-register.ll
9

Is there any particular reason you can't fold this in with the other cases?

11
12

Use - not _ in your prefix names, as per the prevailing style in this file.

106

This won't work. NEXT suffixes must be prefixed by a - not _.

148

Delete blank lines at file end.

llvm/test/tools/llvm-objdump/XCOFF/disassemble-invalid-traceback-table.test
2
  1. This test needs additional comments explaining what the test is trying to test. How is the input invalid?
  2. You should invest time in adding functionality for writing traceback tables to yaml2obj, so you can use it to generate the invalid output, rather than checking in an opaque and impossible to maintain binary (most people won't have access to the xlc compiler, for example, should there be a problem with the test).
15

Here and below.

25

Get rid of additional blank lines.

llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table.test
8

See my above comments - add traceback table generation to yaml2obj rather than adding canned binaries that people are unable to update.

llvm/tools/llvm-objdump/ObjdumpOpts.td
53

Aside from the also incorrect --symbol-description help text, the help text for llvm-objdump options does not include a trailing full stop. Please remove.

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

Don't use std::stringstream. Use the LLVM raw_string_ostream class instead.

121

There are plenty of LLVM functions for string concatenation and manipulation using Twine and StringRef. Please use those, not std::string. Convert only to a std::string on return.

If you haven't already, please read the LLVM Programmer's Manual, especially the section here: https://llvm.org/docs/ProgrammersManual.html#passing-strings-the-stringref-and-twine-classes

That all being said, I don't think you should do any of this - see my comment below.

149–155

Macros are a pain to debug typically, if nothing else, and could cause problems later in the file (unlikely, but possible), unless you undef them.

166

So I really don't like the need to parse the error message for numbers and updating them. Aside from anything else, it's possible that a future message will include other numbers in it, unrelated to the address, so this function would incorrectly change them.

I assume you want to make it clear that the error message offsets are relative to the start of the traceback table. I think the following would be a way of clarifying:
"failure parsing traceback table with address 0x1234\n>>> " (etc)
And then leave the remaining numbers untouched. This helps because I think in most cases, it will be fairly obvious that the offsets in the error messages are relative to the address, because they won't make sense otherwise (e.g. they'll be smaller than the table's address). The raw data will also have the leading address values, which again will not usually line up with the offset, although you could also consider having the leading address actually be the offset value within the traceback table instead, whichever is simpler.

Finally, I don't think we need to worry too much about any risk of confusion - hopefully this warning will never be seen by users, and when it is, it should be straightforward enough to figure things out from context. Therefore, I think we can afford to keep things simple.

172

Remember to put a full stop at the end of sentences.

199

Shorter and more grammatically correct name. You "print" something usually, not "print out" something.

285
290
338
353
362

Why do we need another 4 bytes printed here? Explain why in comments, not what (it is clear from the code what is happening).

llvm/tools/llvm-objdump/llvm-objdump.h
172

These functions are in a public header. Anybody who includes "llvm-objdump.h" whill get these two functions without any form of namespace scoping which is very undesirable.

Either 1) if the functions are only used in one file, move their definition into that file, or 2) if the functions are used in multiple places, put them in the llvm namespace at the bare minimum (and since you are in objdump, they should be in the llvm::objdump namespace.

DiggerLin updated this revision to Diff 371718.Sep 9 2021, 1:47 PM
DiggerLin marked 23 inline comments as done.

address comment

jhenderson added inline comments.Sep 10 2021, 1:04 AM
llvm/test/tools/llvm-objdump/XCOFF/disassemble-invalid-traceback-table.test
1

I repeat my previous comment. How is this invalid?

llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table.test
8

Thsi comment is marked as done, but hasn't been addressed as far as I can see?

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

I assume?

175

Lambdas are objects, so should be named as such.

338–339
DiggerLin added inline comments.Sep 10 2021, 6:32 AM
llvm/lib/Object/XCOFFObjectFile.cpp
1204–1206

good idea, thanks. I will create a separate patch for it. https://reviews.llvm.org/D109603 . after the patch approve, I will change the code based on the new patch.

llvm/test/CodeGen/PowerPC/aix-emit-tracebacktable-clobber-register.ll
9

these one generate the object file. other generate asm.

12

thanks

106

thanks

llvm/test/tools/llvm-objdump/XCOFF/disassemble-invalid-traceback-table.test
2

agree with you. but not this moment, the yaml2obj not support the auxiliary entry for the symbol table, parsing traceback table depend on it.

in the XCOFFEmitter.cpp : line 332
"// TODO: Auxiliary entry is not supported yet."

llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table.test
8

agree with you. same comment as above.

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

thanks

199

thanks

DiggerLin added inline comments.Sep 10 2021, 6:46 AM
llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table.test
8

I will add some new functionality to XCOFFEmitter.cpp before I can create a xcoff object file I can use here.