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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/unittests/Object/XCOFFObjectFileTest.cpp:142
+  EXPECT_EQ(Disp[1], 0x06060000u);
+}
----------------
DiggerLin wrote:
> 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. 
> 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.
They may be similar logic, but that doesn't invalidate the need to test them. If somebody were to refactor the internal code, they might miss one or more of the untested cases that need checking. Adding tests for each case ensures the check is correct, both now and in the future.

> we do not need to DE.getU32(Cur) again.
> 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.

I agree we don't need to check the logic of these functions. I argue we need to check that they are being used correctly however. For example, there are multiple overloads of the functions, some which report errors and some which don't. If you use the wrong version, then the error will not be caught at the right place. Again, imagine a future refactoring, which for some reason removed the `Cursor` class. If they switched to using `getU32(OffsetPtr)`, instead of `getU32(OffsetPtr, &Err)`, the errors would be lost, but not necessarily noticed without testing for those errors at each point.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81585