[PATCH] D81585: [AIX][XCOFF][Patch1] Provide decoding trace back table information API for xcoff object file for llvm-objdump -d

[Digger via Phabricator via llvm-commits]

DiggerLin marked an inline comment as done.
DiggerLin added inline comments.


================
Comment at: llvm/unittests/Object/XCOFFObjectFileTest.cpp:142
+  EXPECT_EQ(Disp[1], 0x06060000u);
+}
----------------
jhenderson wrote:
> DiggerLin wrote:
> > jhenderson wrote:
> > > jhenderson wrote:
> > > > You need testing for what happens when the table is truncated/malformed in such a way that the parsing fails. Currently, you do not test the failures. I think you'll need a test case for each point where the reading might fail if the size were too small.
> > > This hasn't been addressed?
> > added a new test case for it.
> This hasn't been fully addressed:
> 
> > I think you'll need a test case for each point where the reading might fail if the size were too small.
> 
> The new test only tests one specific truncation point. However, there are a large number of places where the data could be truncated within the XCOFFTracebackTable constructor, and this subsequently impacts the behaviour of the remainder of that function, due to the various `if (Cur && ...)` calls. Essentially, you should have one test case for each `getU32` etc function, to show that the `Cursor` is passed in, and that you aren't using some non error-handling version. Since there will be a large number of near-identical test cases, you probably want to use the TEST_P framework for passing in a parameterised test. The parameters would be the data size and expected error message. You can pass a size that is less than the data available, to allow you to share the data across test cases. For an example of something similar, look for the test cases to do with truncated opcodes in `DWARFDebugLineTest.cpp`.
I do not think we need to test for every truncated point.
I think we tested DE.getU64(Cur) , it is enough
Reason:
1. if it fail s,  it will test "if (Cur && ...)" , I do not think we need to test each if (Cur && ...) in the XCOFFTracebackTable construct . They are all the similar logic.
2. we do not need to DE.getU32(Cur) again.
when you looks into the getU32 and getU64, they both call DataExtractor::prepareRead(), the function is the only place can generate the Error,
3. There are a lot of  unit test cases which uses getU32() and getU64(), I do not think we need to create a truncated  test case to test getU32() again in the test case. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81585