Don't cache internal errors generated by ExternCompiler in the repo

Summary: IPC errors communicating with external compiler processes (which generally come about when the remote process dies unexpectedly) should not be cached as fataling units in the on disk or in memory repos. They should also fail more loudly when building a production repo. Reviewed By: markw65 Differential Revision: D7530672 fbshipit-source-id: 86276e11d110dd85ab4c142c69b525d2d2a4485c
facebook · Apr 8, 2018 · bffe1af745cd0556206719b05d92f5eb54145f52 · bffe1af
1 parent 3b613d1
commit bffe1af745cd0556206719b05d92f5eb54145f52
diff --git a/hphp/compiler/package.cpp b/hphp/compiler/package.cpp
@@ -384,7 +384,8 @@ bool Package::parseImpl(const std::string* fileName) {
    if (auto uc = UnitCompiler::create(
          content.data(), content.size(), fileName->c_str(), md5)) {
      try {
-        if (auto ue = uc->compile(m_ar->getParseOnDemandCallBacks())) {
+        auto ue = uc->compile(m_ar->getParseOnDemandCallBacks());
+        if (ue && !ue->m_ICE) {
          m_ar->lock()->addHhasFile(std::move(ue));
          report(0);
          return true;
diff --git a/hphp/runtime/base/unit-cache.cpp b/hphp/runtime/base/unit-cache.cpp
@@ -398,13 +398,13 @@ CachedUnit loadUnitNonRepoAuth(StringData* requestedPath,
    }
  };

-  auto const cuptr = [&] () -> std::shared_ptr<CachedUnitWithFree> {
+  auto const cu = [&] () -> CachedUnit {
    NonRepoUnitCache::accessor rpathAcc;

    if (!cache.insert(rpathAcc, rpath)) {
      if (!isChanged(rpathAcc->second, statInfo)) {
        if (ent) ent->setStr("type", "cache_hit_writelock");
-        return rpathAcc->second.cachedUnit;
+        return rpathAcc->second.cachedUnit->cu;
      }
      if (ent) ent->setStr("type", "cache_stale");
    } else {
@@ -422,9 +422,18 @@ CachedUnit loadUnitNonRepoAuth(StringData* requestedPath,
     */

    auto const cu = createUnitFromFile(rpath, &releaseUnit, w, ent);
+
+    // Don't cache the unit if it was created in response to an internal error
+    // in ExternCompiler. Such units represent transient events.
+    if (UNLIKELY(cu.unit && cu.unit->isICE())) {
+      auto const unit = cu.unit;
+      Treadmill::enqueue([unit] { delete unit; });
+      return cu;
+    }
+
    rpathAcc->second.cachedUnit = std::make_shared<CachedUnitWithFree>(cu);
    updateStatInfo(rpathAcc);
-    return rpathAcc->second.cachedUnit;
+    return rpathAcc->second.cachedUnit->cu;
  }();

  if (path != rpath) {
@@ -433,7 +442,7 @@ CachedUnit loadUnitNonRepoAuth(StringData* requestedPath,
    updateStatInfo(pathAcc);
  }

-  return cuptr->cu;
+  return cu;
 }

 CachedUnit lookupUnitNonRepoAuth(StringData* requestedPath,
diff --git a/hphp/runtime/vm/as.cpp b/hphp/runtime/vm/as.cpp
@@ -107,20 +107,18 @@ TRACE_SET_MOD(hhas);

 namespace HPHP {

+AssemblerError::AssemblerError(int where, const std::string& what)
+  : std::runtime_error(
+      folly::sformat("Assembler Error: line {}: {}", where, what))
+{}
+
 //////////////////////////////////////////////////////////////////////

 namespace {

 struct AsmState;
 typedef void (*ParserFunc)(AsmState& as);

-struct Error : std::runtime_error {
-  explicit Error(int where, const std::string& what)
-    : std::runtime_error(folly::sformat(
-        "Assembler Error: line {}: {}", where, what))
-  {}
-};
-
 struct Input {
  explicit Input(std::istream& in)
    : m_in(in)
@@ -158,7 +156,7 @@ struct Input {
    const int currentLine = m_lineNumber;
    skipWhitespace();
    if (getc() != c) {
-      throw Error(currentLine,
+      throw AssemblerError(currentLine,
        folly::sformat("expected character `{}'", char(c)));
    }
  }
@@ -203,7 +201,7 @@ struct Input {
    if (peek() == '-') buf += (char)getc();
    consumePred(isdigit, std::back_inserter(buf));
    if (buf.empty() || buf == "-") {
-      throw Error(m_lineNumber, "expected integral value");
+      throw AssemblerError(m_lineNumber, "expected integral value");
    }
    return folly::to<int32_t>(buf);
  }
@@ -403,7 +401,7 @@ struct Input {
  };

  void error(const std::string& what) {
-    throw Error(getLineNumber(), what);
+    throw AssemblerError(getLineNumber(), what);
  }

  void io_error_if_bad() {
@@ -527,8 +525,8 @@ struct AsmState {

  template<typename... Args>
  void error(const std::string& fmt, Args&&... args) {
-    throw Error(in.getLineNumber(),
-                folly::sformat(fmt, std::forward<Args>(args)...));
+    throw AssemblerError(in.getLineNumber(),
+      folly::sformat(fmt, std::forward<Args>(args)...));
  }


@@ -850,7 +848,7 @@ void StackDepth::setBase(AsmState& as, int stackDepth) {

  // We finally know the base value. Update AsmState accordingly.
  if (*baseValue + minOffset < 0) {
-    throw Error(
+    throw AssemblerError(
      minOffsetLine,
      "opcode sequence caused stack depth to go negative"
    );
diff --git a/hphp/runtime/vm/as.h b/hphp/runtime/vm/as.h
@@ -66,6 +66,11 @@ enum class AsmResult {
  Unreachable
 };

+struct AssemblerError : std::runtime_error {
+  explicit AssemblerError(const std::string& msg) : std::runtime_error(msg) {}
+  AssemblerError(int where, const std::string& what);
+};
+
 AsmResult assemble_expression(UnitEmitter&, FuncEmitter*, int,
                              const std::string&);

diff --git a/hphp/runtime/vm/extern-compiler.cpp b/hphp/runtime/vm/extern-compiler.cpp
@@ -155,6 +155,12 @@ struct CompileException : Exception {
  {}
 };

+struct CompilerFatal : std::runtime_error {
+  explicit CompilerFatal(const std::string& str)
+    : std::runtime_error(str)
+  {}
+};
+
 [[noreturn]] void throwErrno(const char* what) {
  throw CompileException("{}: {}", what, folly::errnoStr(errno));
 }
@@ -219,7 +225,12 @@ struct ExternCompiler {
        Logger::FError("ExternCompiler Error: {}", ex.what());
      }
      throw;
-    } catch (std::runtime_error& ex) {
+    } catch (CompilerFatal& ex) {
+      if (m_options.verboseErrors) {
+        Logger::FError("ExternCompiler Fatal: {}", ex.what());
+      }
+      throw;
+    } catch (AssemblerError& ex) {
      if (m_options.verboseErrors) {
        auto const msg = folly::sformat(
          "{}\n"
@@ -234,7 +245,12 @@ struct ExternCompiler {

        // Throw the extended message to ensure the fataling unit contains the
        // additional context
-        throw std::runtime_error(msg);
+        throw AssemblerError(msg);
+      }
+      throw;
+    } catch (std::runtime_error& ex) {
+      if (m_options.verboseErrors) {
+        Logger::FError("ExternCompiler Runtime Error: {}", ex.what());
      }
      throw;
    }
@@ -284,7 +300,8 @@ struct CompilerPool {
                         int len,
                         const char* filename,
                         const MD5& md5,
-                         AsmCallbacks* callbacks);
+                         AsmCallbacks* callbacks,
+                         bool& internal_error);
  std::string getVersionString() { return m_version; }

 private:
@@ -370,7 +387,8 @@ CompilerResult CompilerPool::compile(const char* code,
                                     int len,
                                     const char* filename,
                                     const MD5& md5,
-                                     AsmCallbacks* callbacks
+                                     AsmCallbacks* callbacks,
+                                     bool& internal_error
 ) {
  CompilerGuard compiler(*this);
  std::stringstream err;
@@ -381,16 +399,25 @@ CompilerResult CompilerPool::compile(const char* code,
  );
  while (retry++ < max) {
    try {
+      internal_error = false;
      return compiler->compile(filename,
                               md5,
                               folly::StringPiece(code, len),
                               callbacks);
+    } catch (AssemblerError& ex) {
+      // Assembler rejected hhas generated by external compiler
+      return ex.what();
+    } catch (CompilerFatal& ex) {
+      // ExternCompiler returned an error when building this unit
+      return ex.what();
    } catch (CompileException& ex) {
+      internal_error = true;
      // Swallow and retry, we return infra errors in bulk once the retry limit
      // is exceeded.
      err << ex.what();
      if (retry < max) err << '\n';
    } catch (std::runtime_error& ex) {
+      internal_error = true;
      // Nontransient, don't bother with a retry.
      return ex.what();
    }
@@ -443,10 +470,10 @@ std::string ExternCompiler::readProgram() const {
  } else if (type == "error") {
    // We don't need to restart the pipe -- the compiler just wasn't able to
    // build this file...
-    throw std::runtime_error(
+    throw CompilerFatal(
      header.getDefault("error", "[no 'error' field]").asString());
  } else {
-    throw std::runtime_error("unknown message type, " + type);
+    throw CompilerFatal("unknown message type, " + type);
  }

  not_reached();
@@ -749,6 +776,19 @@ folly::Optional<CompilerOptions> hackcConfiguration() {
  };
 }

+CompilerResult hackc_compile(
+  const char* code,
+  int len,
+  const char* filename,
+  const MD5& md5,
+  AsmCallbacks* callbacks,
+  bool& internal_error
+) {
+  return s_manager.get_hackc_pool().compile(
+    code, len, filename,md5, callbacks, internal_error
+  );
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 }

@@ -855,16 +895,6 @@ void compilers_detach_after_fork() {
  }
 }

-CompilerResult hackc_compile(
-  const char* code,
-  int len,
-  const char* filename,
-  const MD5& md5,
-  AsmCallbacks* callbacks
-) {
-  return s_manager.get_hackc_pool().compile(code, len, filename,md5, callbacks);
-}
-
 std::string hackc_version() {
  return s_manager.get_hackc_pool().getVersionString();
 }
@@ -910,11 +940,13 @@ std::unique_ptr<UnitCompiler> UnitCompiler::create(const char* code,

 std::unique_ptr<UnitEmitter> HackcUnitCompiler::compile(
  AsmCallbacks* callbacks) const {
+  bool ice = false;
  auto res = hackc_compile(m_code,
                           m_codeLen,
                           m_filename,
                           m_md5,
-                           callbacks);
+                           callbacks,
+                           ice);
  std::unique_ptr<UnitEmitter> unitEmitter;
  match<void>(
    res,
@@ -932,6 +964,7 @@ std::unique_ptr<UnitEmitter> HackcUnitCompiler::compile(
    }
  );

+  if (unitEmitter) unitEmitter->m_ICE = ice;
  return unitEmitter;
 }

diff --git a/hphp/runtime/vm/extern-compiler.h b/hphp/runtime/vm/extern-compiler.h
@@ -50,11 +50,6 @@ void compilers_detach_after_fork();
 // type of error encountered
 using CompilerResult = boost::variant<std::unique_ptr<UnitEmitter>,std::string>;

-CompilerResult hackc_compile(const char* code,
-                             int len,
-                             const char* filename,
-                             const MD5& md5,
-                             AsmCallbacks* callbacks = nullptr);
 std::string hackc_version();

 struct UnitCompiler {
diff --git a/hphp/runtime/vm/repo.cpp b/hphp/runtime/vm/repo.cpp
@@ -528,7 +528,7 @@ RepoStatus Repo::insertUnit(UnitEmitter* ue, UnitOrigin unitOrigin,
 }

 void Repo::commitUnit(UnitEmitter* ue, UnitOrigin unitOrigin) {
-  if (!RuntimeOption::RepoCommit) return;
+  if (!RuntimeOption::RepoCommit || ue->m_ICE) return;

  try {
    commitMd5(unitOrigin, ue);
diff --git a/hphp/runtime/vm/unit-emitter.cpp b/hphp/runtime/vm/unit-emitter.cpp
@@ -568,6 +568,7 @@ std::unique_ptr<Unit> UnitEmitter::create(bool saveLineTable) {
  }
  u->m_typeAliases = m_typeAliases;
  u->m_metaData = m_metaData;
+  u->m_ICE = m_ICE;

  size_t ix = m_fes.size() + m_hoistablePceIdList.size();
  if (m_mergeOnly && !m_allClassesHoistable) {
diff --git a/hphp/runtime/vm/unit-emitter.h b/hphp/runtime/vm/unit-emitter.h
@@ -382,6 +382,7 @@ struct UnitEmitter {
  bool m_useStrictTypes{false};
  bool m_useStrictTypesForBuiltins{false};
  bool m_returnSeen{false};
+  bool m_ICE{false}; // internal compiler error
  int m_preloadPriority{0};
  TypedValue m_mainReturn;
  UserAttributeMap m_metaData;
diff --git a/hphp/runtime/vm/unit-inl.h b/hphp/runtime/vm/unit-inl.h
@@ -159,6 +159,10 @@ inline const StringData* Unit::dirpath() const {
  return m_dirpath;
 }

+inline bool Unit::isICE() const {
+  return m_ICE;
+}
+
 ///////////////////////////////////////////////////////////////////////////////
 // Bytecode.

diff --git a/hphp/runtime/vm/unit.cpp b/hphp/runtime/vm/unit.cpp
@@ -189,6 +189,7 @@ Unit::Unit()
  , m_isHHFile(false)
  , m_extended(false)
  , m_serialized(false)
+  , m_ICE(false)
  , m_mainReturn(make_tv<KindOfUninit>())
 {}

diff --git a/hphp/runtime/vm/unit.h b/hphp/runtime/vm/unit.h
@@ -351,6 +351,11 @@ struct Unit {
  const StringData* filepath() const;
  const StringData* dirpath() const;

+  /*
+   * Was this unit created in response to an internal compiler error?
+   */
+  bool isICE() const;
+
  /////////////////////////////////////////////////////////////////////////////
  // Bytecode.                                                          [const]

@@ -889,6 +894,7 @@ struct Unit {
  bool m_useStrictTypesForBuiltins : 1;
  bool m_extended : 1;
  bool m_serialized : 1;
+  bool m_ICE : 1; // was this unit the result of an internal compiler error
  LowStringPtr m_dirpath{nullptr};

  TypedValue m_mainReturn;