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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/BinaryFormat/XCOFF.cpp:179-180
   // Two of the bits that haven't got used in the mask.
-  if (Flag & 0x06)
+  if (Flag & 0x06 || Flag == 0x0)
     Res += "Unknown ";
 
----------------
DiggerLin wrote:
> jhenderson wrote:
> > Is this change tightly linked to the rest of the patch, or could it be done first with an appropriate separate test case?
> The function "getExtendedTBTableFlagString" is used the patch, but the Flag == 0x0 we do not deal with before, so add the here to deal with by the way(I think Jason asked to add here if I remember correct). If you strong suggest that I should remove it.  If will remove it.
I think it should be its own separate patch. Ideally, if you can craft a test case, it could be a prerequisite patch to this one. If not, would would be the impact of moving this into a patch that lands after this one? Would it mean some strange fields in e.g. llvm-objdump output?


================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/disassemble-traceback-table.test:37
+
+
+# CHECK:00000000 (idx: 0) .AddNum:
----------------
Delete extra blank line.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.h:12-14
+#include "llvm/MC/MCSubtargetInfo.h"
 #include "llvm/Object/XCOFFObjectFile.h"
+#include "llvm/Support/FormattedStream.h"
----------------
DiggerLin wrote:
> jhenderson wrote:
> > Can you use forward declarations instead of additional includes here?
> using forward delcarations here  and include the file in XCOFFDump.cpp ? what is the benefit ?
It means that when other files include this header, they don't need to pull in all its dependencies. See https://llvm.org/docs/CodingStandards.html#include-as-little-as-possible.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89049