This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by DiggerLin on Jun 10 2020, 9:38 AM.

Details

Summary
  1. This patch provided API for decoding the traceback table info and unit test for the these API.
  2. Another patchs will do the following things:

    2.1 added a new option --traceback-table to decode the trace back table information for xcoff object file when

using llvm-objdump to disassemble the xcoff objfile.

2.2 print out the  traceback table information for llvm-objdump.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
DiggerLin marked 2 inline comments as done.Jul 15 2020, 6:53 AM
DiggerLin added inline comments.
llvm/lib/Object/XCOFFObjectFile.cpp
882

I added a data member "TBTSize" to store the valid traceback table size and API for it.
When the caller want to print out the table content is not recognized , it can not the api the get the valid size.

922

yes, we will have a patch for vector info and long table after the patch.

jasonliu added inline comments.Jul 16 2020, 7:54 AM
llvm/lib/Object/XCOFFObjectFile.cpp
882

Sounds like we are not planning to treat extra bytes at the end of Traceback Table as an error, which kind of makes sense as we already parsed all the traceback table entries we know and it looks good.
Regarding on how we return the parsed traceback table size to the caller, instead of using a member function getTBTSize, have we thought about modify the create/constructor to have uint64_t *Size instead of uint64_t Size, so that the size could be passed back via the parameter? And we would need extra documentations for that as well. An example would be what we used here for DataExtractor: https://llvm.org/doxygen/DataExtractor_8h_source.html#l00100
Also we would want to have a test to test this behavior.

DiggerLin marked 3 inline comments as done.Jul 16 2020, 12:04 PM
DiggerLin added inline comments.
llvm/lib/Object/XCOFFObjectFile.cpp
882

yes. we do not want to planning to treat extra bytes at the end of Traceback Table as an error(the Caller need to deal it , the caller can decide to look it a warning or error. the api also provide API).

if return the parsed traceback table size to the caller through the construct, it means the caller can not construct the XCOFFTracebackTable from a constant . and If the caller do not want the size to be modified, the caller has to has local copy variable first and use the

And I think we already to document here
uint64_t TBTSize; // The size of the XCOFFTracebackTable.

jasonliu added inline comments.Jul 16 2020, 12:20 PM
llvm/lib/Object/XCOFFObjectFile.cpp
882

The concern I have with using a member function is that this functionality is very easy to get ignored. Caller would not know such API is provided for this purpose until they actually read though all the member functions and what they do. Requesting pass a non-const pointer to a size would make it very obvious that the callee would modify the size to provide feedback.
I don't think a local copy of variable to be a big problem though. In fact, saving it as a data member means you would copy it every time in the callee side, no matter caller wants it or not.

DiggerLin updated this revision to Diff 278765.Jul 17 2020, 7:36 AM
DiggerLin marked an inline comment as done.
jasonliu added inline comments.Jul 17 2020, 9:32 AM
llvm/include/llvm/Object/XCOFFObjectFile.h
411

I think a pointer to Size is better than reference to Size in this case. Reference is too subtle for this scenario and caller might not notice this is taken by reference.
And as mentioned in the other comment, a comment and a test case is needed for this function.

For the comment, I would suggest:

/// Parse a XCOFF Traceback Table from \a Ptr with \a Size bytes.
/// Returns a XCOFFTracebackTable upon successful parsing, otherwise an Error is returned.
///
/// \param[in] Ptr
///    A pointer points to the beginning of XCOFF Traceback Table (right after the initial 4 bytes of zeros).
/// \param[in, out] Size
///    The length of the XCOFF Traceback Table. 
///    If XCOFF Traceback Table is not parsed successfully or there are extra bytes that are not recognized,
///    \a Size will be updated to reflect the actual length of the XCOFF Traceback Table that get parsed.

Also please include a test that the XCOFFTracebackTable have extra content upon a successful parsing, and check if the Size gets modified correctly to the content that we are able to parse.

llvm/lib/Object/XCOFFObjectFile.cpp
857

both continue (here and in else) seems redundant for the current control flow.

DiggerLin marked 4 inline comments as done.Jul 17 2020, 1:48 PM
DiggerLin added inline comments.
llvm/include/llvm/Object/XCOFFObjectFile.h
411

change as suggestion , the test case already there .

EXPECT_EQ(Size, 25u);

