[AIX][XCOFF] print out the traceback info

[DiggerLin]

Summary

1 .print out the traceback info

2. 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.

[jhenderson]

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

144

I assume?

192

Lambdas are objects, so should be named as such.

355–356

[DiggerLin]

llvm/lib/Object/XCOFFObjectFile.cpp

1490–1492

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

111

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

189

thanks

216

thanks

[DiggerLin]

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.

[DiggerLin]

llvm/tools/llvm-objdump/XCOFFDump.cpp

144

yes, thanks

[DiggerLin]

addressed comment , rebased the patch , changed the test case from using canned xcoff object file to using yaml2obj.

[jhenderson]

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

2–3

Also reflow this comment to the typical 80 character width.

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

2

Also reflow this comment.

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

1734–1735

Perhaps worth pulling Symbols[SI - 1].XCOFFSymInfo.StorageMappingClass into a helper variable, rather than repeating it.

[DiggerLin]

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

1734–1735

the Symbols[SI - 1] has XCOFFSymInfo only when Obj.isXCOFF(), if I put Symbols[SI - 1].XCOFFSymInfo.StorageMappingClass into a helper variable, it will the code complicate.

I maybe need to change the code as

bool DumpTracebackTableForXCOFFFunction = false;
if( Obj.isXCOFF() && Section.isText() && TracebackTable) {
  Optional<XCOFF::StorageMappingClass>  Smc = Symbols[SI - 1].XCOFFSymInfo.StorageMappingClass;

if (Smc && value() ==XCOFF::XMC_PR) 
   DumpTracebackTableForXCOFFFunction = true;
}

[DiggerLin]

address James' comment

[jhenderson]

I don't feel confident enough to be able to say whether various cases have been tested or not. I'd like to leave that to someone with more XCOFF knowledge.

llvm/lib/BinaryFormat/XCOFF.cpp

179–180 ↗

