[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 25 inline comments as done.
DiggerLin added inline comments.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:408
+                                              uint64_t Size);
+  XCOFFTracebackTable(const uint8_t *Ptr, uint64_t Size, Error &Err);
+
----------------
jhenderson wrote:
> Make this private so that all users must use the `create` function.
thanks


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:396
+  const uint8_t *TBPtr;
+  const uint64_t Size;
+  Optional<std::string> ParaType;
----------------
jasonliu wrote:
> 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?
thanks


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:883
+XCOFFTracebackTable::XCOFFTracebackTable(const uint8_t *Ptr,
+                                         const uint64_t Size, Error &Err)
+    : TBPtr(Ptr), Size(Size) {
----------------
jhenderson wrote:
> Delete the `const` for `Size`.
thanks


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:888
+                   /* AddressSize */ 0);
+  uint64_t OffsetPtr = 0;
+
----------------
jhenderson wrote:
> This isn't a pointer, so remove the `Ptr`!
thanks


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:890
+
+  DE.getU64(&OffsetPtr, &Err);
+
----------------
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.


================
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");
+
----------------
jasonliu wrote:
> 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.
I think report_fatal_error maybe reasonable here. if we skip the section , how does the user know a object file has  these section or not ?   if report a error , the user will know the llvm-objdump do not support the vector etc section and need to ask to developer the functionality. and I think we also will create a new patch to support vector etc .


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:850
+    if (I != 0)
+      ParaType += ", ";
+    if ((Value & TracebackTable::FixedParaTypeBit) == 0) {
----------------
jasonliu wrote:
> 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. 
I do not think it is a large number of loop. if it is a large number of loop, I will consider your suggestion.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:904
+  if (!Err && isFuncNamePresent()) {
+    uint16_t Len = DE.getU16(&offset_ptr, &Err);
+    if (!Err)
----------------
jasonliu wrote:
> jasonliu wrote:
> > 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);
> > ```
> > ?
> I meant
> ```
> FunctionNameLen = DE.getU16(&OffsetPtr, &Err);
> if (!Err)
>   FunctionName = DE.getBytes(&OffsetPtr, FunctionNameLen, &Err);
> ```
what I think is "the FunctionNameLen  is Optional type ,  when calling 
FunctionName = DE.getBytes(&OffsetPtr, FunctionNameLen, &Err);
the FunctionNameLen need to convert to uint16_t .
And I think adding a local variable Len is less cost than calling a function FunctionNameLen.getValue() "



================
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:
> > jasonliu wrote:
> > > 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);
> > > ```
> > > ?
> > I meant
> > ```
> > FunctionNameLen = DE.getU16(&OffsetPtr, &Err);
> > if (!Err)
> >   FunctionName = DE.getBytes(&OffsetPtr, FunctionNameLen, &Err);
> > ```
> what I think is "the FunctionNameLen  is Optional type ,  when calling 
> FunctionName = DE.getBytes(&OffsetPtr, FunctionNameLen, &Err);
> the FunctionNameLen need to convert to uint16_t .
> And I think adding a local variable Len is less cost than calling a function FunctionNameLen.getValue() "
> 
discuss offline , change as suggestion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81585