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
70 msx64 windows > LLVM.CodeGen/PowerPC::aix-emit-tracebacktable-vectorinfo_hasvarg.ll
Script: -- : 'RUN: at line 7'; not --crash c:\ws\w64\llvm-project\premerge-checks\build\bin\llc.exe -verify-machineinstrs -mtriple powerpc-ibm-aix-xcoff -mcpu=pwr7 -mattr=+altivec -vec-extabi -xcoff-traceback-table=true 2>&1 < C:\ws\w64\llvm-project\premerge-checks\llvm\test\CodeGen\PowerPC\aix-emit-tracebacktable-vectorinfo_hasvarg.ll | c:\ws\w64\llvm-project\premerge-checks\build\bin\filecheck.exe --check-prefixes=CHECK-ASM,COMMON C:\ws\w64\llvm-project\premerge-checks\llvm\test\CodeGen\PowerPC\aix-emit-tracebacktable-vectorinfo_hasvarg.ll
80 msx64 windows > LLVM.DebugInfo/XCOFF::explicit-section.ll
Script: -- : 'RUN: at line 2'; c:\ws\w64\llvm-project\premerge-checks\build\bin\llc.exe -mtriple powerpc-ibm-aix-xcoff < C:\ws\w64\llvm-project\premerge-checks\llvm\test\DebugInfo\XCOFF\explicit-section.ll | c:\ws\w64\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w64\llvm-project\premerge-checks\llvm\test\DebugInfo\XCOFF\explicit-section.ll
50 msx64 windows > LLVM.tools/llvm-objdump/XCOFF::disassemble-invalid-traceback-table.test
Script: -- : 'RUN: at line 3'; c:\ws\w64\llvm-project\premerge-checks\build\bin\llvm-objdump.exe -D --traceback-table --symbol-description C:\ws\w64\llvm-project\premerge-checks\llvm\test\tools\llvm-objdump\XCOFF/Inputs/xcoff-invalid-traceback-table.o 2>&1 | c:\ws\w64\llvm-project\premerge-checks\build\bin\filecheck.exe --implicit-check-not warning: --check-prefixes=WARN C:\ws\w64\llvm-project\premerge-checks\llvm\test\tools\llvm-objdump\XCOFF\disassemble-invalid-traceback-table.test
30 msx64 windows > LLVM.tools/llvm-objdump/XCOFF::disassemble-traceback-table.test
Script: -- : 'RUN: at line 3'; c:\ws\w64\llvm-project\premerge-checks\build\bin\llvm-objdump.exe -D --traceback-table --symbol-description C:\ws\w64\llvm-project\premerge-checks\llvm\test\tools\llvm-objdump\XCOFF/Inputs/xcoff-traceback-table.o | c:\ws\w64\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w64\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 retitled this revision from [AIX]{XCOFF] print out the traceback info to [AIX][XCOFF] print out the traceback info .Oct 29 2020, 8:48 AM

I've skimmed some of this, but most of this is XCOFF specific, so other developers are probably better off reviewing it.

