[PATCH] D89071: [SystemZ/z/OS] Add GOFFObjectFile class and details of GOFF file format

[Yusra Syeda via Phabricator via llvm-commits]

yusra.syeda marked 5 inline comments as done.
yusra.syeda added inline comments.


================
Comment at: llvm/include/llvm/Object/GOFFObjectFile.h:50
+public:
+  std::error_code getSymbolName(SymbolRef Symbol, StringRef &Res) const;
+
----------------
jhenderson wrote:
> Why is `getSymbolName` still returning a `std::error_code` and not an `Error`?
Seems like I missed this. It's been updated.


================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:290
+        std::errc::invalid_argument,
+        "symbolType must be SectionDef/ElemDef/LabelDef/PartRef/ExtRef.");
+
----------------
jhenderson wrote:
> What has actually been specified though? Which symbol (if known)?
These are the only options for the symbolType in the ESDSymbolType enum. If it's not one of these then the type is invalid.


================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:303
+      return createStringError(std::errc::invalid_argument,
+                               "executable must be CODE/DATA/Unspecified.");
+    switch (Executable) {
----------------
jhenderson wrote:
> What type is it actually?
> 
> Also, clang-format.
These are the only options in the ESDExecutable enum, so if it's not one of these then the type is invalid.


================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:338
+  }
+  llvm_unreachable("unable to get symbol section");
+  return section_iterator(SectionRef(Sec, this));
----------------
jhenderson wrote:
> yusra.syeda wrote:
> > jhenderson wrote:
> > > Is this really unreachable? What happens if there is no symbol section in the file?
> > Yes, this should be unreachable and is added as an extra safety check.
> You didn't answer my question: "What happens if there is no symbol section in the file?"
This should be unreachable and it should be an error if there is no symbol section. I updated the llvm_unreachable statement to return an Error instead. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89071