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

  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

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
jhenderson added inline comments.Sep 10 2021, 1:04 AM
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 added inline comments.Sep 10 2021, 6:32 AM
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 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.

DiggerLin edited the summary of this revision. (Show Details)Nov 17 2022, 12:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 17 2022, 12:28 PM
Herald added a subscriber: StephenFan. · View Herald Transcript
DiggerLin marked 6 inline comments as done.Nov 25 2022, 10:34 AM
DiggerLin added inline comments.
llvm/tools/llvm-objdump/XCOFFDump.cpp
144

yes, thanks

DiggerLin marked an inline comment as done.

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

jhenderson added inline comments.
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 marked 3 inline comments as done.Nov 28 2022, 10:56 AM
DiggerLin added inline comments.
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 marked an inline comment as done.

address James' comment

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 marked 15 inline comments as done.Dec 2 2022, 7:46 AM
DiggerLin added inline comments.
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 updated this revision to Diff 479631.EditedDec 2 2022, 7:47 AM
DiggerLin marked 7 inline comments as done.

address James' comment. thanks James.

jhenderson added inline comments.Dec 6 2022, 12:14 AM
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 edited reviewers, added: stephenpeckham; removed: jasonliu.Dec 6 2022, 10:43 AM
DiggerLin updated this revision to Diff 480549.Dec 6 2022, 10:53 AM
DiggerLin marked an inline comment as done.
DiggerLin added inline comments.Dec 6 2022, 10:54 AM
llvm/lib/BinaryFormat/XCOFF.cpp
179–180 ↗(On Diff #478284)

I will put this one in a separate patch.

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 added inline comments.Dec 9 2022, 4:18 AM
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 marked 5 inline comments as done.

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 added inline comments.Jan 5 2023, 12:25 PM
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 added inline comments.Thu, Feb 23, 1:19 AM
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).