This is an archive of the discontinued LLVM Phabricator instance.

[AIX][XCOFF][Patch2] decode vector information and extent long table of the traceback table of the xcoff.
ClosedPublic

Authored by DiggerLin on Aug 24 2020, 7:36 AM.

Details

Summary
  1. decode the Vector extension if has_vec is set
  2. decode long table fields, if longtbtable is set.

There is conflict on the bit order of HasVectorInfoMask and HasExtensionTableMask between AIX os header and IBM aix compiler XLC.
In the /usr/include/sys/debug.h defines
static constexpr uint32_t HasVectorInfoMask = 0x0040'0000;
static constexpr uint32_t HasExtensionTableMask = 0x0080'0000;
but the XLC defines as

static constexpr uint32_t HasVectorInfoMask = 0x0080'0000;
static constexpr uint32_t HasExtensionTableMask = 0x0040'0000;
we follows the definition of the IBM AIX compiler XLC here.

for the convenient to review , I copy some related code from the /usr/include/sys/debug.h of aix os
‘’‘
/*

  • Vector extension portion of the optional table (if has_vec is set). */

struct vec_ext {

unsigned vr_saved:6;    /* Number of non-volatile vector regs saved */
                        /* first register saved is assumed to be */
                        /* 32 - vr_saved                         */
unsigned saves_vrsave:1;/* Set if vrsave is saved on the stack */
unsigned has_varargs:1;
unsigned vectorparms:7; /* number of vector parameters */
unsigned vec_present:1; /* Set if routine performs vmx instructions */
unsigned char vecparminfo[4];/* bitmask array for each vector parm in */
                         /* order as found in the original parminfo, */
                         /* describes the type of vector:            */
                         /*       b'00 = vector char                 */
                         /*       b'01 = vector short                */
                         /*       b'10 = vector int                  */
                         /*       b'11 = vector float                */

};

/*

  • Optional portions of procedure-end table. *
  • Optional portions exist in the following order independently, not as
  • 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*/

};’‘’

Diff Detail

Event Timeline

DiggerLin created this revision.Aug 24 2020, 7:36 AM
DiggerLin requested review of this revision.Aug 24 2020, 7:36 AM
DiggerLin edited the summary of this revision. (Show Details)Aug 24 2020, 7:46 AM
DiggerLin edited the summary of this revision. (Show Details)
DiggerLin retitled this revision from [AIX][XCOFF] decode vector information and extent long table of the traceback table of the xcoff. to [AIX][XCOFF][Patch2] decode vector information and extent long table of the traceback table of the xcoff..Aug 24 2020, 7:49 AM
DiggerLin edited the summary of this revision. (Show Details)Aug 24 2020, 12:12 PM
DiggerLin updated this revision to Diff 287652.Aug 25 2020, 6:26 AM
jasonliu added inline comments.Sep 15 2020, 1:31 PM
llvm/include/llvm/BinaryFormat/XCOFF.h
349–370

I think with the two newly added fields, these two fields are not necessary. We just need to tweak parseParmsType a little bit so that it could uses the ParmTypeIsFloatingBits and ParmTypeIsDoubleBits below instead?

llvm/include/llvm/Object/XCOFFObjectFile.h
401

If this constructor is only intended to get called by XCOFFTracebackTable, could we make the constructor private and mark XCOFFTracebackTable as a friend in this class?

407

Remove this declaration if it's not used.

426

The naming of XTBTable is not very self-explanatory here.
And I'm not sure if this field is actually useful, as this is really like a placeholder field. We don't really know what it is or what it does.
It's guaranteed later we will find out what it actually does, then we would need to update it to something else. So should we just skip this field?

llvm/lib/Object/XCOFFObjectFile.cpp
851

nit: it might be better just spell it out as getNumberOfVRSaved so that it matches the field name below.

862

nit: getNumberOfVectorParms

876

It would be better if I's type matches the return types of getNumOfVectorParms(), which is uint8_t.

902

If we are going to separate it into two functions, why don't we also passing in the number of vector parameters as parameter here? So that the while loop could just check if the total number of parameter is reached instead of using || Value. And you don't need the bool Begin as well.

915

nit: ++I would be better here?

917

nit: I don't see a blank line very often after a break in the switch.

930

Let's have a default statement and assert here, so that we don't get surprised.

1031

I feel how we get the vector extension portion of the optional table is a bit wierd.
This extension part is an all or nothing, but we takes two steps to get the data out of it.

Could we do something like
VecExt = TBVectorExt(DE.getBytes(Cur, TBVectorExtLength));

Then do some extra conversion inside of the constructor?

llvm/unittests/Object/XCOFFObjectFileTest.cpp
214–216

I noticed we always test the same result from these three fields, is it possible to edit the traceback table a little bit to get it to return a different result?

DiggerLin marked 13 inline comments as done.Oct 13 2020, 1:55 PM
DiggerLin added inline comments.
llvm/include/llvm/BinaryFormat/XCOFF.h
349–370

I will change the Macro as
static constexpr uint32_t ParmTypeIsFloatingBit = 0x8000'0000;
-->
static constexpr uint32_t ParmTypeIsFixedBit = 0x8000'0000;

and

If I change function parseParmsType as