the original Size is 28 bytes.

llvm/lib/Object/XCOFFObjectFile.cpp
857

thanks

DiggerLin updated this revision to Diff 278895.Jul 17 2020, 1:48 PM
DiggerLin marked 2 inline comments as done.
jasonliu accepted this revision.Jul 17 2020, 1:58 PM

LGTM with minor nit.

llvm/include/llvm/Object/XCOFFObjectFile.h
411

nit:
XCOFF pronounces "ex", so it should be an XCOFF.
The same for Returns an XCOFF... below.
Sorry for the churn.

This revision is now accepted and ready to land.Jul 17 2020, 1:58 PM
llvm/include/llvm/BinaryFormat/XCOFF.h
313

The "less" is part of a compound adjective and is not a separate word:
s/IsTOCLessMask/IsTOClessMask/;

314

Here and below: I don't see an advantage to saving three characters by using "FloatPoint" instead of "FloatingPoint".

315

The name should be IsFloatingPointOperationLogOrAbortEnabledMask.

321

This is not a boolean value. Please fix.

llvm/include/llvm/BinaryFormat/XCOFF.h
328

This is a boolean and the value expresses whether the function is fixup code, so IsFixupMask is appropriate.

329

Here and below: Please use "FPR" to denote floating-point registers instead of "FP".

335

Here and below: Please use "GPR" to denote general-purpose registers instead of just "GP".

339

Here and below: Please use "Parms" or "Params" ("Parms" does appear below). "Para" is not used by any of the three references I have and I am not aware of it being a common short form for "parameter".

343

Noting more instances of "FloatPoint" to replace here.

344

This is a boolean value: HasParmsOnStackMask.

hubert.reinterpretcast requested changes to this revision.Jul 17 2020, 10:12 PM
hubert.reinterpretcast added inline comments.
llvm/include/llvm/BinaryFormat/XCOFF.h
347

// Masks to select leftmost bits for decoding parameter type information.

348

This is named opposite to its meaning under the usual convention of zero meaning "false" with respect to the indicated property. Suggestion: ParmTypeIsFloatingBit.

349

ParmTypeFloatingIsDoubleBit

llvm/lib/Object/XCOFFObjectFile.cpp
893

I do not believe that it is okay for this to just parse incorrectly when hasVectorInfo() is true.

This revision now requires changes to proceed.Jul 17 2020, 10:12 PM
llvm/include/llvm/Object/XCOFFObjectFile.h
403

Why a separate field for the length of the function name? The StringRef for the function name has a length field.

416

s/pointer points/pointer that points/;
s/of XCOFF/of an XCOFF/;

419

s/pointer points/pointer that points/;

422

s/get/got/;

Is the value of Size modified or not when create reports an error? If modified, does Size represent the last successfully parsed field?

434

Note: More instances of "FloatPoint" here.

435

The function names have fallen out of sync with the mask naming.

440

This name is very different from the name we are using for the associated mask. Please go through the names and resync.

451

Same comments here as for the mask names.

DiggerLin updated this revision to Diff 279095.Jul 19 2020, 9:38 AM
DiggerLin marked 26 inline comments as done.

address comment

llvm/include/llvm/BinaryFormat/XCOFF.h
321

thanks

llvm/include/llvm/Object/XCOFFObjectFile.h
403

good idea.

llvm/lib/Object/XCOFFObjectFile.cpp
893

yes, when hasVectorInfo() is true , there is a different version of parse parameter type for the vector info , I will implement them is a following up patch, I added comment here.

llvm/include/llvm/Object/XCOFFObjectFile.h
414

Let's be consistent with the value of the offset field that the beginning of a traceback table is the initial 4 bytes of zeros:
A pointer that points just past the initial 4 bytes of zeros at the beginning of an XCOFF Traceback Table.

421

Nit: It seems the previous line ended prematurely?

llvm/lib/Object/XCOFFObjectFile.cpp
888

Please add a comment to indicate that we are skipping the required initial 8 bytes here.

890

I see no attempt here to verify that the read of the initial 8 bytes has not failed.

894

When checking values that are not semantically boolean values, I would suggest explicitly writing the comparison against zero.

933

I am not sure that functions that perform a read via a pointer to non-owned memory should be named "get" functions as opposed to "read" functions.

