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

[Jason Liu via Phabricator via llvm-commits]

jasonliu added inline comments.


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:349
   // Masks to select leftmost bits for decoding parameter type information.
   static constexpr uint32_t ParmTypeIsFloatingBit = 0x8000'0000;
   static constexpr uint32_t ParmTypeFloatingIsDoubleBit = 0x4000'0000;
----------------
Suggest to add a comment to separate the two groups of masks. 


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:351
   static constexpr uint32_t ParmTypeFloatingIsDoubleBit = 0x4000'0000;
+  static constexpr uint32_t ParmTypeIsFixedBits = 0x0000'0000;
+  static constexpr uint32_t ParmTypeIsVectorBits = 0x4000'0000;
----------------



================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:410
+
+  friend class XCOFFTracebackTable;
+};
----------------
nit: move friend declaration to the line just below `class TBVectorExt {`


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:851
+  ((Data & (TracebackTable::X)) >> (TracebackTable::S))
+uint8_t TBVectorExt::geNumberOfVRsSaved() const {
+  return GETVALUEWITHMASKSHIFT(NumberOfVRSavedMask, NumberOfVRSavedShift);
----------------



================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:929-930
+    default:
+      assert(true &&
+             "Should not come here in function parseParmsTypeWithVecInfo!");
+    }
----------------



================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:1036
+  if (Cur && hasExtensionTable())
+    ExtLongTBTable = DE.getU8(Cur);
 
----------------
Since we already have the query `hasExtensionTable()`, it would make sense to call this field `ExtensionTableFlags`, and its getter `getExtensionTableFlags`.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:1031
+      if (Cur)
+        VecExt = TBVectorExt(VRData, VecParmsInfo);
+    }
----------------
DiggerLin wrote:
> jasonliu wrote:
> > 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?
> 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. 
> 
Instead of splitting it here, it would make more sense to do that inside of TBVectorExt().


================
Comment at: llvm/unittests/Object/XCOFFObjectFileTest.cpp:191
+  ASSERT_TRUE(TT.getExtLongTBTable());
+  EXPECT_EQ(TT.getExtLongTBTable().getValue(), 0);
+
----------------
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 */ 
```


================
Comment at: llvm/unittests/Object/XCOFFObjectFileTest.cpp:224
+  ASSERT_TRUE(TT.getExtLongTBTable());
+  EXPECT_EQ(TT.getExtLongTBTable().getValue(), 0);
+
----------------
Same above for setting ExtensionTable, but not having any value in it. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86461