Page MenuHomePhabricator

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

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
llvm/include/llvm/Object/XCOFFObjectFile.h
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.Sun, Jul 19, 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.Mon, Jul 20, 6:44 AM
DiggerLin updated this revision to Diff 279231.Mon, Jul 20, 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.Tue, Jul 21, 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.Tue, Jul 21, 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.Tue, Jul 21, 1:08 PM
DiggerLin marked 6 inline comments as done.

address comment

DiggerLin added inline comments.Wed, Jul 22, 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.Thu, Jul 23, 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.Thu, Jul 23, 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.Thu, Jul 23, 7:57 AM
DiggerLin marked 2 inline comments as done.

address comment

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

Any new comments ?

This comment was removed by DiggerLin.
jhenderson added inline comments.Fri, Jul 31, 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.Wed, Aug 5, 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.Wed, Aug 5, 7:58 AM
DiggerLin marked an inline comment as done.

added a truncated test case

jhenderson added inline comments.Thu, Aug 6, 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.Thu, Aug 6, 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.Fri, Aug 7, 8:42 AM
DiggerLin added inline comments.
llvm/lib/Object/XCOFFObjectFile.cpp
887

thanks

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

address comment