llvm/lib/Object/XCOFFObjectFile.cpp
888

In keeping with the ELF ABI supplement document, we can describe the skipping of the "8 bytes of mandatory fields".

I've been very busy the past couple of weeks, but should be able to come back to look at this in the next day or two. Please don't land it before I get a chance to.

DiggerLin marked 7 inline comments as done.Jul 20 2020, 6:44 AM
DiggerLin updated this revision to Diff 279231.Jul 20 2020, 7:01 AM
DiggerLin marked an inline comment as done.

address comment

llvm/lib/Object/XCOFFObjectFile.cpp
933

I think maybe better to keep name "get" functions .
if the function has argument as
getVersion(void* prt, uint64_t size), it maybe better to change to readVersion(void* prt, uint64_t size).

but there is no argument here, keep getVersion myabe better.

llvm/lib/Object/XCOFFObjectFile.cpp
933

Another possibility for the name is "extract". @DiggerLin, please see if @jhenderson has an opinion on this naming aspect.

llvm/lib/Object/XCOFFObjectFile.cpp
887

Please reduce the scope of ParmNum by merging the adjacent blocks that set and use it.

899

Please add a comment that, as long as there are no "fixed-point" or floating-point parameters, this field remains not present even when hasVectorInfo gives true and indicates the presence of vector parameters.

This statement is true even when GCC counts vector (float!) parameters as "fixed-point" (and does not encode vector parameter information separately).

llvm/include/llvm/Object/XCOFFObjectFile.h
443

There's no query corresponding to IsFixupMask?

llvm/lib/Object/XCOFFObjectFile.cpp
911

This should reserve the number of entries beforehand (since that number is known).

912

Minor nit: Let's be consistent in using ++I.

914

Please use std::move.

927

Minor nit: s/Info/info/;

974

The capitalization of this function name does not match that of the other functions.

@DiggerLin, for context (since I will be on vacation), I've gotten through the functional code changes, but have not reviewed the testing.

I'm out of time to look at the tests today. I'll come back to them, hopefully later in the week.

llvm/include/llvm/BinaryFormat/XCOFF.h
320

Does this name reflect a name from a specification? I'd expect it to be IsFunctionNamePresentMask or possibly HasFunctionNameMask otherwise ("is" expects an adjective, i.e. "present", whereas has would expect a noun, i.e. "name").

350

I don't have any XCOFF knowledge, but should this be ParmTypeIsFloatingDoubleBit?

llvm/include/llvm/Object/XCOFFObjectFile.h
420–421

I think the following would be slightly clearer:

"\a Size will be updated to be the size up to the end of the last succsessfully parsed field of the table."

437

Equivalent comment to mask name.

llvm/lib/Object/XCOFFObjectFile.cpp
877

Do you actually need the std::move here?

886

I made a typo in my original comment. The style for this should be /*IsLittleEndian=*/false and /*AddressSize=*/0 without any spaces.

890

I don't think the return 0 behaviour of Cursor in the event of an error stops you using it. You can still do if (C) to check whether you should do a thing, just like you do with if (!Err). For example:

DataExtractor::Cursor C(Offset);
DE.getU64(C);
if (C)
  ParmNum = getNumberOfFixedParms() + getNumberOfFPParms();
if (C && ParmNum > 0 && !hasVectorInfo())
  ParmsType = parseParmsType(DE.getU32(C), ParmNum);
if (C && hasTraceBackTableOffset())
  TraceBackTableOffset = DE.getU32(C);

...
Offset = C.tell();
Err = C.takeError();

It doesn't save much, but I find the get* functions easier to read that way.

933

I don't have any particular opinions on this. I think extractVersion or readVersion might be slightly better than getVersion etc because it shows that it's not just a simple getter of a private member or similar.

DiggerLin marked 14 inline comments as done.Jul 21 2020, 7:23 AM

@DiggerLin, for context (since I will be on vacation), I've gotten through the functional code changes, but have not reviewed the testing.

OK, thanks hubert.

llvm/include/llvm/Object/XCOFFObjectFile.h
443

yes, it is a reserved bit. I am not sure is there anyone to require a reserved bit.

llvm/lib/Object/XCOFFObjectFile.cpp
877

yes. I think so. there is iImplicitly-declared move constructor for XCOFFTracebackTable , and there is there are several Optional type of data member, for example, SmallString has move constructor.

