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 3'; /mnt/disks/ssd0/agent/llvm-project/build/bin/llvm-objdump -D --traceback-table --symbol-description /mnt/disks/ssd0/agent/llvm-project/llvm/test/tools/llvm-objdump/XCOFF/Inputs/xcoff-invalid-traceback-table.o 2>&1 | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck --check-prefixes=CHECH,WARN /mnt/disks/ssd0/agent/llvm-project/llvm/test/tools/llvm-objdump/XCOFF/disassemble-invalid-traceback-table.test
20 msx64 debian > LLVM.tools/llvm-objdump/XCOFF::disassemble-traceback-table.test
Script: -- : 'RUN: at line 3'; /mnt/disks/ssd0/agent/llvm-project/build/bin/llvm-objdump -D --traceback-table --symbol-description /mnt/disks/ssd0/agent/llvm-project/llvm/test/tools/llvm-objdump/XCOFF/Inputs/xcoff-traceback-table.o | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table.test
100 msx64 windows > Clang.CodeGenCXX::weak-extern-typeinfo.cpp
Script: -- : 'RUN: at line 1'; c:\ws\w16n2-1\llvm-project\premerge-checks\build\bin\clang.exe -cc1 -internal-isystem c:\ws\w16n2-1\llvm-project\premerge-checks\build\lib\clang\12.0.0\include -nostdsysteminc C:\ws\w16n2-1\llvm-project\premerge-checks\clang\test\CodeGenCXX\weak-extern-typeinfo.cpp -emit-llvm -triple x86_64-pc-windows-gnu -o - | c:\ws\w16n2-1\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w16n2-1\llvm-project\premerge-checks\clang\test\CodeGenCXX\weak-extern-typeinfo.cpp
60 msx64 windows > Clang.SemaCXX::typeid-ref.cpp
Script: -- : 'RUN: at line 1'; c:\ws\w16n2-1\llvm-project\premerge-checks\build\bin\clang.exe -cc1 -internal-isystem c:\ws\w16n2-1\llvm-project\premerge-checks\build\lib\clang\12.0.0\include -nostdsysteminc -triple x86_64-pc-windows-gnu -emit-llvm -o - C:\ws\w16n2-1\llvm-project\premerge-checks\clang\test\SemaCXX\typeid-ref.cpp | c:\ws\w16n2-1\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w16n2-1\llvm-project\premerge-checks\clang\test\SemaCXX\typeid-ref.cpp
50 msx64 windows > LLVM.tools/llvm-objdump/XCOFF::disassemble-invalid-traceback-table.test
Script: -- : 'RUN: at line 3'; c:\ws\w16n2-1\llvm-project\premerge-checks\build\bin\llvm-objdump.exe -D --traceback-table --symbol-description C:\ws\w16n2-1\llvm-project\premerge-checks\llvm\test\tools\llvm-objdump\XCOFF/Inputs/xcoff-invalid-traceback-table.o 2>&1 | c:\ws\w16n2-1\llvm-project\premerge-checks\build\bin\filecheck.exe --check-prefixes=CHECH,WARN C:\ws\w16n2-1\llvm-project\premerge-checks\llvm\test\tools\llvm-objdump\XCOFF\disassemble-invalid-traceback-table.test
View Full Test Results (6 Failed)

Event Timeline

DiggerLin created this revision.Oct 8 2020, 8:05 AM
Herald added a project: Restricted Project. · View Herald Transcript
DiggerLin requested review of this revision.Oct 8 2020, 8:05 AM
DiggerLin updated this revision to Diff 299041.Oct 19 2020, 7:08 AM
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.Fri, Nov 20, 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.Fri, Nov 27, 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.Fri, Nov 27, 7:41 AM
DiggerLin marked 2 inline comments as done.

address comment

DiggerLin added inline comments.Fri, Nov 27, 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.Fri, Nov 27, 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.Fri, Nov 27, 1:43 PM
DiggerLin marked 4 inline comments as done.

address comment

jhenderson added inline comments.Mon, Nov 30, 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.Mon, Nov 30, 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.Wed, Dec 2, 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.Wed, Dec 2, 2:31 PM
DiggerLin marked 5 inline comments as done.

address comment

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