[PATCH] D103490: [SystemZ][z/OS] Add support for TXT records in the GOFF reader

Wed Jun 9 01:13:59 PDT 2021

jhenderson added a comment.

In D103490#2806504 <https://reviews.llvm.org/D103490#2806504>, @yusra.syeda wrote:

> ping :)

Sorry, I've been away, and this is pretty low on my review priority list, so I haven't had a chance to look at this until now.

================
Comment at: llvm/include/llvm/Object/GOFF.h:95
+public:
+  /// \brief Maximum length of data; any more must go in continuation.
+  static const uint8_t TXTMaxDataLength = 56;
----------------

================
Comment at: llvm/include/llvm/Object/GOFF.h:103
+
+  static void getElementEsdId(const uint8_t *Record, uint32_t &EsdId) {
+    get<uint32_t>(Record, 4, EsdId);
----------------
What's the motivation for returning by updating an input parameter, rather than returning via the return type?

================
Comment at: llvm/include/llvm/Object/GOFFObjectFile.h:33
 class GOFFObjectFile : public ObjectFile {
+  friend class GOFFSymbolRef;
+
----------------
There may be a perfectly valid reason for this, but why make this a friend class rather than just making the relevant functions public?

================
Comment at: llvm/include/llvm/Object/GOFFObjectFile.h:36
   IndexedMap<const uint8_t *> EsdPtrs; // Indexed by EsdId.
+  SmallVector<const uint8_t *, 256> TextPtrs;

----------------
Any particular reason you've chosen the size 256 here and in various other places? It may deserve a comment or some sort of named constant.

================
Comment at: llvm/include/llvm/Object/GOFFObjectFile.h:70-71

+  bool isSectionNoLoad(DataRefImpl Sec) const;
+  bool isSectionReadOnlyData(DataRefImpl Sec) const;
+  bool isSectionZeroInit(DataRefImpl Sec) const;
----------------
What's the motivation for adding these two functions at this point? It seems like a bad idea to expand the interface without a use-case (and if there is a use-case, what is it)?

================
Comment at: llvm/include/llvm/Object/GOFFObjectFile.h:72
+  bool isSectionReadOnlyData(DataRefImpl Sec) const;
+  bool isSectionZeroInit(DataRefImpl Sec) const;
+
----------------
By name, this function sounds awfully like `isSectionBSS` (which for ELF at least means a section that is allocated space and zero-initialised at load time).

================
Comment at: llvm/include/llvm/Object/GOFFObjectFile.h:130
+public:
+  GOFFSymbolRef(const SymbolRef &B) : SymbolRef(B) {
+    assert(isa<GOFFObjectFile>(SymbolRef::getObject()));
----------------
`*Ref` types are designed to be copied. Don't pass them as `const &`.

================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:432
+    LLVM_DEBUG(dbgs() << "Got section: " << Res << '\n');
+    // TODO: Make clang look for C____clangast instead of this
+    if (Res.equals("C___clangast"))
----------------
This TODO sounds a bit odd. What does the clang compiler have to do with this code?

================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:478
+  ESDRecord::getFillBytePresent(EdEsdRecord, FillBytePresent);
+  uint8_t FillByte = '\0';
+  if (FillBytePresent)
----------------
Any particular reason you've used `'\0'` rather than simply `0`? After all, you're storing it in a `uint8_t` (a number) rather than a `char`/`unsigned char`.

================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:519-520
+
+  StringRef Res = StringRef(Data);
+  return arrayRefFromStringRef(Res);
+}
----------------
Why not simply `return arrayRefFromStringRef(StringRef(Data));`? In fact, could you get away with returning simply `Data.data()` or similar?

================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:527
+  ESDRecord::getAlignment(EsdRecord, Pow2Alignment);
+  uint64_t ByteAlignment = 1 << (uint64_t)Pow2Alignment;
+  return ByteAlignment;
----------------
I'm pretty sure the right hand side of this function will return an `int` because that's the type of the literal `1` in this context. I think you want to do `UINT64_C(1)` to force the type of the literal (and therefore the result of the left-shift operation) to 64-bit.

================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:539-540
+  ESDRecord::getExecutable(EsdRecord, Executable);
+  bool IsSectionText = (Executable == GOFF::ESD_EXE_CODE);
+  return IsSectionText;
+}
----------------
Don't add unnecessary local variable. Same below for the other functions.

================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:564-573
+  bool Result;
+  if (isSectionData(Sec)) {
+    const uint8_t *EsdRecord = getSectionEdEsdRecord(Sec);
+    GOFF::ESDLoadingBehavior LoadingBehavior;
+    ESDRecord::getLoadingBehavior(EsdRecord, LoadingBehavior);
+    Result = LoadingBehavior == GOFF::ESD_LB_Initial;
+  } else {
----------------

================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:576-580
+bool GOFFObjectFile::isSectionZeroInit(DataRefImpl Sec) const {
+  // GOFF uses fill characters and fill characters are applied
+  // on getSectionContents() - so we say false to zero init.
+  return false;
+}
----------------
Why are you adding this function?

================
Comment at: llvm/unittests/Object/GOFFObjectFileTest.cpp:507
+TEST(GOFFObjectFileTest, TXTConstruct) {
+  char GOFFData[GOFF::RecordLength * 6] = {0x00};
+
----------------
You can omit the explicit 0 from here. This will zero-initialise the whole array.

================
Comment at: llvm/unittests/Object/GOFFObjectFileTest.cpp:587
+
+
+  for (SymbolRef Symbol : GOFFObj->symbols()) {
----------------
Get rid of double blank line.

================
Comment at: llvm/unittests/Object/GOFFObjectFileTest.cpp:595
+
+  for (const SectionRef &Section : GOFFObj->sections()) {
+    Expected<StringRef> SectionContent = Section.getContents();
----------------

================
Comment at: llvm/unittests/Object/GOFFObjectFileTest.cpp:600-603
+    const char *Str = "\x12\x34\x56\x78\x9a\xbc\xde\xf0";
+    StringRef Expected(Str);
+
+    ASSERT_EQ(Expected, Contents);
----------------
You should be able to just do `EXPECT_EQ(Contents, "\x12\x34\x56\x78\x9a\xbc\xde\xf0");`

Use `EXPECT_EQ` because one section's content not being right shouldn't impact other sections, same as with the SymbolName check above.

Additionally, you need checks that `symbols()` and `sections()` return the right number of symbols and sections somewhere. Your test would pass as things stand, if `symbols()` or `sections()` returned an empty list (and therefore no looping occurred).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103490