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

[Digger Lin via Phabricator via llvm-commits]

DiggerLin 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) {
----------------
jhenderson wrote:
> 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;
>   }
> }
> ```
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; 




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89049