[PATCH] D89049: [AIX][XCOFF] print out the traceback info
Digger Lin via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Jun 23 11:45:44 PDT 2023
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:
> DiggerLin wrote:
> > 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;
> >
> >
> 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).
thanks, changed as suggestion.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89049/new/
https://reviews.llvm.org/D89049
More information about the llvm-commits
mailing list