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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Object/GOFFObjectFile.h:50
+public:
+  Error getSymbolName(SymbolRef Symbol, StringRef &Res) const;
+
----------------
Sorry, I should have spotted this earlier. It would be a more common style for this function's signature to return an `Expected` rather than take the `Res` as an argument:

`Expected<StringRef> getSymbolName(SymbolRef Symbol) const;`

This would match, e.g. `getSectionName` or `getSectionContents` as good examples.


================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:46
+  if ((Object.getBufferSize() % GOFF::RecordLength) != 0) {
+    Err = createStringError(object_error::unexpected_eof,
+                            "object file is not the right size. Must be a multiple "
----------------
Please remember to clang-format your changes.


================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:48
+                            "object file is not the right size. Must be a multiple "
+                            "of 80 bytes, but is %d bytes",
+                            Object.getBufferSize());
----------------
`getBufferSize()` returns `size_t` not `int`. The correct format specifier is `%z` for `size_t`.


================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:201
+    Res = *NameOrErr;
+    return errorCodeToError(std::error_code());
+  }
----------------
Here, you'd want `Error::success()`, but actually, if you switch to using `Expected`, you'd return `*NameOrErr` directly.


================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:343
+  return createStringError(std::errc::invalid_argument,
+                           "unable to get symbol section");
+}
----------------
I think it would be slightly clearly to say `no symbol section found`. "unable to get" sounds like there was an actual problem retrieving the section (e.g. some part of the section data was invalid), whereas "no ... found" is clear that it's simply not there.


================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:290
+        std::errc::invalid_argument,
+        "symbolType must be SectionDef/ElemDef/LabelDef/PartRef/ExtRef.");
+
----------------
yusra.syeda wrote:
> 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.
I don't think you understood what I meant - when I asked this and the similar question below regarding executable type, I meant you should include the additional context, i.e. which symbol had an invalid type (i.e. the index or possibly name) and what that invalid type was, e.g. "symbol 42 has unknown type 0x12". It's important to do this properly because the user's input object might be corrupted in some way, and the code needs to make it easier for that user to find the problem.


================
Comment at: llvm/lib/Object/GOFFObjectFile.cpp:303
+      return createStringError(std::errc::invalid_argument,
+                               "executable must be CODE/DATA/Unspecified.");
+    switch (Executable) {
----------------
yusra.syeda wrote:
> 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.
Same as above - give more useful context in this message, e.g. "executable has unknown type 0x1111".

Also, I think it's more common to omit the `std::` prefix from `std::errc` values (LLVM has its own version of this set, which partly parallels the std one). Please take a look at removing that prefix from all these `std::errc` instances.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89071