[PATCH] D89049: [AIX][XCOFF] print out the traceback info

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:371
+  // Print all padding.
+  uint64_t NextLineBytesNum = 4 - Index % 4;
+  if (Size - Index <= 4) {
----------------
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;
  }
}
```


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:393
+  while (Index < Size) {
+
+    // Set the 'Index' back to the first byte of the previously printed line.
----------------
Don't start blocks with a blank line.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:354
+    OS << "\n";
+    // The size of displacement(address) is 4 bytes in 32-bit object files, and
+    // 8 bytes in 64-bit object files.
----------------
jhenderson wrote:
> 
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89049/new/

https://reviews.llvm.org/D89049