[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
llvm-commits at lists.llvm.org
Wed Jul 8 14:11:11 PDT 2020
DiggerLin marked 48 inline comments as done.
DiggerLin 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;
----------------
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);
================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:334
+ // Byte 6
+ static constexpr uint32_t HasVecInfoMask = 0x0080'0000;
+ static constexpr uint32_t Spare4Mask = 0x0040'0000;
----------------
jasonliu wrote:
> I find the 6th byte here is a bit different than what we have in the OS headers:
> ```
> /* Byte 6 */
> unsigned longtbtable:1; /* Set if xtbtable extension exists. */
> unsigned has_vec:1; /* Set if optional vector info is present */
> unsigned gpr_saved:6; /* Number of GPRs saved, max of 32 */
> ```
yes, the have two other documents both are different with the /usr/include/sys/debug.h (aix OS) . I keep the patch same as /usr/include/sys/debug.h now.
================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:396
+ const uint8_t *TBPtr;
+ const uint64_t Size;
+ Optional<std::string> ParaType;
----------------
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.
================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:400
+ Optional<uint32_t> HandMask;
+ Optional<uint32_t> CtlInfo;
+ Optional<uint16_t> FunctionNameLen;
----------------
jasonliu wrote:
> TODO: no number of CTL anchors? or Displacement into stack of each anchor?
since most of function do not have controlled storage , decoding of controlled storage will put on another patch.
================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:846
+
+static std::string parseParaType(uint32_t Value, unsigned int ParaNum) {
+ std::string ParaType;
----------------
jhenderson wrote:
> Any particular reason you're using `unsigned int` here instead of just `unsigned` like you do below? Should it actually be a `size_t` in both cases?
we know the ParaNum is less than 512 , I do not think we need size_t for it.
================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:850
+ if (I != 0)
+ ParaType += ", ";
+ if ((Value & TracebackTable::FixedParaTypeBit) == 0) {
----------------
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.
================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:885
+ DataExtractor DE(ArrayRef<uint8_t>(Ptr, S), false, 4);
+ uint64_t offset_ptr = 0;
+
----------------
jhenderson wrote:
> Please use LLVM style for variable names.
thanks
================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:902
+ CtlInfo = DE.getU32(&offset_ptr, &Err);
+
+ if (!Err && isFuncNamePresent()) {
----------------
jasonliu wrote:
> Are we missing something between CtlInfo and FunctionNameLen?
>
> ```
> * 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.
> ```
> i.e. the ctl_info_disp?
controlled storage info related will be put into another patch.
================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:904
+ if (!Err && isFuncNamePresent()) {
+ uint16_t Len = DE.getU16(&offset_ptr, &Err);
+ if (!Err)
----------------
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.
================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:916
+ (support::endian::read32be(TBPtr + P) & TracebackTable::X)
+#define GETBITWITHMASKSHIFT(P, X, S) \
+ (support::endian::read32be(TBPtr + P) & TracebackTable::X) >> \
----------------
jasonliu wrote:
> Macros are missing undefs.
>
thanks
================
Comment at: llvm/unittests/Object/XCOFFObjectFileTest.cpp:85
+ XCOFFTracebackTable::create(V1, sizeof(V1));
+ ASSERT_TRUE(!!TTOrErr1) << "Parse error";
+ XCOFFTracebackTable TT1 = TTOrErr1.get();
----------------
jhenderson wrote:
> Here and in the equivalent cases elsewhere, use `ASSERT_THAT_EXPECTED(TTOrErr1, Succeeded());`
thanks
================
Comment at: llvm/unittests/Object/XCOFFObjectFileTest.cpp:86
+ ASSERT_TRUE(!!TTOrErr1) << "Parse error";
+ XCOFFTracebackTable TT1 = TTOrErr1.get();
+ EXPECT_EQ(TT1.getVersion(), 1);
----------------
jhenderson wrote:
> `XCOFFTracebackTable TT1 = *TTOrErr1;` is more traditional usage.
thanks
================
Comment at: llvm/unittests/Object/XCOFFObjectFileTest.cpp:95
+
+ EXPECT_TRUE(TT1.getParaType());
+ EXPECT_STREQ(TT1.getParaType().getValue().data(), "i, i, f, f, d");
----------------
jhenderson wrote:
> `ASSERT_TRUE` or the next check will crash if it ever fails.
thanks
================
Comment at: llvm/unittests/Object/XCOFFObjectFileTest.cpp:96
+ EXPECT_TRUE(TT1.getParaType());
+ EXPECT_STREQ(TT1.getParaType().getValue().data(), "i, i, f, f, d");
+
----------------
jhenderson wrote:
> Does `EXPECT_EQ(TT1.getParaType().getValue(), "i, i, f, f, d");` not work?
according to https://github.com/google/googletest/blob/master/googletest/docs/primer.md
EXPECT_EQ(val1, val2); compare two values. val1 == val2
EXPECT_STREQ(str1,str2); compare two C strings the two C strings have the same content
================
Comment at: llvm/unittests/Object/XCOFFObjectFileTest.cpp:101-103
+ ASSERT_TRUE(!!TTOrErr2) << "Parse error";
+ XCOFFTracebackTable TT2 = TTOrErr2.get();
+ EXPECT_STREQ(TT2.getParaType().getValue().data(), "f, f, d, i, i");
----------------
jhenderson wrote:
> For this and the ones below, same comments as above, but you also need an `ASSERT_TRUE(TT2.getParaType())` to avoid a crash in case the `Optional` is empty.
thanks
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81585/new/
https://reviews.llvm.org/D81585
More information about the llvm-commits
mailing list