Adding a new option -traceback-table to print out the traceback info of xcoff ojbect file.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
llvm/tools/llvm-objdump/llvm-objdump.cpp | ||
---|---|---|
2709–2710 | 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). |
llvm/lib/Object/XCOFFObjectFile.cpp | ||
---|---|---|
1151 | thanks ,fixed it. | |
llvm/tools/llvm-objdump/XCOFFDump.cpp | ||
254 | thanks, fixed it. | |
266 | thanks | |
llvm/tools/llvm-objdump/llvm-objdump.cpp | ||
1407 | 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"? | |
1459 | in the line 1629 ,End already includes the SectionAddr uint64_t End = std::min<uint64_t>(SectionAddr + SectSize, StopAddress); | |
2709–2710 | Sorry that I can not understand the comment, I do not change code above your comment. |
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 | ||
2709–2710 | 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:
Does that make things clearer? |
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 | ||
---|---|---|
44 | 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 | ||
2717 | You don't need this anymore, because the Disassemble value is checked. See point 2 in my comment above. |
- rebased to latest master , it including change Optional to std::optional. and changing test case based on patch https://reviews.llvm.org/D146071
- address comment.
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 | ||
---|---|---|
44 | 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 | ||
2717 | 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. |
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 | ||
133 | 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); | |
137 | Don't print trailing whitespace. | |
155 | 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 | ||
---|---|---|
1178 | 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 will generate a valid XCOFF file | |
llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table-warning.test | ||
22 ↗ | (On Diff #515324) | SymbolAlignmentAndType: 0x21 |
31 ↗ | (On Diff #515324) | SymbolAlignmentAndType: 0x21 |
llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table.test | ||
29 | SymbolAlignmentAndType: 0x21 | |
38 | SymbolAlignmentAndType: 0x21 | |
llvm/tools/llvm-objdump/XCOFFDump.cpp | ||
253 | 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 | ||
---|---|---|
1178 | 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. |
llvm/tools/llvm-objdump/XCOFFDump.cpp | ||
---|---|---|
253 | I will use '#' for the first one. ControlledStorageInfoDisp[0] |
llvm/lib/Object/XCOFFObjectFile.cpp | ||
---|---|---|
1193 | Wouldn't this dereference 0 if !ParmsTypeOrError is true? | |
llvm/tools/llvm-objdump/XCOFFDump.cpp | ||
335 | ||
339 | ||
361 | 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 | ||
1459 |
@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; |
llvm/lib/Object/XCOFFObjectFile.cpp | ||
---|---|---|
1193 | 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 | ||
361 | 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 | ||
1459 | good catch , thanks. I will fix it. |
llvm/tools/llvm-objdump/XCOFFDump.cpp | ||
---|---|---|
361 | 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? |
llvm/tools/llvm-objdump/XCOFFDump.cpp | ||
---|---|---|
361 | 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) |
reformat padding output of traceback table based on the discussion with Xingxue and stephen packham offline.
Thanks for working on this feature! LGTM once CI is fixed, which is a clang-format issue.
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.
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.
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 | ||
---|---|---|
1149 | Is this change needed? | |
1202 | 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 | ||
121 | 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. | |
287 | ? | |
297 | ? | |
302 | ? |
llvm/tools/llvm-objdump/XCOFFDump.cpp | ||
---|---|---|
115 | ? | |
251 | ||
262 | ? | |
329 | ? | |
343–344 | I think the comment can be simplified, as suggested. Please reflow appropriately. | |
361 | 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. | |
367 | Don't use else after a return. | |
374 | This is more grammatically correct. | |
399 | 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. | |
402 | Is this supposed to be duplicated? | |
404 | Don't end a block with a blank line. | |
llvm/tools/llvm-objdump/llvm-objdump.cpp | ||
1406 |
llvm/lib/Object/XCOFFObjectFile.cpp | ||
---|---|---|
1149 | yes , it is not necessary . | |
1202 | yes. thanks | |
llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table.test | ||
3–4 | agree. | |
llvm/tools/llvm-objdump/XCOFFDump.cpp | ||
262 | thanks | |
402 | 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 |
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. |
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 | ||
339 | ||
399 | Ping this comment? | |
402 | 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? | |
402 | 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:
Is this correct? |
llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table.test | ||
---|---|---|
42–45 | thanks | |
llvm/tools/llvm-objdump/XCOFFDump.cpp | ||
160 | thanks | |
399 | yes, some bytes can be zero in the code
I delete the comment. thanks | |
402 | it will not print as above. after print 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. 5b: 00 # Padding ... 0x68: 01 00 00 00 0x72: 00 00 00 01 0x76: 00 00 00 00 ... 0x90: 01 00 00 00 | |
402 | yes. you are correct!
except for the first align padding like (which do not need a block of 4 bytes at first align padding) 5b: 00 # Padding ... |
llvm/tools/llvm-objdump/XCOFFDump.cpp | ||
---|---|---|
339 | 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. | |
356 | 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:
// 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; } } | |
378 | Don't start blocks with a blank line. |
llvm/tools/llvm-objdump/XCOFFDump.cpp | ||
---|---|---|
356 | 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
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
|
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 | ||
---|---|---|
356 | 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). |
llvm/tools/llvm-objdump/XCOFFDump.cpp | ||
---|---|---|
356 | thanks, changed as suggestion. |
llvm/test/CodeGen/PowerPC/aix-emit-tracebacktable-clobber-register.ll | ||
---|---|---|
12 | 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 | ||
---|---|---|
147 | delete blank line | |
148 | omit braces | |
171 | 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. | |
262 | delete blank line | |
llvm/tools/llvm-objdump/llvm-objdump.cpp | ||
1406 | 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.
llvm/tools/llvm-objdump/llvm-objdump.cpp | ||
---|---|---|
1406 | 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. |
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 | Did you miss this comment? | |
llvm/tools/llvm-objdump/XCOFFDump.cpp | ||
271 | 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. |
add --strict-whitespace for test case aix-emit-tracebacktable-clobber-register.ll @MaskRay
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
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 | ||
43 | 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. |
llvm/tools/llvm-objdump/ObjdumpOpts.td | ||
---|---|---|
43 | Makes sense to me (probably it should be a subsequent patch though). |
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]: are label, there start from column zero. and the test case use --strict-whitespace CHECK:00000000 (idx: 0) .AddNum[PR]: | |
llvm/tools/llvm-objdump/ObjdumpOpts.td | ||
43 | thanks. I will do it in subsequent patch for this comment. |