static SmallString<32> parseParmsType(uint32_t Value, unsigned ParmsNum) {

SmallString<32> ParmsType;
for (unsigned I = 0; I < ParmsNum; ++I) {
  if (I != 0)
    ParmsType += ", ";
  if ((Value & TracebackTable::ParmTypeIsFixedBit) == 0) {
    // Fixed parameter type.
    ParmsType += "i";
    Value <<= 1;
  } else {
    switch (Value & TracebackTable::**ParmTypeMask**) {
       case TracebackTable::ParmTypeIsFloatingBits:
           ParmsType += "f";
       case  TracebackTable::ParmTypeIsDoubleBits
            ParmsType += "d";
    Value <<= 2;
  }
}
return ParmsType;

}

I do not think using Macro ParmTypeMask is a good idea hear. for the parameter type without vector info , the ParmType is not always 2bits.

llvm/include/llvm/Object/XCOFFObjectFile.h
401

OK, thanks

407

thanks

426

I unsigned char xtbtable; /* More tbtable fields, if longtbtable is set*/

I changed the name to ExtLongTBTable.

llvm/lib/Object/XCOFFObjectFile.cpp
902

the Vector parameter is not included in the ParmsNum. I think we do not know the number of vector parameter untill we parse the VecExt = TBVectorExt(VRData, VecParmsInfo);

1031

for the definition of

struct vec_ext {

unsigned vr_saved:6;    /* Number of non-volatile vector regs saved */
                        /* first register saved is assumed to be */
                        /* 32 - vr_saved                         */
unsigned saves_vrsave:1;/* Set if vrsave is saved on the stack */
unsigned has_varargs:1;
unsigned vectorparms:7; /* number of vector parameters */
unsigned vec_present:1; /* Set if routine performs vmx instructions */
unsigned char vecparminfo[4];/* bitmask array for each vector parm in */
                         /* order as found in the original parminfo, */
                         /* describes the type of vector:            */
                         /*       b'00 = vector char                 */
                         /*       b'01 = vector short                */
                         /*       b'10 = vector int                  */
                         /*       b'11 = vector float                */

};

for the first 5 element of the structure vec_ext are bit fields of 2 bytes data.
the vecparminfo is 4 bytes. If I do as as your suggest , I still split the 6 bytes data into uint16_t and uint32_t when parsing the data in the TBVectorExt.

DiggerLin updated this revision to Diff 297953.Oct 13 2020, 2:01 PM
DiggerLin marked 6 inline comments as done.

address comment

jasonliu added inline comments.Nov 11 2020, 8:32 AM
llvm/include/llvm/BinaryFormat/XCOFF.h
349

Suggest to add a comment to separate the two groups of masks.

351
llvm/include/llvm/Object/XCOFFObjectFile.h
410

nit: move friend declaration to the line just below class TBVectorExt {

llvm/lib/Object/XCOFFObjectFile.cpp
851
929–930
1031

Instead of splitting it here, it would make more sense to do that inside of TBVectorExt().

1036

Since we already have the query hasExtensionTable(), it would make sense to call this field ExtensionTableFlags, and its getter getExtensionTableFlags.

llvm/unittests/Object/XCOFFObjectFileTest.cpp
191

If we have an extension table, it doesn't make sense that we do not set any flags in it. Maybe we could put a canary flag into the data and check it here:

#define TB_SSP_CANARY 0x20  /* stack smasher canary present on stack */
224

Same above for setting ExtensionTable, but not having any value in it.

DiggerLin marked 7 inline comments as done.Nov 12 2020, 9:03 AM
DiggerLin added inline comments.
llvm/lib/Object/XCOFFObjectFile.cpp
1036

we already has
if (Cur && hasTraceBackTableOffset())

TraceBackTableOffset = DE.getU32(Cur);

if (Cur && hasExtensionTable())

ExtensionTable = DE.getU8(Cur);
DiggerLin updated this revision to Diff 304864.Nov 12 2020, 9:08 AM
DiggerLin marked an inline comment as done.

address comment

jasonliu added inline comments.Nov 13 2020, 7:34 AM
llvm/include/llvm/Object/XCOFFObjectFile.h
402

It doesn't really make sense to pass it as const &.
Please see https://llvm.org/docs/ProgrammersManual.html#the-stringref-class for the last sentence.

llvm/lib/Object/XCOFFObjectFile.cpp
929–930

This comment is not addressed yet.

jasonliu added inline comments.Nov 16 2020, 8:30 AM
llvm/unittests/Object/XCOFFObjectFileTest.cpp
172

This is a mask that's likely to get reused somewhere else when we try to decoding and encoding the traceback table content. Could we put that set of masks in the XCOFF.h header?

DiggerLin marked 3 inline comments as done.Nov 18 2020, 6:02 AM
DiggerLin updated this revision to Diff 306085.Nov 18 2020, 6:13 AM

address comment

DiggerLin updated this revision to Diff 306126.Nov 18 2020, 8:31 AM
jasonliu accepted this revision.Nov 18 2020, 8:42 AM

Thanks. LGTM.
Could you create a new revision that contains the context for future reference?

This revision is now accepted and ready to land.Nov 18 2020, 8:42 AM

upload a revision with context.

This revision was landed with ongoing or failed builds.Nov 19 2020, 7:24 AM
This revision was automatically updated to reflect the committed changes.