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

[Jason Liu via Phabricator via llvm-commits]

jasonliu added inline comments.


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:310
+  static constexpr uint32_t IsEprolMask = 0x0000'4000;
+  static constexpr uint32_t HasCodeLenMask = 0x0000'2000;
+  static constexpr uint32_t IntProcMask = 0x0000'1000;
----------------
DiggerLin wrote:
> jasonliu wrote:
> > In AIX OS header "/usr/include/sys/debug.h", we have this field as 
> > ```
> > unsigned has_tboff:1; /* Set if offset from start of proc stored */
> > ```
> > It might be better to rename HasCodeLenMask sot that it has a bit association with the original name? So that people do not need to reason about if this is the correct field or not.
> I think I will like to keep the HasFunctionCodeLenMask.
> for the we will give the function code lenth later depend in the the bit.
> Changing the code to  
> if (!Err & HasTraceBackTableOffset() 
>     CodeLen = DE.getU32(&OffsetPtr, &Err);
> 
> is not better than
> 
>   if (!Err && hasFunctionCodeLen())
>     CodeLen = DE.getU32(&OffsetPtr, &Err);
> 
I think my point is to have association with the original name, and hasFunctionCodeLen does not have association with the name in system header which could cause confusion. 


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:396
+  const uint8_t *TBPtr;
+  const uint64_t Size;
+  Optional<std::string> ParaType;
----------------
DiggerLin wrote:
> jasonliu wrote:
> > Do you actually need the Size as data member?
> we need to Size to know  whether the traceback table is long enough for the all for the fields.
But right now you only need to use it in the constructor and it was passed in as a parameter. So why do you need to save it as a data member? Or did I miss any other usage of `Size` as data member?


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:849
+static SmallString<32> parseParaType(uint32_t Value, unsigned ParaNum) {
+  SmallString<32> ParaType;
+  for (unsigned I = 0; I < ParaNum; ++I) {
----------------
Why always 32? As I mentioned in the other comment, ParaNum have implication for how large your SmallString could be. 


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:896
+    report_fatal_error("vector info, controlled storage info and extension "
+                       "table of traceback table not yet implemented");
+
----------------
I would hope we could skip the sections we do not want to parse for now gracefully instead of just report_fatal_error and stop parsing all together.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:850
+    if (I != 0)
+      ParaType += ", ";
+    if ((Value & TracebackTable::FixedParaTypeBit) == 0) {
----------------
DiggerLin wrote:
> jasonliu wrote:
> > Consider doing ParaType += "i, " and ParaType += "f, " ...
> > and do a removal of ", " after parsing all parameters.
> since we will use SmallString, The cost of deleting last ","  is  more expense than currently implement.
But you could avoid so many "if (I != 0)" condition which is not that efficient. 


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:904
+  if (!Err && isFuncNamePresent()) {
+    uint16_t Len = DE.getU16(&offset_ptr, &Err);
+    if (!Err)
----------------
DiggerLin wrote:
> jasonliu wrote:
> > Why do we need to declare a new variable?
> yes , we need it . it been use here 
> FunctionName = DE.getBytes(&offset_ptr, Len, &Err);
> 
> since after we get a value the point offset_ptr  moved, we can not get it second time.
What's wrong with 
```
    FunctionNameLen = DE.getU16(&OffsetPtr, &Err);
    if (!Err)
      FunctionName = DE.getBytes(&OffsetPtr, Len, &Err);
```
?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81585