llvm/include/llvm/Object/XCOFFObjectFile.h
454 ↗(On Diff #299041)

This change is unrelated to this patch, so should be moved to a separate commit.

llvm/tools/llvm-objdump/XCOFFDump.cpp
146–148

Please report warnings/errors properly, as per what all tools do, rather than print them inline with the disassembly. See also the bit on warning/error guidelines in the coding standards too.

321–324

No need to #undef things at EOF in a cpp surely?

DiggerLin marked 2 inline comments as done.Nov 20 2020, 11:07 AM
DiggerLin added inline comments.
llvm/include/llvm/Object/XCOFFObjectFile.h
454 ↗(On Diff #299041)
llvm/tools/llvm-objdump/XCOFFDump.cpp
321–324

I think we will add more code in the file in future when we add more functionality. . if not have undef now, we maybe forgot to add the undef in future when we add more code in future.

The traceback decode format here is quite different from what binutils objdump would generate on AIX:

$ objdump -D --private=traceback  xcoff-traceback-table.o

xcoff-traceback-table.o:     file format aixcoff-rs6000
00000000: .foo
 tags at 0000008c
 version: 0, lang: 0, global_link: 0, is_eprol: 0, has_tboff: 1, int_proc: 0
 has_ctl: 0, tocless: 0, fp_pres: 1, log_abort: 0, int_hndl: 0
 name_pres: 1, uses_alloca: 0, cl_dis_inv: 0, saves_cr: 0, saves_lr: 0
 stores_bc: 1, fixup: 0, fpr_saved: 0 , spare3: 0, gpr_saved: 1
 fixparms: 2    floatparms: 3    parm_on_stk: 1
 parminfo: 0x5a000000
 tb_offset: 0x00000088 (start=0x00000000)
 Name (len: 3): foo
 (end of tags at 000000a1)


Disassembly of section .text:

00000000 <.foo>
...

I'm not sure if having such a large discrepancy in decode format is a good idea for llvm-objdump.

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

Should we be expecting a non-zero exit code here?

DiggerLin marked 4 inline comments as done.Nov 27 2020, 7:14 AM
DiggerLin added inline comments.
llvm/test/tools/llvm-objdump/XCOFF/disassemble-invalid-traceback-table.test
4

when traceback parse error. we just a warning and raw data of traceback table. not exit and continue to parse other part of the object file.

llvm/tools/llvm-objdump/XCOFFDump.cpp
146–148

we do not want report and errror

146–148

the design purpose as , when parse error on the traceback table, we just emit a warning and print the information out and then continue to parse other part of the xcoff object file.

DiggerLin updated this revision to Diff 308055.Nov 27 2020, 7:41 AM
DiggerLin marked 2 inline comments as done.

address comment

DiggerLin added inline comments.Nov 27 2020, 7:45 AM
llvm/include/llvm/Object/XCOFFObjectFile.h
407 ↗(On Diff #308055)

for the function name , I already has the separate patch for it. https://reviews.llvm.org/D92225 .

jasonliu added inline comments.Nov 27 2020, 10:47 AM
llvm/include/llvm/Object/XCOFFObjectFile.h
407 ↗(On Diff #308055)

Please rebase the patch on D92225 when you have the chance.

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

The = with space is not consistent in the printing. Please only use with one style here.
I think we should choose between

# xx = xxx

or

# xx=xxx

Have a slight preference on the first one.

37

I think it's still a tad too long for this line. Perhaps it's better to move +hasTraceBackTableOffset to the next line.

52

Please print the FunctionNameLen and FunctionName in separate lines.
In this case:

d8: 00 03            # FunctionNameLen = 3
da: 66 6f 6f         # FunctionName = foo
58

It would be great if we could decode this field for user, just like the Language field.

DiggerLin updated this revision to Diff 308110.Nov 27 2020, 1:43 PM
DiggerLin marked 4 inline comments as done.

address comment

jhenderson added inline comments.Nov 30 2020, 12:33 AM
llvm/tools/llvm-objdump/XCOFFDump.cpp
166–171

Use objdump::reportWarning rather than printing to the warning stream directly, I recommend. It does the flushing and printing to the errs() stream itself.

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.

The warning message you are printing itself needs a number of grammar fixes, but I'm not familiar enough with XCOFF terminology to have a reasonable recommendation. Perhaps the following (note included whitespace and capitalization fixes in the message):

Twine("traceback table parsing failure\n>>> ") + toString(TTOrErr.takeError()) + " from the start of the traceback table.\n>>> Raw traceback table data is:"

I'm hesitant about including the "from the start of the traceback table" bit, because it bakes in an assumption about what the underlying error message will look like, which may no longer apply in the future if the message changes. Personally, I'm not sure it's needed, and would be inclined to remove it.

Rather than using hard tabs in the output, I've followed an approach taken from LLD's error message styling too. Hard tabs are bad and should be avoided in my opinion, because the output will appear different depending on people's window settings.

I've omitted the "The value in..." bit, as I'm really not sure what value it's referring to here, and I suspect a user might get more confused by it than not, but if you want to include it, I'd add it as a note after the first line, saying "note: the value in ...".

Note also that reportWarning includes a trailing new line, so there is no need to explicitly add one, just as it also does outs().flush().

jasonliu added inline comments.Nov 30 2020, 11:04 AM
llvm/tools/llvm-objdump/XCOFFDump.cpp
110

This should be 24 for non-X86 platform. Right now, our traceback table printing starts later in the column than other instructions, and this is why.
The other thing is, since this function's content is taken entirely from PrettyPrinter's printInst, have you thought about making this function callable for both the traceback table usage and printInst's usage? For example, rename this function and make it a member function of PrettyPrinter, then pass the PrettyPrinter into dumpTracebackTable. Or just rename it and move it to common code of llvm-objdump, and call this function from printInst.

116

The switch logic here doesn't work. This flag could represent multiply settings at the same time.
For example, it could have TB_LONGTBTABLE2 and TB_SSP_CANARY at the same time.

131

I would imagine this is something we are going to need in the encoding part as well. Should we put this as an enum in XCOFF.h.

142–148

If you don't need the convenient of a macro, could you write them in lambda's instead?
For example, the SPLIT and PRINTOUTBYTES.

153

Call getInstStartColumn(STI) to make the TabStop consistent between TracebackTable printing and other instruction printing.

203

nit: could we add ; to the end of the line even if it's a function-like macro that already had ; at the end?

204

What's the reason for the cast here?
If you really need the cast, please use static_cast instead of a c-style cast.

216

Do you need to pass TbTable into PRINTBOOL and PRINTGET? It seems you always pass TBTable right now, why not just hardcode it then?

271

minor nit: I would prefer this function-like macro to resides with other function-like macro at the beginning of this function.

320

Do you need the cast? If you really do, please use static_cast instead of c style cast.

348

Do you need .data() ?

358–359

if (End - Address + Index >= 8)
Do you actually meant to have if (End - Address - Index >= 8) ?

Also, it might not be safe to skip the bytes printing when the remaining bytes are larger than 8. As the following bytes might not be all zeros. In that case, we should print out the bytes.

DiggerLin marked 14 inline comments as done.Dec 2 2020, 2:11 PM
DiggerLin added inline comments.
llvm/tools/llvm-objdump/XCOFFDump.cpp
142–148

I change the PRINTOUTBYTES to lambda, but I can not see a benefit of change SPLIT from Macro to lambda

166–171

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.

204

OS only accept following
raw_ostream &operator<<(unsigned long N);

raw_ostream &operator<<(long N);
raw_ostream &operator<<(unsigned long long N);
raw_ostream &operator<<(long long N);
raw_ostream &operator<<(const void *P);

raw_ostream &operator<<(unsigned int N) {
  return this->operator<<(static_cast<unsigned long>(N));
}

raw_ostream &operator<<(int N) {
  return this->operator<<(static_cast<long>(N));
}

raw_ostream &operator<<(double N);
216

TBVectorExt uses the macro too

358–359

thanks. done.

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
114

Can this function be static too?

126

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

166–171

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.

204

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
114

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
106

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

164

minor:

203

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;
205

minor: also add a blank line above this line

276

minor:

291

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

322

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

329

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?

338

minor: s/padding/paddings

351

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

llvm/tools/llvm-objdump/llvm-objdump.h
157

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
203

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.

291

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

322

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
157

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
157

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