[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-350
   // Masks to select leftmost bits for decoding parameter type information.
   static constexpr uint32_t ParmTypeIsFloatingBit = 0x8000'0000;
   static constexpr uint32_t ParmTypeFloatingIsDoubleBit = 0x4000'0000;
+  static constexpr uint32_t ParmTypeIsFixedBits = 0x0000'0000;
----------------
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?


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:401
+public:
+  TBVectorExt(uint16_t Data, uint32_t Vpi) : Data(Data), VecParmsInfo(Vpi) {}
+  uint8_t geNumOfVRSaved() const;
----------------
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?


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:407
+  bool hasVMXInstruction() const;
+  uint32_t getVecParmsInfo() const;
+  SmallString<32> getVectorParmsInfoString() const;
----------------
Remove this declaration if it's not used.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:424
+  Optional<TBVectorExt> VecExt;
+  Optional<uint8_t> XTBTable;
 
----------------
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?


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:851
+  ((Data & (TracebackTable::X)) >> (TracebackTable::S))
+uint8_t TBVectorExt::geNumOfVRSaved() const {
+  return GETVALUEWITHMASKSHIFT(NumberOfVRSavedMask, NumberOfVRSavedShift);
----------------
nit: it might be better just spell it out as `getNumberOfVRSaved` so that it matches the field name below.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:862
+}
+uint8_t TBVectorExt::getNumOfVectorParms() const {
+  return GETVALUEWITHMASKSHIFT(NumberOfVectorParmsMask,
----------------
nit: `getNumberOfVectorParms`


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:876
+  uint32_t Value = VecParmsInfo;
+  for (unsigned I = 0; I < getNumOfVectorParms(); ++I) {
+    if (I != 0)
----------------
It would be better if `I`'s type matches the return types of `getNumOfVectorParms()`, which is uint8_t.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:902
+static SmallString<32> parseParmsTypeWithVecInfo(uint32_t Value,
+                                                 unsigned int ParmsNum) {
+  SmallString<32> ParmsType;
----------------
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.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:915
+      ParmsType += "i";
+      I++;
+      break;
----------------
nit: ++I would be better here?



================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:917
+      break;
+
+    case TracebackTable::ParmTypeIsVectorBits:
----------------
nit: I don't see a blank line very often after a `break` in the switch.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:930
+      I++;
+      break;
+    }
----------------
Let's have a default statement and assert here, so that we don't get surprised.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:1031
+      if (Cur)
+        VecExt = TBVectorExt(VRData, VecParmsInfo);
+    }
----------------
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?


================
Comment at: llvm/unittests/Object/XCOFFObjectFileTest.cpp:214-216
+  EXPECT_EQ(VecExt.geNumOfVRSaved(), 0);
+  EXPECT_TRUE(VecExt.isVRSavedOnStack());
+  EXPECT_FALSE(VecExt.hasVarArgs());
----------------
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? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86461