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

[Digger via Phabricator via llvm-commits]

DiggerLin marked 14 inline comments as done.
DiggerLin added inline comments.


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:320
+  static constexpr uint32_t IsInterruptHandlerMask = 0x0000'0080;
+  static constexpr uint32_t HasFunctionNamePresentMask = 0x0000'0040;
+  static constexpr uint32_t IsAllocaUsedMask = 0x0000'0020;
----------------
jhenderson wrote:
> 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").
thanks. it maybe better to change to IsFunctionNamePresentMask 


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:350
+  static constexpr uint32_t ParmTypeIsFloatingBit = 0x8000'0000;
+  static constexpr uint32_t ParmTypeFloatingIsDoubleBit = 0x4000'0000;
+};
----------------
jhenderson wrote:
> I don't have any XCOFF knowledge, but should this be `ParmTypeIsFloatingDoubleBit`?
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*/
};


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:890
+
+  DE.getU64(&OffsetPtr, &Err);
+
----------------
jhenderson wrote:
> DiggerLin wrote:
> > jhenderson wrote:
> > > Not sure if you've come across it yet, but you could simplify all these calls using `DataExtractor::Cursor` instead. This stores the error internally, whilst also allowing you to continue parsing to the end, with no harmful effects (because nothing is read if `Cursor` is in an error state), similar to `Err` being passed in everywhere. It also tracks the offset. Usage is:
> > > 
> > > ```
> > > DataExtractor::Cursor C(/*Offset=*/0);
> > > DE.getU64(C); // By the way, what is this value for?
> > > ...
> > > // No need for ifs here.
> > > CodeLen = DE.getU32(C);
> > > HandlerMask = DE.getU32(C);
> > > ...
> > > 
> > > Err = C.takeError();
> > > ```
> > I considered to use Cursor before, but most of our data member are Optianl I think it maybe  better to keep current implement, using Cursor, it still get a value(for example zero), and the CodeLen, ParaType, FunctionName etc are Optional type. it will do  Optional &operator=(T &&y)  for all the fields(CodeLen HandlerMask etc,)  even if there is error before and if will always call parseParaType().   I do not think it is efficient.
> 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.
thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81585