887

thanks

911

thanks

914

thanks.

974

thanks

DiggerLin marked 14 inline comments as done.Jul 21 2020, 8:03 AM
DiggerLin added inline comments.
llvm/include/llvm/BinaryFormat/XCOFF.h
320

thanks. it maybe better to change to IsFunctionNamePresentMask

350

I think the name "ParmTypeFloatingIsDoubleBit" express the spec better.
I copied some content from the file of /usr/include/sys/debug.h of aix OS.

  • a structure or an union. Whether or not portions exist is determinable
  • from bit-fields within the base procedure-end table. *
  • parminfo exists if fixedparms or floatparms != 0.
  • tb_offset exists if has_tboff bit is set.
  • hand_mask exists if int_hndl bit is set.
  • ctl_info exists if has_ctl bit is set.
  • ctl_info_disp exists if ctl_info exists.
  • name_len exists if name_present bit is set.
  • name exists if name_len exists.
  • alloca_reg exists if uses_alloca bit is set.
  • vec_ext exists if has_vec bit is set. */

struct tbtable_ext {

unsigned int parminfo;  /* Order and type encoding of parameters:
                         * Left-justified bit-encoding as follows:
                         * '0'  ==> fixed parameter
                         * '10' ==> single-precision float parameter
                         * '11' ==> double-precision float parameter
                         *
                         * if has_vec is set, encoded as follows:
                         * '00' ==> fixed parameter
                         * '01' ==> vector parameter
                         * '10' ==> single-precision float parameter
                         * '11' ==> double-precision float parameter
                         */
unsigned int tb_offset; /* Offset from start of code to tb table */
int hand_mask;          /* What interrupts are handled by */
int ctl_info;           /* Number of CTL anchors, followed by */
int ctl_info_disp[1];   /* Actually ctl_info_disp[ctl_info] */
                        /* Displacements into stack of each anchor */
short name_len;         /* Length of procedure name */
char name[1];           /* Actually char[name_len] (no NULL) */
char alloca_reg;        /* Register for alloca automatic storage */
struct vec_ext vec_ext; /* Vector extension (if has_vec is set) */
unsigned char xtbtable; /* More tbtable fields, if longtbtable is set*/

};

llvm/lib/Object/XCOFFObjectFile.cpp
890

thanks

DiggerLin updated this revision to Diff 279616.Jul 21 2020, 1:08 PM
DiggerLin marked 6 inline comments as done.Jul 21 2020, 1:08 PM

address comment

DiggerLin added inline comments.Jul 22 2020, 10:59 AM
llvm/include/llvm/Object/XCOFFObjectFile.h
443

sorry, in the old document , it is reserved bit, in the /usr/include/sys/debug.h , it is not.

jhenderson added inline comments.Jul 23 2020, 1:44 AM
llvm/include/llvm/Object/XCOFFObjectFile.h
421

Always check my text for typos :)

succsessfully -> successfully

llvm/lib/Object/XCOFFObjectFile.cpp
872–874

You pass in the Size as a pointer here, yet it can never be null, and is immediately dereferenced. I think it would make sense to use a reference.

877