(On Diff #478284)

Is this change tightly linked to the rest of the patch, or could it be done first with an appropriate separate test case?

llvm/lib/Object/XCOFFObjectFile.cpp

1463–1464

Do we have testing that these two bytes are now skipped (and which would have failed before because they weren't)?

1488

Noting that there needs to be test cases both with and without this flag.

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

109

Nit: double blank line.

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

7

It's not strictly required, but I think the suggested edit would improve readability.

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

3

Sorry, missed this last time.

8

As you've got lots of whitespace formatting as part of your changes, you should use --strict-whitespace and --match-full-lines in your FileCheck command.

llvm/tools/llvm-objdump/XCOFFDump.cpp

124

Prefer static_cast to C-style casts.

126–128

This should be a function, not a macro.

162

Also move it to the line immediately before the for loop a bit below where it is updated.

164–165

168

184

Why 8?

383

"Padding" -> "Padding" or "Padding bytes"

387

I'm not sure you really need this comment, given you've already labelled the previous bit with "Padding".

llvm/tools/llvm-objdump/XCOFFDump.h

12–14

Can you use forward declarations instead of additional includes here?

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

162

Why have you deleted this blank line?

[DiggerLin]

llvm/lib/BinaryFormat/XCOFF.cpp

179–180 ↗

(On Diff #478284)

The function "getExtendedTBTableFlagString" is used the patch, but the Flag == 0x0 we do not deal with before, so add the here to deal with by the way(I think Jason asked to add here if I remember correct). If you strong suggest that I should remove it. If will remove it.

llvm/lib/Object/XCOFFObjectFile.cpp

1463–1464

yes, We tested the modification in "XCOFFTracebackTableAPIHasVectorInfo" of the XCOFFObjectFileTest.cpp, we change

EXPECT_EQ(Size, 45u);
--->
EXPECT_EQ(Size, 47u);

1488

we has in llvm/test/CodeGen/PowerPC/aix-emit-tracebacktable-clobber-register.ll
; OBJ-DIS-NEXT: 13a: 08 # ExtensionTable = TB_EH_INFO
; OBJ-DIS-NEXT: 13b: 00 # Alignment padding for except info displacement
; OBJ-DIS-NEXT: 13c: 00 00 00 08 # Except info displacement

and in disassemble-traceback-table.test

1. CHECK-NEXT: 54: c0 00 00 00 # VectorParmsInfoString = vf
2. CHECK-NEXT: 58: 00 00 # Padding
3. CHECK-NEXT: 5a: 20 # ExtensionTable = TB_SSP_CANARY
4. CHECK-NEXT: 5b: 00 # Paddings

llvm/tools/llvm-objdump/XCOFFDump.cpp

184

if the remainning zero are >=8 , we only print as
errs() << "\t\t...\n";
just keep it same AIX tool objdump.

387

we just want to comment of every line the binary, without "Padding", user maybe not sure the line is not decode this moment or just "padding"

llvm/tools/llvm-objdump/XCOFFDump.h

12–14

using forward delcarations here and include the file in XCOFFDump.cpp ? what is the benefit ?

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

162

thanks

[DiggerLin]

address James' comment. thanks James.

[jhenderson]

llvm/lib/BinaryFormat/XCOFF.cpp

179–180 ↗

(On Diff #478284)

I think it should be its own separate patch. Ideally, if you can craft a test case, it could be a prerequisite patch to this one. If not, would would be the impact of moving this into a patch that lands after this one? Would it mean some strange fields in e.g. llvm-objdump output?

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

38

Delete extra blank line.

llvm/tools/llvm-objdump/XCOFFDump.h

12–14

It means that when other files include this header, they don't need to pull in all its dependencies. See https://llvm.org/docs/CodingStandards.html#include-as-little-as-possible.

[DiggerLin]

llvm/lib/BinaryFormat/XCOFF.cpp

179–180 ↗

(On Diff #478284)

I will put this one in a separate patch.

[jhenderson]

I don't have anything else to add at this point. If I get a chance I'll take one final look through the whole patch before I go on vacation, but if I don't by the end of this week, feel free to land without my approval (you should still get somebody with XCOFF knowledge to approve).

[jhenderson]

llvm/docs/CommandGuide/llvm-objdump.rst

440

llvm/tools/llvm-objdump/ObjdumpOpts.td

71

A thought: should --traceback-table imply --disassemble-all, like e.g. --source implies --disassemble?

(If you decide to do this, please make sure to mention it in the documentation and help text).

llvm/tools/llvm-objdump/XCOFFDump.cpp

143

"begin" is a verb meaning "to start something". "start" is what you are looking for.

179

193

Same below for each of the other comments like this.

283

Should this assertion be an actual error? In other words, if the function name length stored was actually 0 (in a malformed object), would this code get here?

394

Redundant?

[DiggerLin]

added --traceback-table imply --disassemble-all and addressed comments.

llvm/tools/llvm-objdump/ObjdumpOpts.td

71

good idea.

llvm/tools/llvm-objdump/XCOFFDump.cpp

143

I think it maybe better to consistent with https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp#L1991

if you strong suggestion to change to "start" , I can add a NFC for both after the patch commit.

193

thanks.

283

I think I can keep the assert here, If we want to check the "FunctionNameLen==0 and isFuncNamePresent is true" malform. we can implement it in function XCOFFTracebackTable::XCOFFTracebackTable() as a new patch.(Current patch is big patch now).

https://github.com/llvm/llvm-project/blob/main/llvm/lib/Object/XCOFFObjectFile.cpp#L1443

if (Cur && isFuncNamePresent()) {
  uint16_t FunctionNameLen = DE.getU16(Cur);
  if (Cur)
    FunctionName = DE.getBytes(Cur, FunctionNameLen);
}

[stephenpeckham]

llvm/lib/Object/XCOFFObjectFile.cpp

1437

The controlled storage offsets are always 4 bytes, even for 64-bit binaries

llvm/tools/llvm-objdump/XCOFFDump.cpp

271

The controlled storage offsets are always 4 bytes, even for 64-bit binaries.

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

1736

Programs have glink code which also has traceback tables, so you should check for XMC_GL as well as XMC_PR.

1793

Programs are usually linked with a non-zero .text origin, so I think you need SectionAddr + End instead of End.

[jhenderson]

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

6

Is it important that this test uses --disassemble-all? -d is the short form of --disassemble.

8

Probably worth a comment like "## Show that --traceback-table implies --disassemble."

llvm/tools/llvm-objdump/ObjdumpOpts.td

70–73

Couple of minor nits. Firstly, the help text shouldn't end with ".". Secondly, I think the "implies" bit should be in a different sentence to the "for XCOFF" bit, i.e. "Decode traceback table in disassembly. Implies --disassemble. This option is for XCOFF files only".

llvm/tools/llvm-objdump/XCOFFDump.cpp

143

Unfortunately, you didn't use a permalink, so since you included that link, the file has changed and I don't know what you're referring to. It might be you're referring to this line: https://github.com/llvm/llvm-project/blob/48027f03f2d5fef884ebe118c42f800b90fae2f9/llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp#L1987, but if so, that example is of use of it as a verb. Either way, grammatically, I am correct (citation: https://dictionary.cambridge.org/dictionary/english/begin, which lists only "verb" forms of "begin"), and this piece of code should be modified before committing anything (don't land something broken in the first place, even if it's not going to have a functional effect).

283

We shouldn't land known broken code, and an assertion that can be hit by user code is broken. Whether that means adding the check in a prerequisite patch or this one depends on the solution you choose to go with.

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

3231–3232

I'd be okay with moving the "if traceback table, enable --disassemble" logic to here, like the other cases, and dropping the "if XCOFF" check. It will simplify the code slightly, as you can then drop the !TracebackTable check from the below if clause. It also means that disassembly is printed for both XCOFF and non-XCOFF objects in the same invocation of llvm-objdump, which is good for consistency.

(If you do this, it might be worth adding a trivial test case showing that disassembly is enabled for non-XCOFF objects with --traceback-table).