This is an archive of the discontinued LLVM Phabricator instance.

[AIX][XCOFF] print out the traceback info
ClosedPublic

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

Details

Summary

Adding a new option -traceback-table to print out the traceback info of xcoff ojbect file.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
jhenderson added inline comments.Feb 23 2023, 1:19 AM
llvm/tools/llvm-objdump/llvm-objdump.cpp
3008–3009

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

DiggerLin updated this revision to Diff 513737.Apr 14 2023, 1:53 PM
DiggerLin marked 12 inline comments as done.

address comment

DiggerLin added inline comments.Apr 14 2023, 1:55 PM
llvm/lib/Object/XCOFFObjectFile.cpp
955

thanks ,fixed it.

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

thanks, fixed it.

259

thanks

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

since the patch is a big patch and last for about 3 years, if I added the "XMC_GL" now, I also need to add a test case for "XMC_GL", it will let the patch bigger. Can I have a separate patch to deal with "XMC_GL"?

2002

in the line 1629 ,End already includes the SectionAddr

uint64_t End = std::min<uint64_t>(SectionAddr + SectSize, StopAddress);

3008–3009

Sorry that I can not understand the comment, I do not change code above your comment.

jhenderson added inline comments.Apr 17 2023, 1:51 AM
llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table-warning.test
1 ↗(On Diff #513737)
4 ↗(On Diff #513737)

Nit

14 ↗(On Diff #513737)

Nit: normally, there isn't blank lines between parts of YAML.

35 ↗(On Diff #513737)

Please a) review the error message content and fix it (I'm not going to point it out - it should be obvious to you with a quick look), b) review the LLVM coding standards for formatting of error and warning messages and fix accordingly. If you're still not sure what needs to be changed, please ask, but I don't want to need to explain everything in detail when things should be obvious.

There's no need for the extra space indentation at the start of the CHECK line, i.e. it should be # WARN: {{.*}} etc. Also, there's no real need for the first {{.*}}: before warning:. Finally, it would be good to check the actual file name after warning:. You can add -D FILE=%t.o to the FileCheck invocation and then use [[FILE]] in place of the {{.*}} for the file path.

llvm/tools/llvm-objdump/llvm-objdump.cpp
3008–3009

In this version of the patch, at line 2834, you add a check for (O->isXCOFF() && TracebackTable) to cause disassembly to be printed. This is different to how other command-line options, such as -S (PrintSource) cause printDisassembly to be called, if --disassemble is specified. I think you should change how --traceback-table causes disassembly to be printed, to be the same as these other options.

By changing this, there are a few effects:

  1. You can remove the change you made at line 2834.
  2. You can remove the !TracebackTable check at line 3239, since it will be redundant.
  3. If you use --traceback-table when you provide a non-XCOFF file, disassembly will be printed, even if the user hasn't specified --disassemble. There should be a new test case to show this behaviour.

Does that make things clearer?

DiggerLin marked 4 inline comments as done.
DiggerLin added inline comments.Apr 17 2023, 12:55 PM
llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table-warning.test
1 ↗(On Diff #513737)

thanks.

llvm/tools/llvm-objdump/llvm-objdump.cpp
3008–3009

thanks a lot for explain.

DiggerLin marked an inline comment as done.Apr 17 2023, 12:55 PM
DiggerLin added inline comments.Apr 17 2023, 1:54 PM
llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table-warning.test
35 ↗(On Diff #513737)

sorry for above mistake.

As a gentle reminder, please could you make sure you do a personal review of your own code before uploading a patch for review (including updates to an existing patch). If you are working in a company, it is generally a good idea to get a second person to take a look at your code within your company before pushing to the open source community for review. This will help reduce the number of silly mistakes, improving the time it takes to get a patch through review, due to fewer iterations of the patch being needed, and increasing the desire from LLVM community reviewers , such as myself, to review your code (which again will improve the time it takes to get a patch reviewed).

llvm/test/CodeGen/PowerPC/aix-emit-tracebacktable-clobber-register.ll
49 ↗(On Diff #514371)

Please make a separate small commit to change the comment here that is emitted.

llvm/test/tools/llvm-objdump/X86/elf-disassemble.test
4 ↗(On Diff #514371)

I think this case deserves a comment. To avoid confusing the issue, I'd move the RUN line to the end of the test (after the checks), and add a comment saying that this shows that disassembly is enabled by default for --traceback-table, or something to that effect.

llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table-warning.test
34 ↗(On Diff #514371)

I think this line has trailing whitespace you can remove?

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

You don't need this anymore, because the Disassemble value is checked. See point 2 in my comment above.

DiggerLin updated this revision to Diff 515324.Apr 20 2023, 7:30 AM
DiggerLin marked 4 inline comments as done.
  1. rebased to latest master , it including change Optional to std::optional. and changing test case based on patch https://reviews.llvm.org/D146071
  2. address comment.

As a gentle reminder, please could you make sure you do a personal review of your own code before uploading a patch for review (including updates to an existing patch). If you are working in a company, it is generally a good idea to get a second person to take a look at your code within your company before pushing to the open source community for review. This will help reduce the number of silly mistakes, improving the time it takes to get a patch through review, due to fewer iterations of the patch being needed, and increasing the desire from LLVM community reviewers , such as myself, to review your code (which again will improve the time it takes to get a patch reviewed).

I understand your frustration. In fact, every time when I update, I conduct a self-review, but I still have blind spots. I will ask my internal team member to do some review and reduce your work. Sorry for the inconvenient.

llvm/test/CodeGen/PowerPC/aix-emit-tracebacktable-clobber-register.ll
49 ↗(On Diff #514371)

yes, I will do it in a separate patch.

llvm/test/tools/llvm-objdump/X86/elf-disassemble.test
4 ↗(On Diff #514371)

I put it after the "# RUN: llvm-objdump %t.o -d | FileCheck %s --implicit-check-not=Disassembly" because option "--traceback-table" implies enable -d or "--disassemble" not implies enable "--disassemble-all".

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

thanks. actually, I take a reivew myself when I tried to update the patch, sorry for I do not find the error. thanks for your time.

DiggerLin marked an inline comment as done.Apr 20 2023, 7:41 AM

Mostly looks fine now, apart from a few small things, but someone familiar with XCOFF needs to review this too.

llvm/test/tools/llvm-objdump/X86/elf-disassemble.test
6 ↗(On Diff #515324)

Sorry, I should have mentioned this before: I reckon having a bit of extra info in the comment would be useful - see the inline edit.

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

I'm going to assume that XCOFFTracebackTable::create actually needs to modify Size for some other use-case, rather than just taking it by value.

Rather than resetting the Size value a few lines down, I think it might be better to create a copy of the Size variable and pass that in, e.g:

// XCOFFTracebackTable::create modifies the size parameter, so ensure Size isn't changed.
uint64_t SizeCopy = End - Address;
Expected<XCOFFTracebackTable> TTOrErr =
    XCOFFTracebackTable::create(Bytes.data() + Index, SizeCopy, Is64Bit);
130

Don't print trailing whitespace.

148

Sorry, I missed this earlier: I don't like how you print the warning using reportWarning, and then go on to add more content to stderr outside the reportWarning function. This bakes in an assumption that reportWarning actually prints the warning to stderr. This is fine for now, but imagine if somebody wanted to add a --suppress-warnings option to llvm-objdump, or another option that changed where warnings are printed to. In those cases, the main warning would be hidden, but not the body of the message.

Better would be to use a single local stream that includes the initial part of the message and to which the rest of the warning content is printed. You then finish off this block with a call to reportWarning, passing in the whole string you built up, with the complete message.

The changes to the yaml2obj files allow valid XCOFF files to be generated. With the suggested changes, you can run "ld -r" on the generate object files.

Testing for this patch is difficult because the compiler doesn't generate all combinations of optional traceback-table fields.

llvm/lib/Object/XCOFFObjectFile.cpp
973

This is not right. The only potential padding in a traceback table is before the EH value (if TB_EH_INFO is set), because the eh value must be aligned on a word boundary.

Removing DH.skip(2) will affect the tests.

An object with vector information and EH information will be displayed incorrectly.

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

SectionOrLength: 40
SymbolAlignmentAndType: 0x21

will generate a valid XCOFF file

llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table-warning.test
22 ↗(On Diff #515324)

SymbolAlignmentAndType: 0x21
SetionOrLength: 0x24

31 ↗(On Diff #515324)

SymbolAlignmentAndType: 0x21

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

SymbolAlignmentAndType: 0x21
SectionOrLength:0x24

38

SymbolAlignmentAndType: 0x21
SectionOrLength:0x38

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

It's not critical, but I would use "# " in the output for the array of displacements.

I['m satisfied with the current state of the code. The updates I suggested to the test cases could be handled in another patch.

llvm/lib/Object/XCOFFObjectFile.cpp
973

After additional investigation, I retract my previous comment. The LLVM compiler emits padding for the vector extension of the traceback table while the legacy compilers do not.

DiggerLin marked 12 inline comments as done.
DiggerLin added inline comments.Apr 28 2023, 10:37 AM
llvm/tools/llvm-objdump/XCOFFDump.cpp
246

I will use '#' for the first one. ControlledStorageInfoDisp[0]

DiggerLin added inline comments.Apr 28 2023, 12:06 PM
llvm/test/tools/llvm-objdump/XCOFF/disassemble-invalid-traceback-table.test
7

I will delete the trail white space in next update.

28

I will align the warning(back a space) in next update.

xingxue added inline comments.May 1 2023, 12:36 PM
llvm/lib/Object/XCOFFObjectFile.cpp
971–972

Wouldn't this dereference 0 if !ParmsTypeOrError is true?

llvm/tools/llvm-objdump/XCOFFDump.cpp
328
332
354

If the remaining bytes are paddings, why do we differentiate if a padding is zero or not? Can we just print ... or ... # Paddings for paddings?

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

in the line 1629 ,End already includes the SectionAddr

uint64_t End = std::min<uint64_t>(SectionAddr + SectSize, StopAddress);

@stephenpeckham's comment is still valid. Although End was initialized with SectionAddr added as you pointed out, SectionAddr was later subtracted from it in line 1649.

End -= SectionAddr;
DiggerLin updated this revision to Diff 519058.May 3 2023, 6:55 AM
DiggerLin marked 6 inline comments as done.

address xing's comment

DiggerLin added inline comments.May 3 2023, 6:57 AM
llvm/lib/Object/XCOFFObjectFile.cpp
971–972

ParmsTypeOrError is an object of class Expected.

In https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/Support/Error.h#L559

there is definition of

  /// Return false if there is an error.
  explicit operator bool() {
#if LLVM_ENABLE_ABI_BREAKING_CHECKS
    Unchecked = HasError;
#endif
    return !HasError;
  }

!ParmsTypeOrError) means there is an error.

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

it just to print out the "... # Paddings" if all the remaining is zero and there are 8 or more bytes remaining in the end.

otherwise, it will print all the bytes out with following code.

uint16_t AlignmentLen = 4 - Index % 4;
 printRawData(Bytes.slice(Index, AlignmentLen), Address + Index, OS, STI);
 OS << "\t# Padding\n";
 Index += AlignmentLen;
 while (Index < End - Address) {
   printRawData(Bytes.slice(Index, 4), Address + Index, OS, STI);
   OS << "\t# Padding\n";
   Index += 4;
 }
llvm/tools/llvm-objdump/llvm-objdump.cpp
2002

good catch , thanks. I will fix it.

xingxue added inline comments.May 3 2023, 8:22 AM
llvm/tools/llvm-objdump/XCOFFDump.cpp
354

I meant to print ... or ...\t# Paddings to indicate the rest are paddings regardless of whether a padding byte is zero or not. @stephenpeckham @jhenderson, what do you think?

DiggerLin marked an inline comment as done.May 8 2023, 6:21 AM
DiggerLin added inline comments.
llvm/tools/llvm-objdump/XCOFFDump.cpp
354

Print out no-zero value will letting user know what is non-zero value of the padding(in our llvm xcoff ojbect writer, the padding will always zero for sections, if not zero means ,something wrong, and printing out non-zero will let user get more info)
@hubert.reinterpretcast , do you have any option on above xing's comment ?

DiggerLin marked an inline comment as done.May 11 2023, 6:47 AM

reformat padding output of traceback table based on the discussion with Xingxue and stephen packham offline.

xingxue accepted this revision.May 16 2023, 1:53 PM

Thanks for working on this feature! LGTM once CI is fixed, which is a clang-format issue.

This revision is now accepted and ready to land.May 16 2023, 1:53 PM

I plan on giving this another look through next week, as by then, I should have got past my high priority stuff at work. If you don't mind, please hold off submitting until then.

I plan on giving this another look through next week, as by then, I should have got past my high priority stuff at work. If you don't mind, please hold off submitting until then.

OK, I will hold off submitting, thanks for your time.

Apologies @DiggerLin, I didn't have time to look last week as the critical work I was expecting to have finished by early last week has thrown up several issues that I need to resolve urgently. Due to UK bank holidays and personal time off` this week, it won't be until next week before I have time to look at this (at the earliest). I will try to prioritise this in the upstream reviews that I look at first though.

Apologies @DiggerLin, I didn't have time to look last week as the critical work I was expecting to have finished by early last week has thrown up several issues that I need to resolve urgently. Due to UK bank holidays and personal time off` this week, it won't be until next week before I have time to look at this (at the earliest). I will try to prioritise this in the upstream reviews that I look at first though.

Thanks a lot , @jhenderson

Pre-merge check is complaining about clang-format. Please make sure to run clang-format on all your changes.

There are a lot of cases where you've used the .value() method of std::optional. I believe operator*()/operator->() are more typical ways of achieving the same thing (and according to cppreference.com, may even be more efficient).

llvm/lib/Object/XCOFFObjectFile.cpp
953

Is this change needed?

979

Is this an option?

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

Sorry, didn't spot this before.

llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table-warning.test
22 ↗(On Diff #521328)
llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table.test
3–4

I don't think the part about llc is relevant. It will also go out-of-date if llc should ever be able to generate it in the future.

20

How much of this text data is actually important to the test? If a significant proportion of it is, it would be worth giving instructions on how to generate the machine code that this represents.

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

As this is specific to XCOFFObjectFile, I think it would make sense to pass in XCOFFObjectFile, rather than ObjectFile, and do the cast outside this function.

280

?

290

?

295

?

jhenderson added inline comments.Jun 12 2023, 1:47 AM
llvm/tools/llvm-objdump/XCOFFDump.cpp
108

?

244
255

?

322

?

336–337

I think the comment can be simplified, as suggested. Please reflow appropriately.

354

I don't have a strong opinion on whether non-zero padding should be printed or not. I'm happy to defer to others on this.

360

Don't use else after a return.

367

This is more grammatically correct.

392

I'm not sure this comment makes sense (some of the bytes can be zero, just not the first one), but regardless, I don't think it's really needed.

395

Is this supposed to be duplicated?

397

Don't end a block with a blank line.

llvm/tools/llvm-objdump/llvm-objdump.cpp
1949
DiggerLin marked 20 inline comments as done.Jun 12 2023, 11:01 AM
DiggerLin added inline comments.
llvm/lib/Object/XCOFFObjectFile.cpp
953

yes , it is not necessary .

979

yes. thanks

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

agree.

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

thanks

395

yes, the code will make the output as

# CHECK-NEXT:      5b: 00               # Padding
# CHECK-NEXT:        ...
# CHECK-NEXT:      68: 01 00 00 00
# CHECK-NEXT:      72: 00 00 00 00
# CHECK-NEXT:        ...
# CHECK-NEXT:      90: 01 00 00 00

without the duplication code, above will be changed to

# CHECK-NEXT:      5b: 00               # Padding
# CHECK-NEXT:        ...
# CHECK-NEXT:      68: 01 00 00 00
# CHECK-NEXT:        ...
# CHECK-NEXT:      90: 01 00 00 00
DiggerLin marked 5 inline comments as done.Jun 12 2023, 11:05 AM
DiggerLin added inline comments.
llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table.test
20

there are two functions in the test case. it test different traceback table fields. and it also test different padding format.

address James' comment

DiggerLin marked 3 inline comments as done.Jun 12 2023, 12:30 PM
MaskRay added inline comments.Jun 12 2023, 7:06 PM
llvm/tools/llvm-objdump/XCOFFDump.cpp
143

We don't usually add parens for (A + B) > C and ... ? : A + B.

153

Nit: here and throughout, you can use '\n' as it compiles to less machine code and is slightly faster.

jhenderson added inline comments.Jun 13 2023, 2:14 AM
llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table.test
42–45

(Strictly, you could use "data are" and be grammatically correct, but it sounds weird to a native speaker)

llvm/tools/llvm-objdump/XCOFFDump.cpp
332
392

Ping this comment?

395

The logic here is fairly intertwined, so I might be misunderstanding something, but if the aim is to ensure that there is always a set of zeros before the "...", I'm not convinced that this code achieves that. As far as I can tell, the first four bytes are printed and definitely have at least one non-zero byte involved. The second call though could print non-zero bytes too, so you'd end up with, e.g.:

5b: 00               # Padding
  ...
68: 01 00 00 00
72: 00 00 00 01
  ...
90: 01 00 00 00

If the next 12 bytes are then all 0, the ... will still be printed. I think?

395

I've gone back over the logic again and I'm finding it really hard to figure out. I'd like to summarise what my understanding of the goals of this code actually are, to make sure I understand things correctly:

  1. Print a representation of the padding bytes from the end of the parsed data to the end of the full data.
  2. Data is printed out in blocks of 4 bytes, aligned to the word boundary.
  3. The first line may have fewer than 4 bytes, if the parsed data didn't end on a word boundary.
  4. Consecutive blocks of 12 zeros or more are skipped and replaced with "...".
  5. Any such skipped block must be preceded by a block of 4 bytes that are all zero.

Is this correct?

DiggerLin marked 5 inline comments as done.Jun 13 2023, 1:01 PM
DiggerLin added inline comments.
llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table.test
42–45

thanks

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

thanks

392

yes, some bytes can be zero in the code

// Print four bytes which has non-zero value.
 if (Print4BytesOrFewer())
   return;

I delete the comment. thanks

395

it will not print as above.

after print
72: 00 00 00 01

there is code " Index -= NextLineBytesNum;"

while (Index < Size) {
   Index -= NextLineBytesNum;

to reset the index to 0x72, so there is not more than 12 consecutive bytes of zeros.
it will print next line , like

5b: 00               # Padding
  ...
0x68: 01 00 00 00
0x72: 00 00 00 01 
0x76: 00 00 00 00  
   ...
0x90: 01 00 00 00
395

yes. you are correct!

Any such skipped block must be preceded by a block of 4 bytes that are all zero.

except for the first align padding like (which do not need a block of 4 bytes at first align padding)

5b: 00               # Padding
  ...
DiggerLin updated this revision to Diff 531038.Jun 13 2023, 1:05 PM
DiggerLin marked 4 inline comments as done.
jhenderson added inline comments.Jun 14 2023, 1:27 AM
llvm/tools/llvm-objdump/XCOFFDump.cpp
332

You missed the space between "displacement" and "(address)". Unless that's a function call (i.e. a "function" called "displacement" with a "parameter" called "address"), there should always be a space before the opening parenthesis in English prose.

349

Having read through this code a few more times, I think we can simplify it in a few ways. Here's my attempt. There might be bugs/typos in it, but hopefully it gets the gist of what I'm thinking, even if it's not 100%.

Some things that motivated this:

  • The initial if (Size - Index <= 4) line looks unnecessary to me. Everything else will just work wtihout it, I believe.
  • Without too much difficulty, we can get rid of the special alignment printing.
  • Print4BytesOrFewer is just PrintBytes with some extra stuff, so it should probably call PrintBytes.
  • I also am not convinced that the Size == Index line belongs in Print4BytesOrFewer, although I see why it's being done. I think it would be more readable in the main code logic to pull it out.
  • As Print4BytesOrFewer is specific to the Padding, it might be worth renaming it for clarity, e.g. PrintPaddingLine. However, after rewriting the code to all be in a single loop, I realised that we only needed one call to PrintBytes, so the lambda can disappear completely.
// Print all padding.
const char *LineSuffix = "\t# Padding\n";
while(Index < Size) {
  // Determine how many bytes to print (4, except for the first line, which will be just enough to align to the word boundary).
  uint64_t PrintSize = 4 - Index % 4;
  PrintSize = std::min(PrintSize, Size - Index);

  // Identify if the to-be printed line contains only zero-valued bytes.
  bool LastLinePrintedWasAllZero = true;
  for (size_t I = Index; I < Index + PrintSize; ++Index)
    LastLinePrintedWasAllZero &= Bytes[I] == 0;

  // Print the line.
  PrintBytes(PrintSize);
  OS << LineSuffix;
  LineSuffix = "\n";

  // If three or more consecutive lines would only contain 0-valued bytes, replace all after the first with "...".
  if (LastLinePrintedWasAllZero) {
    // Look to find the next non-zero byte. 
    auto NextNonZero = std::find_if_not(Bytes.begin() + Index, Bytes.begin() + Size, [](uint8_t Byte){ return Byte == 0; });
    // The next two lines won't both be all zero, so proceed as normal.
    if (NextNonZero < Bytes.begin() + Index + 8)
      continue;

    // The next two lines would both be all zero. Skip them and all subsequent zero-valued lines. Make sure we only skip full lines by rounding down to the word boundary (the last line is always considered a full line).
    OS << std::string(8, ' ') << "...\n";
    Index += std::distance(Bytes.begin() + Index, NextNonZero);
    if (Index == Size)
      return;
    Index = Index - Index % 4;
  }
}
371

Don't start blocks with a blank line.

DiggerLin marked 2 inline comments as done.
DiggerLin added inline comments.
llvm/tools/llvm-objdump/XCOFFDump.cpp
349

thanks, I got some ideal from your code to simplify my code. But I kept most of my code.

for I think some part of my code is more efficient.

for example:

// Identify if the to-be printed line contains only zero-valued bytes.
  bool LastLinePrintedWasAllZero = true;
  for (size_t I = Index; I < Index + PrintSize; ++Index)
    LastLinePrintedWasAllZero &= Bytes[I] == 0;

  // If three or more consecutive lines would only contain 0-valued bytes, replace all after the first with "...".
  if (LastLinePrintedWasAllZero) {
    // Look to find the next non-zero byte. 
    auto NextNonZero = std::find_if_not(Bytes.begin() + Index, Bytes.begin() + Size, [](uint8_t Byte){ return Byte == 0; });

    // The next two lines won't both be all zero, so proceed as normal.
if (NextNonZero < Bytes.begin() + Index + 8)
      continue;

    // The next two lines would both be all zero. Skip them and all subsequent zero-valued lines. Make sure we only skip full lines by rounding down to the word boundary (the last line is always considered a full line).
    OS << std::string(8, ' ') << "...\n";

I use

// Set the 'CurIndex' back to the first byte of the previously printed line.

uint64_t CurIndex = Index - PrintSize;

// Count the number of consecutive bytes of zeros starting from the first
// byte of the previously printed line.
while (Bytes[CurIndex] == 0 && CurIndex < Size)
  ++CurIndex;

// If three or more consecutive lines would only contain 0-valued bytes,
// replace all after the first with "...".
if (CurIndex >= Index + 8) {
   OS << std::string(8, ' ') << "...\n";

and in the code

   // The next two lines won't both be all zero, so proceed as normal.
if (NextNonZero < Bytes.begin() + Index + 8)
      continue;

we already know that "the two line won't both be all zero" , we just need to print out immediately without to check all zero again.

I use code to print out the two lines immediately

while (Index < CurIndex )
   if (Print4BytesOrFewer())
     return;
DiggerLin updated this revision to Diff 531710.Jun 15 2023, 5:43 AM

Ah, sorry. I wrote a lengthy comment last week with another suggested approach, but somehow failed to submit it.

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

I see your point, but I'm still not a fan of how the code you've written is structured, as it makes it difficult to understand and see what is going on. How does the following attempt look? This ensures we inspect each line to see if it is all 0 or not once only, but also ensures we only have one location where we call PrintBytes, and only one Index tracker.

// Print all padding.
const char *LineSuffix = "\t# Padding\n";
auto IsWordZero = [&](uint64_t WordPos) {
  if (WordPos >= Size)
    return true;
  uint64_t LineLength = 4 - Index % 4;
  LineLength = std::min(4, Size - WordPos);
  return std::all_of(Bytes + WordPos, Bytes + WordPos + LineLength, [](uint8_t Byte){ return Byte == 0; });
};
bool AreWordsZero = { IsWordZero(Index), IsWordZero(Index + 4), IsWordZero(Index + 8) };
bool ShouldPrintCurrentWord = true;
while(true) {
  // Determine the length of the line (4, except for the first line, which will be just enough to align to the word boundary, and the last line, which will be the remainder of the data).
  uint64_t LineLength = 4 - Index % 4;
  LineLength = std::min(LineLength , Size - Index);
  if (ShouldPrintLine) {
    // Print the line.
    printRawData(Bytes.slice(Index, LineLength), Address + Index, OS, STI);
    OS << LineSuffix;
    LineSuffix = "\n";
  }

  if (Index + LineLength == Size)
    return;

  // For 3 or more consecutive lines of zeros, skip all but the first one, and replace them with "...".
  if (AreWordsZero[0] && AreWordsZero[1] && AreWordsZero[2]) {
    if (ShouldPrintLine)
      OS << std::string(8, ' ') << "...\n";
    ShouldPrintLine = false;
  } else if (!ShouldPrintLine && !AreWordsZero[1]) {
    // We have reached the end of a skipped block of zeros.
    ShouldPrintLine = true;
  }
  AreWordsZero[0] = AreWordsZero[1];
  AreWordsZero[1] = AreWordsZero[2];
  AreWordsZero[2] = IsWordZero(Index + 8);
  Index += LineLength;
}

This essentially uses a sliding window approach to keep track of the upcoming words and whether they are all zero or not, whilst also tracking whether a line should be printed or not. In addition to checking each byte only once to see if it is all zero, this code also only has only one place where Index is updated (hence the use of printRawBytes rather than PrintBytes), and one place where the data is printed (those two points are the main issues I have with your current code).

DiggerLin updated this revision to Diff 533994.Jun 23 2023, 9:51 AM
DiggerLin marked an inline comment as done.
DiggerLin added inline comments.Jun 23 2023, 11:45 AM
llvm/tools/llvm-objdump/XCOFFDump.cpp
349

thanks, changed as suggestion.

MaskRay added inline comments.Jun 23 2023, 1:02 PM
llvm/test/CodeGen/PowerPC/aix-emit-tracebacktable-clobber-register.ll
12 ↗(On Diff #533994)

You may want --strict-whitespace as well, otherwise the space formatting change in the code cannot be tested.

I just took a rough look. The new code and test look good to me.

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

delete blank line

141

omit braces

164

move this to the first use site and delete the blank line after the declaration.

actually, it may be better to avoid this used-only-once variable.

255

delete blank line

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

Is there a risk that SI-1 == size_t(-1) ?

1 .print out the traceback info

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.

This patch adds a major feature but the description seems a bit too concise.

DiggerLin updated this revision to Diff 534548.Jun 26 2023, 7:18 AM
DiggerLin marked 8 inline comments as done.
DiggerLin edited the summary of this revision. (Show Details)

address MaskRay's comment

DiggerLin added inline comments.Jun 26 2023, 7:20 AM
llvm/tools/llvm-objdump/llvm-objdump.cpp
1949

I do not think there is a risk

for (size_t SI = 0, SE = Symbols.size(); SI != SE;) {
    // Advance SI past all the symbols starting at the same address,
    // and make an ArrayRef of them.
    unsigned FirstSI = SI;
    uint64_t Start = Symbols[SI].Addr;
    ArrayRef<SymbolInfoTy> SymbolsHere;
    while (SI != SE && Symbols[SI].Addr == Start)
      ++SI;
while (SI != SE && Symbols[SI].Addr == Start)
  ++SI;

must be run when the code first time enter the loop. so after the ++SI, the SI must be large than 0.

DiggerLin updated this revision to Diff 535205.Jun 27 2023, 5:34 PM

rebase the patch

jhenderson accepted this revision.Jun 28 2023, 12:28 AM

Apart from a couple of nits, LGTM!! Thanks for the patience, and sorry I wasn't able to review particularly efficiently.

Please don't land this without further sign off from @MaskRay and possibly @stephenpeckham.

llvm/test/CodeGen/PowerPC/aix-emit-tracebacktable-clobber-register.ll
12 ↗(On Diff #533994)

Did you miss this comment?

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

FWIW, I think the blank line that was removed here is okay to be present. I think @MaskRay was suggesting that a variable that is declared and then used in an if statement immediately afterwards should not have a blank line between it and the if statement.

A blank line after a non-trivial if block is usually a good thing, if there is code after the if statement.

DiggerLin updated this revision to Diff 535398.EditedJun 28 2023, 7:25 AM

add --strict-whitespace for test case aix-emit-tracebacktable-clobber-register.ll @MaskRay

DiggerLin marked 2 inline comments as done.Jun 28 2023, 7:26 AM

Apart from a couple of nits, LGTM!! Thanks for the patience, and sorry I wasn't able to review particularly efficiently.

I appreciate your thorough review and taking the time to provide your inputs. I have addressed the nits you mentioned, and I'm glad to hear that you found the overall work to be acceptable. I completely understand that reviewing is time-consuming, so there's no need to apologize. Your comments and patience are greatly appreciated! , Thanks for your times. @jhenderson
MaskRay accepted this revision.Jun 28 2023, 2:41 PM

I only spot checked this and this looks good.

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

ditto. align

llvm/tools/llvm-objdump/ObjdumpOpts.td
66 ↗(On Diff #535398)

I think it's time to use Group<grp_xcoff>;, then you can omit This option is for XCOFF files only since the options will be listed with a --help section named XCOFF. See grp_mach_o for an example.

jhenderson added inline comments.Jun 29 2023, 12:23 AM
llvm/tools/llvm-objdump/ObjdumpOpts.td
66 ↗(On Diff #535398)

Makes sense to me (probably it should be a subsequent patch though).

DiggerLin marked 4 inline comments as done.Jun 29 2023, 7:33 AM
DiggerLin added inline comments.
llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table.test
56

the original output as

00000000 (idx: 0) .AddNum[PR]:
       0: 94 21 ff c0   stwu 1, -64(1)
       4: 00 00 00 00   # Traceback table start


00000024 (idx: 2) .foo[PR]:
      24: 93 e1 ff fc   stw 31, -4(1)
      28: 00 00 00 00   # Traceback table start
      2c: 00            # Version = 0

00000000 (idx: 0) .AddNum[PR]:
00000024 (idx: 2) .foo[PR]:

are label, there start from column zero.

and the test case use --strict-whitespace
so I have to keep as

CHECK:00000000 (idx: 0) .AddNum[PR]:
llvm/tools/llvm-objdump/ObjdumpOpts.td
66 ↗(On Diff #535398)

thanks. I will do it in subsequent patch for this comment.

DiggerLin marked 2 inline comments as done.Jun 29 2023, 8:31 AM

I've skimmed through the latest changes and I didn't see any additional problems.

This revision was landed with ongoing or failed builds.Jul 6 2023, 8:49 AM
This revision was automatically updated to reflect the committed changes.