[PATCH] D50169: [XRay] Improve error reporting when loading traces

Keith via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Aug 6 01:20:08 PDT 2018


kpw accepted this revision.
kpw added a comment.
This revision is now accepted and ready to land.

Consider creating wrapper functions around DataExtractor's getU64, getU32, getU16, and getU8 that take a DataExtractor&, an Offset&, a descriptive twine, and return an Expected<T>. I'm trying to come up with ideas to keep all the granular checks as you desire, but pull some of the ceremonial noise out of the flow of the main logic.

the use would be something like:

  auto result = expectU16(RecordExtractor, OffsetPtr, "cpu id");
  if (auto err = result.takeError()) return err;
  State.CPUId = *result;

The implementation (you'd have to duplicate functions for a few different types) would be something like

  Expected<uint16_t> expectU16(DataExtractor &RecordExtractor, uint32_t &OffsetPtr, Twine FieldDescriptor) {
    auto PreReadOffset = OffsetPtr;
    auto result = RecordExtractor.getU16(&OffsetPtr);
    if (OffsetPtr == PreReadOffset)
      return createStringError(
          std::make_error_code(std::errc::executable_format_error),
          "Failed reading %s at offset %d.", FieldDescriptor, OffsetPtr);
    return result;
  }

I'm not sure this would amount to much less code, but it would move boilerplate out of "the way" and I claim that readers (or future you or I) would be able to follow the core state transitions more easily.



================
Comment at: llvm/lib/XRay/Trace.cpp:121-124
+    if (!Reader.isValidOffsetForDataOfSize(OffsetPtr, 32))
+      return createStringError(
+          std::make_error_code(std::errc::executable_format_error),
+          "Not enough bytes to read a full record at offset %d.", OffsetPtr);
----------------
This checks makes the other PreReadOffset error checks in this loop defensive coding at best, but for the moment dead code. They aren't hurting anything by being there, and are a defense against future changes, so that's OK if you really want.


https://reviews.llvm.org/D50169





More information about the llvm-commits mailing list