In general terms, you should never need to std::move a local variable in a return statement. This is because it inihibits return-value optimizations. In return statements, the compiler automatically elides the copy or move, if it can. Explicitly specifying std::move prevents the compiler doing this. (See https://stackoverflow.com/questions/17473753/c11-return-value-optimization-or-move and various other resources on the internet). The presence of move constructors (whether implicitly declared or not) is irrelevant here.

That being said, at least with some compilers, when performing an implicit conversion on return (as happens here), you may need a std::move (see https://stackoverflow.com/questions/17481018/when-is-explicit-move-needed-for-a-return-statement). This is something that should disappear at some point, but I suspect our C++14 support might make it impossible.

Basically, try removing it, and building with different compilers, and see what happens.

llvm/unittests/Object/XCOFFObjectFileTest.cpp
19

This test is simple enough that I'd just delete this blank line. I'd also show the behaviour where there are too few bytes, and too many bytes.

102

It's possible that Size might have been changed by the previous call to create, so it's not safe to reuse it here. It might make more sense to split each of these cases up into a separate test, with a fixture providing the common stuff for reuse.

143

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.

DiggerLin marked 8 inline comments as done.Jul 23 2020, 7:55 AM
DiggerLin added inline comments.
llvm/lib/Object/XCOFFObjectFile.cpp
877

OK, thanks

llvm/unittests/Object/XCOFFObjectFileTest.cpp
19

thanks

DiggerLin updated this revision to Diff 280119.Jul 23 2020, 7:57 AM
DiggerLin marked 2 inline comments as done.

address comment

DiggerLin accepted this revision.Jul 29 2020, 7:27 AM

Any new comments ?

This comment was removed by DiggerLin.
jhenderson added inline comments.Jul 31 2020, 12:43 AM
llvm/unittests/Object/XCOFFObjectFileTest.cpp
111

Nit: clang-format is complaining here. Please make sure to reformat all your new code.

143

This hasn't been addressed?

DiggerLin marked 2 inline comments as done.Aug 5 2020, 7:57 AM
DiggerLin added inline comments.
llvm/unittests/Object/XCOFFObjectFileTest.cpp
143

added a new test case for it.

DiggerLin updated this revision to Diff 283248.Aug 5 2020, 7:58 AM
DiggerLin marked an inline comment as done.

added a truncated test case

jhenderson added inline comments.Aug 6 2020, 12:25 AM
llvm/unittests/Object/XCOFFObjectFileTest.cpp
143

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.

DiggerLin marked an inline comment as done.Aug 6 2020, 1:12 PM
DiggerLin added inline comments.
llvm/unittests/Object/XCOFFObjectFileTest.cpp
143

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,

  1. 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.
llvm/include/llvm/Object/XCOFFObjectFile.h
419–421

Minor nit: Add "the" before XCOFF.

llvm/lib/Object/XCOFFObjectFile.cpp
887

My request was to have the scope of ParmNum reduced. That is, please move the declaration of ParmNum to where it is set and used (and also arrange for it to be in a smaller block scope that does not extend much further than the point where ParmNum is used).

904

Even with hasVectorInfo, the field may be present. Should that happen, the field still needs to be read (even if not interpreted). I assume we are missing a test case where hasVectorInfo is true.

919

We should not update the field unless if Cur is still valid.

997

Please rename the "get" functions to be "read" functions as indicated by previous comments.

DiggerLin marked 4 inline comments as done.Aug 7 2020, 8:42 AM
DiggerLin added inline comments.
llvm/lib/Object/XCOFFObjectFile.cpp
887

thanks

DiggerLin updated this revision to Diff 283915.Aug 7 2020, 8:52 AM
DiggerLin marked an inline comment as done.

address comment

jhenderson added inline comments.Aug 10 2020, 2:02 AM
llvm/unittests/Object/XCOFFObjectFileTest.cpp
143

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.

DiggerLin updated this revision to Diff 284391.Aug 10 2020, 8:36 AM

add new test case.

jhenderson added inline comments.Aug 11 2020, 2:44 AM
llvm/unittests/Object/XCOFFObjectFileTest.cpp
163

Rather than repeatedly declaring an ever-increasing array, but otherwise identical array you should pull it out into a constant variable shared by the tests, and then use a different size in each case. Thus something like:

const uint8_t BasicTable = { ... };

TEST(XCOFFObjectFileTest, XCOFFTracebackTableTruncatedAtMandatory) {
  Expected<XCOFFTracebackTable> TTOrErr = XCOFFTracebackTable::create(BasicTable, 0x6);
  EXPECT_THAT_ERROR(
      TTOrErr.takeError(),
      FailedWithMessage(
          "unexpected end of data at offset 0x6 while reading [0x0, 0x8)"));
}

TEST(XCOFFObjectFileTest, XCOFFTracebackTableTruncatedAtParamsType) {
  Expected<XCOFFTracebackTable> TTOrErr = XCOFFTracebackTable::create(BasicTable, 9);
  EXPECT_THAT_ERROR(
      TTOrErr.takeError(),
      FailedWithMessage(
          "unexpected end of data at offset 0x9 while reading [0x8, 0xc)"));
}

etc.

You might even be able to use the table in some of your earlier tests too.

DiggerLin marked an inline comment as done.Aug 11 2020, 6:00 AM
DiggerLin added inline comments.
llvm/unittests/Object/XCOFFObjectFileTest.cpp
163

I think I can not do that, some bytes of the V[] is different.

DiggerLin marked an inline comment as done.Aug 11 2020, 6:18 AM
DiggerLin added inline comments.
llvm/unittests/Object/XCOFFObjectFileTest.cpp
163

if there are V[] is same , I will change as your suggestion.

DiggerLin updated this revision to Diff 284714.Aug 11 2020, 7:30 AM

address comment

llvm/include/llvm/Object/XCOFFObjectFile.h
396

Add Doxygen-style comment for the class to explain that this class provides methods to extract traceback table data from a buffer and that the various accessors may continue to reference the buffer provided via the constructor.

397

I don't think this should be modified following initialization in the constructor.

llvm/lib/Object/XCOFFObjectFile.cpp
892–905

The scope of ParmNum can be further reduced, which I want because ParmNum does not encompass the number of vector parameters. Additionally, "Params" is not consistent with the use of "Parms" in most places of this patch.

942

Use defensive parentheses for references to macro parameters.

944–945

Use defensive parentheses for references to macro parameters and for the definition of the macro.

997

Sorry for the churn. I think we can go with just naming these "get" and documenting the class better, especially considering the awkwardness of renaming the "is", etc. functions as well (since those functions also read from the buffer).

Please update for the other "read" functions as well (so we don't leave a mix of "read" and "get" functions that are different in naming but not really different in nature).

llvm/unittests/Object/XCOFFObjectFileTest.cpp
82

s/Params/Parms;

172

s/Params/Parms;

jhenderson added inline comments.Aug 12 2020, 12:29 AM
llvm/unittests/Object/XCOFFObjectFileTest.cpp
127

Fix the name here please. I'm not sure I even understand what V_T stands for... How about TTableData?

151–160

The inputs to this test are identical to the previous case above. Could you simply merge the two together?

159

There's nothing after this in the test that depends on this line. No need for it to be an ASSERT_FALSE.

DiggerLin marked 6 inline comments as done.Aug 12 2020, 6:53 AM
DiggerLin added inline comments.
llvm/unittests/Object/XCOFFObjectFileTest.cpp
151–160

the two test case has different test case name, One to test XCOFFTracebackTableAPI for HasVectorInfo , Other to test XCOFFTracebackTableAPI for ControlledStorageInfoDisp. If merge, it is difficult to have a good test case name here.

DiggerLin updated this revision to Diff 285086.Aug 12 2020, 7:39 AM
DiggerLin marked an inline comment as done.

address comment

jhenderson added inline comments.Aug 13 2020, 12:15 AM
llvm/include/llvm/Object/XCOFFObjectFile.h
396–398
llvm/lib/Object/XCOFFObjectFile.cpp
898

Can you drop the Cur && from here? It looks like there's no way it could be invalid at this point.

llvm/unittests/Object/XCOFFObjectFileTest.cpp
147

You need an ASSERT_EQ(Disp.size(), 2) before this check.

151–160

I guess when vector info parsing is expanded, this second test will get expanded, but the first won't be?

DiggerLin marked 3 inline comments as done.Aug 13 2020, 7:07 AM
DiggerLin added inline comments.
llvm/lib/Object/XCOFFObjectFile.cpp
898

thanks

llvm/unittests/Object/XCOFFObjectFileTest.cpp
151–160

yes, the next patch will parse the VectorInfo expand info, the decode of parameters type with VectorInfo is true.

DiggerLin updated this revision to Diff 285368.Aug 13 2020, 7:34 AM
DiggerLin marked 2 inline comments as done.

address comment

llvm/lib/Object/XCOFFObjectFile.cpp
934

Minor nit: s/is/is one/;

942

The second set of parentheses I requested has not been added.

944–945

I requested three sets of parentheses that are still not present.

llvm/unittests/Object/XCOFFObjectFileTest.cpp
180

The Size should match the first value, right? Can we check that for each of the truncated cases?

235

s/Funciton/Function/;

No more comments from me at this point. Thanks!

DiggerLin updated this revision to Diff 285648.Aug 14 2020, 7:20 AM
DiggerLin marked an inline comment as done.

address comment

This revision is now accepted and ready to land.Aug 14 2020, 12:56 PM
This revision was landed with ongoing or failed builds.Aug 17 2020, 1:24 PM
This revision was automatically updated to reflect the committed changes.