]> git.ipfire.org Git - thirdparty/ccache.git/commitdiff
Remove the unify mode
authorJoel Rosdahl <joel@rosdahl.net>
Sun, 29 Dec 2019 18:21:39 +0000 (19:21 +0100)
committerJoel Rosdahl <joel@rosdahl.net>
Sun, 5 Jan 2020 19:14:28 +0000 (20:14 +0100)
The unify mode has not received enough attention and has at least these
bugs:

1. The direct mode doesn’t work if the unify mode is enabled. This is
   because the unify mode doesn’t call into process_preprocessed_file
   which stores the paths and hashes of included files needed by the
   direct mode.
2. The .incbin directive detection has no effect when using the unify
   mode. This is again because the unify mode doesn’t use
   process_preprocessed_file which is where the .incbin detection takes
   place.
3. The unifier’s tokenizer doesn’t understand C++11 raw string literals.
4. The unifier ignores comments, but comments may have semantic meaning
   to modern compilers, e.g. “fall through” comments.

Bugs 3 and 4 are fixable by improving the unifier’s tokenization
algorithm, but since it’s a bit tricky it likely won’t be worth the
effort, especially not as a bug fix.

Bugs 1 and 2 are also fixable by unifying the two code paths, but that’s
a non-trivial effort.

In addition to the bugs, I believe that the usefullness of the unify
mode is low:

* It’s only applicable when not using -g.
* It won't be enabled for C++ unless somebody fixes bug 3.
* It can make line numbers in warning messages and __LINE__ expansions
  incorrect.
* Since comments should not be ignored, the unify mode can only make a
  difference for some types of whitespace changes, like adding or
  removing blank lines or changing a+b to a + b. (a + b is already
  normalized to a + b by the preprocessor.)

Therefore I’ll just remove the unify mode to fix the bugs.

Fixes #497.

(cherry picked from commit 947a72ce3712a6901e7ddc43a50c5df40739113e)

Makefile.in
dev.mk.in
doc/MANUAL.adoc
src/Config.cpp
src/Config.hpp
src/ccache.cpp
src/unify.cpp [deleted file]
src/unify.hpp [deleted file]
test/suites/base.bash
unittest/test_Config.cpp

index 53181a657a7ed0d9d0eb69bc0606d52296a38831..149ea1a3baeefee7195f69be24cc33feff2d0edb 100644 (file)
@@ -61,8 +61,7 @@ non_third_party_sources = \
     src/lockfile.cpp \
     src/manifest.cpp \
     src/result.cpp \
-    src/stats.cpp \
-    src/unify.cpp
+    src/stats.cpp
 generated_sources = \
     src/version.cpp
 third_party_sources = \
index d7323587b86e1bcccf3b65eaad7f4951a83e1c42..1bd2dcd4296b537be4a0e3a489a31ca63288343a 100644 (file)
--- a/dev.mk.in
+++ b/dev.mk.in
@@ -68,7 +68,6 @@ non_third_party_headers = \
     src/manifest.hpp \
     src/result.hpp \
     src/system.hpp \
-    src/unify.hpp \
     unittest/framework.hpp \
     unittest/util.hpp
 third_party_headers = \
index e4af9982dd6d36b3fa5b58f60dc8fd4435ac8786..b9820e0f140d96b578e8fc01ac27c3030eed76ee 100644 (file)
@@ -704,18 +704,6 @@ NOTE: In previous versions of ccache, *CCACHE_TEMPDIR* had to be on the same
     with other users. Note that this also affects the file permissions set on
     the object files created from your compilations.
 
-[[config_unify]] *unify* (*CCACHE_UNIFY* or *CCACHE_NOUNIFY*, see <<_boolean_values,Boolean values>> above)::
-
-    If true, ccache will use a C/C++ unifier when hashing the preprocessor
-    output if the *-g* option is not used. The unifier is slower than a normal
-    hash, so setting this environment variable loses a little bit of speed, but
-    it means that ccache can take advantage of not recompiling when the changes
-    to the source code consist of reformatting only. Note that enabling the
-    unifier changes the hash, so cached compilations produced when the unifier
-    is enabled cannot be reused when the unifier is disabled, and vice versa.
-    Enabling the unifier may result in incorrect line number information in
-    compiler warning messages and expansions of the `__LINE__` macro.
-
 
 Cache size management
 ---------------------
@@ -1073,7 +1061,6 @@ The depend mode will be disabled if any of the following holds:
 
 * the configuration setting <<config_depend_mode,*depend_mode*>> is false
 * the configuration setting <<config_run_second_cpp,*run_second_cpp*>> is false
-* the configuration setting <<config_unify,*unify*>> is true
 * the compiler is not generating dependencies using *-MD* or *-MMD*
 
 
index 1aba8916f8c1a412fe529a529441683353139630..c12b36e0d845b0f8c7c357adf47a10deceee480c 100644 (file)
@@ -1,4 +1,4 @@
-// Copyright (C) 2019 Joel Rosdahl and other contributors
+// Copyright (C) 2019-2020 Joel Rosdahl and other contributors
 //
 // See doc/AUTHORS.adoc for a complete list of contributors.
 //
@@ -69,7 +69,6 @@ enum class ConfigItem {
   stats,
   temporary_dir,
   umask,
-  unify,
 };
 
 const std::unordered_map<std::string, ConfigItem> k_config_key_table = {
@@ -107,7 +106,6 @@ const std::unordered_map<std::string, ConfigItem> k_config_key_table = {
   {"stats", ConfigItem::stats},
   {"temporary_dir", ConfigItem::temporary_dir},
   {"umask", ConfigItem::umask},
-  {"unify", ConfigItem::unify},
 };
 
 const std::unordered_map<std::string, std::string> k_env_variable_table = {
@@ -146,7 +144,6 @@ const std::unordered_map<std::string, std::string> k_env_variable_table = {
   {"STATS", "stats"},
   {"TEMPDIR", "temporary_dir"},
   {"UMASK", "umask"},
-  {"UNIFY", "unify"},
 };
 
 typedef std::function<void(
@@ -577,9 +574,6 @@ Config::get_string_value(const std::string& key) const
 
   case ConfigItem::umask:
     return format_umask(m_umask);
-
-  case ConfigItem::unify:
-    return format_bool(m_unify);
   }
 
   assert(false);
@@ -798,10 +792,6 @@ Config::set_item(const std::string& key,
   case ConfigItem::umask:
     m_umask = parse_umask(value);
     break;
-
-  case ConfigItem::unify:
-    m_unify = parse_bool(value, from_env_variable, negate);
-    break;
   }
 
   m_origins.emplace(key, origin);
index bf8dbfaa48c86f8965c3b3db4356baa394326b27..5f2c4c01637a9022d31198ec9a5a44721d488d2c 100644 (file)
@@ -1,4 +1,4 @@
-// Copyright (C) 2019 Joel Rosdahl and other contributors
+// Copyright (C) 2019-2020 Joel Rosdahl and other contributors
 //
 // See doc/AUTHORS.adoc for a complete list of contributors.
 //
@@ -69,7 +69,6 @@ public:
   bool stats() const;
   const std::string& temporary_dir() const;
   uint32_t umask() const;
-  bool unify() const;
 
   void set_base_dir(const std::string& value);
   void set_cache_dir(const std::string& value);
@@ -80,7 +79,6 @@ public:
   void set_max_files(uint32_t value);
   void set_max_size(uint64_t value);
   void set_run_second_cpp(bool value);
-  void set_unify(bool value);
 
   typedef std::function<void(const std::string& key,
                              const std::string& value,
@@ -142,7 +140,6 @@ private:
   bool m_stats = true;
   std::string m_temporary_dir = "";
   uint32_t m_umask = std::numeric_limits<uint32_t>::max(); // Don't set umask
-  bool m_unify = false;
 
   std::unordered_map<std::string /*key*/, std::string /*origin*/> m_origins;
 
@@ -357,12 +354,6 @@ Config::umask() const
   return m_umask;
 }
 
-inline bool
-Config::unify() const
-{
-  return m_unify;
-}
-
 inline void
 Config::set_base_dir(const std::string& value)
 {
@@ -416,9 +407,3 @@ Config::set_run_second_cpp(bool value)
 {
   m_run_second_cpp = value;
 }
-
-inline void
-Config::set_unify(bool value)
-{
-  m_unify = value;
-}
index 96c3972d3e45f9c21d26930f1d861a8644872c8c..036bc6b2be327fce321e9c9e0069ba1860aa2bf7 100644 (file)
@@ -1,5 +1,5 @@
 // Copyright (C) 2002-2007 Andrew Tridgell
-// Copyright (C) 2009-2019 Joel Rosdahl and other contributors
+// Copyright (C) 2009-2020 Joel Rosdahl and other contributors
 //
 // See doc/AUTHORS.adoc for a complete list of contributors.
 //
@@ -31,7 +31,6 @@
 #include "language.hpp"
 #include "manifest.hpp"
 #include "result.hpp"
-#include "unify.hpp"
 
 #include "third_party/fmt/core.h"
 
@@ -1601,27 +1600,11 @@ get_result_name_from_cpp(struct args* args, struct hash* hash)
     failed();
   }
 
-  if (g_config.unify()) {
-    // When we are doing the unifying tricks we need to include the input file
-    // name in the hash to get the warnings right.
-    hash_delimiter(hash, "unifyfilename");
-    hash_string(hash, input_file);
-
-    hash_delimiter(hash, "unifycpp");
-
-    bool debug_unify = getenv("CCACHE_DEBUG_UNIFY");
-    if (unify_hash(hash, path_stdout, debug_unify) != 0) {
-      stats_update(STATS_ERROR);
-      cc_log("Failed to unify %s", path_stdout);
-      failed();
-    }
-  } else {
-    hash_delimiter(hash, "cpp");
-    if (!process_preprocessed_file(
-          hash, path_stdout, guessed_compiler == GUESSED_PUMP)) {
-      stats_update(STATS_ERROR);
-      failed();
-    }
+  hash_delimiter(hash, "cpp");
+  if (!process_preprocessed_file(
+        hash, path_stdout, guessed_compiler == GUESSED_PUMP)) {
+    stats_update(STATS_ERROR);
+    failed();
   }
 
   hash_delimiter(hash, "cppstderr");
@@ -3181,11 +3164,6 @@ cc_process_args(struct args* args,
     }
   } // for
 
-  if (generating_debuginfo && g_config.unify()) {
-    cc_log("Generating debug info; disabling unify mode");
-    g_config.set_unify(false);
-  }
-
   if (generating_debuginfo_level_3 && !g_config.run_second_cpp()) {
     cc_log("Generating debug info level 3; not compiling preprocessed code");
     g_config.set_run_second_cpp(true);
@@ -3851,7 +3829,7 @@ ccache(int argc, char* argv[])
 
   if (g_config.depend_mode()
       && (!generating_dependencies || str_eq(output_dep, "/dev/null")
-          || !g_config.run_second_cpp() || g_config.unify())) {
+          || !g_config.run_second_cpp())) {
     cc_log("Disabling depend mode");
     g_config.set_depend_mode(false);
   }
diff --git a/src/unify.cpp b/src/unify.cpp
deleted file mode 100644 (file)
index 7a7744e..0000000
+++ /dev/null
@@ -1,263 +0,0 @@
-// Copyright (C) 2002 Andrew Tridgell
-// Copyright (C) 2009-2019 Joel Rosdahl and other contributors
-//
-// See doc/AUTHORS.adoc for a complete list of contributors.
-//
-// This program is free software; you can redistribute it and/or modify it
-// under the terms of the GNU General Public License as published by the Free
-// Software Foundation; either version 3 of the License, or (at your option)
-// any later version.
-//
-// This program is distributed in the hope that it will be useful, but WITHOUT
-// ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
-// FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
-// more details.
-//
-// You should have received a copy of the GNU General Public License along with
-// this program; if not, write to the Free Software Foundation, Inc., 51
-// Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
-
-// C/C++ unifier
-//
-// The idea is that changes that don't affect the resulting C code should not
-// change the hash. This is achieved by folding white-space and other
-// non-semantic fluff in the input into a single unified format.
-//
-// This unifier was design to match the output of the unifier in compilercache,
-// which is flex based. The major difference is that this unifier is much
-// faster (about 2x) and more forgiving of syntactic errors. Continuing on
-// syntactic errors is important to cope with C/C++ extensions in the local
-// compiler (for example, inline assembly systems).
-
-#include "unify.hpp"
-
-#include "ccache.hpp"
-#include "hash.hpp"
-
-static bool print_unified = true;
-
-static const char* const s_tokens[] = {
-  "...", ">>=", "<<=", "+=", "-=", "*=", "/=", "%=", "&=", "^=", "|=",
-  ">>",  "<<",  "++",  "--", "->", "&&", "||", "<=", ">=", "==", "!=",
-  ";",   "{",   "<%",  "}",  "%>", ",",  ":",  "=",  "(",  ")",  "[",
-  "<:",  "]",   ":>",  ".",  "&",  "!",  "~",  "-",  "+",  "*",  "/",
-  "%",   "<",   ">",   "^",  "|",  "?",  0};
-
-#define C_ALPHA 1
-#define C_SPACE 2
-#define C_TOKEN 4
-#define C_QUOTE 8
-#define C_DIGIT 16
-#define C_HEX 32
-#define C_FLOAT 64
-#define C_SIGN 128
-
-static struct
-{
-  unsigned char type;
-  unsigned char num_toks;
-  const char* toks[7];
-} tokens[256];
-
-// Build up the table used by the unifier.
-static void
-build_table(void)
-{
-  static bool done;
-  if (done) {
-    return;
-  }
-  done = true;
-
-  memset(tokens, 0, sizeof(tokens));
-  for (unsigned char c = 0; c < 128; c++) {
-    if (isalpha(c) || c == '_') {
-      tokens[c].type |= C_ALPHA;
-    }
-    if (isdigit(c)) {
-      tokens[c].type |= C_DIGIT;
-    }
-    if (isspace(c)) {
-      tokens[c].type |= C_SPACE;
-    }
-    if (isxdigit(c)) {
-      tokens[c].type |= C_HEX;
-    }
-  }
-  tokens[static_cast<unsigned char>('\'')].type |= C_QUOTE;
-  tokens[static_cast<unsigned char>('"')].type |= C_QUOTE;
-  tokens[static_cast<unsigned char>('l')].type |= C_FLOAT;
-  tokens[static_cast<unsigned char>('L')].type |= C_FLOAT;
-  tokens[static_cast<unsigned char>('f')].type |= C_FLOAT;
-  tokens[static_cast<unsigned char>('F')].type |= C_FLOAT;
-  tokens[static_cast<unsigned char>('U')].type |= C_FLOAT;
-  tokens[static_cast<unsigned char>('u')].type |= C_FLOAT;
-
-  tokens[static_cast<unsigned char>('-')].type |= C_SIGN;
-  tokens[static_cast<unsigned char>('+')].type |= C_SIGN;
-
-  for (int i = 0; s_tokens[i]; i++) {
-    unsigned char c = s_tokens[i][0];
-    tokens[c].type |= C_TOKEN;
-    tokens[c].toks[tokens[c].num_toks] = s_tokens[i];
-    tokens[c].num_toks++;
-  }
-}
-
-// Buffer up characters before hashing them.
-static void
-pushchar(struct hash* hash, unsigned char c)
-{
-  static unsigned char buf[64];
-  static size_t len;
-
-  if (c == 0) {
-    if (len > 0) {
-      hash_buffer(hash, (char*)buf, len);
-      if (print_unified) {
-        printf("%.*s", (int)len, buf);
-      }
-      len = 0;
-    }
-    return;
-  }
-
-  buf[len++] = c;
-  if (len == 64) {
-    hash_buffer(hash, (char*)buf, len);
-    if (print_unified) {
-      printf("%.*s", (int)len, buf);
-    }
-    len = 0;
-  }
-}
-
-// Hash some C/C++ code after unifying.
-static void
-unify(struct hash* hash, unsigned char* p, size_t size)
-{
-  build_table();
-
-  for (size_t ofs = 0; ofs < size;) {
-    if (p[ofs] == '#') {
-      if ((size - ofs) > 2 && p[ofs + 1] == ' ' && isdigit(p[ofs + 2])) {
-        do {
-          ofs++;
-        } while (ofs < size && p[ofs] != '\n');
-        ofs++;
-      } else {
-        do {
-          pushchar(hash, p[ofs]);
-          ofs++;
-        } while (ofs < size && p[ofs] != '\n');
-        pushchar(hash, '\n');
-        ofs++;
-      }
-      continue;
-    }
-
-    if (tokens[p[ofs]].type & C_ALPHA) {
-      do {
-        pushchar(hash, p[ofs]);
-        ofs++;
-      } while (ofs < size && (tokens[p[ofs]].type & (C_ALPHA | C_DIGIT)));
-      pushchar(hash, '\n');
-      continue;
-    }
-
-    if (tokens[p[ofs]].type & C_DIGIT) {
-      do {
-        pushchar(hash, p[ofs]);
-        ofs++;
-      } while (ofs < size
-               && ((tokens[p[ofs]].type & C_DIGIT) || p[ofs] == '.'));
-      if (ofs < size && (p[ofs] == 'x' || p[ofs] == 'X')) {
-        do {
-          pushchar(hash, p[ofs]);
-          ofs++;
-        } while (ofs < size && (tokens[p[ofs]].type & C_HEX));
-      }
-      if (ofs < size && (p[ofs] == 'E' || p[ofs] == 'e')) {
-        pushchar(hash, p[ofs]);
-        ofs++;
-        while (ofs < size && (tokens[p[ofs]].type & (C_DIGIT | C_SIGN))) {
-          pushchar(hash, p[ofs]);
-          ofs++;
-        }
-      }
-      while (ofs < size && (tokens[p[ofs]].type & C_FLOAT)) {
-        pushchar(hash, p[ofs]);
-        ofs++;
-      }
-      pushchar(hash, '\n');
-      continue;
-    }
-
-    if (tokens[p[ofs]].type & C_SPACE) {
-      do {
-        ofs++;
-      } while (ofs < size && (tokens[p[ofs]].type & C_SPACE));
-      continue;
-    }
-
-    if (tokens[p[ofs]].type & C_QUOTE) {
-      unsigned char q = p[ofs];
-      pushchar(hash, p[ofs]);
-      do {
-        ofs++;
-        while (ofs < size - 1 && p[ofs] == '\\') {
-          pushchar(hash, p[ofs]);
-          pushchar(hash, p[ofs + 1]);
-          ofs += 2;
-        }
-        pushchar(hash, p[ofs]);
-      } while (ofs < size && p[ofs] != q);
-      pushchar(hash, '\n');
-      ofs++;
-      continue;
-    }
-
-    if (tokens[p[ofs]].type & C_TOKEN) {
-      unsigned char q = p[ofs];
-      int i;
-      for (i = 0; i < tokens[q].num_toks; i++) {
-        const unsigned char* s = (const unsigned char*)tokens[q].toks[i];
-        int len = strlen((const char*)s);
-        if (size >= ofs + len && memcmp(&p[ofs], s, len) == 0) {
-          int j;
-          for (j = 0; s[j]; j++) {
-            pushchar(hash, s[j]);
-            ofs++;
-          }
-          pushchar(hash, '\n');
-          break;
-        }
-      }
-      if (i < tokens[q].num_toks) {
-        continue;
-      }
-    }
-
-    pushchar(hash, p[ofs]);
-    pushchar(hash, '\n');
-    ofs++;
-  }
-  pushchar(hash, 0);
-}
-
-// Hash a file that consists of preprocessor output, but remove any line number
-// information from the hash.
-int
-unify_hash(struct hash* hash, const char* fname, bool debug)
-{
-  char* data;
-  size_t size;
-  if (!read_file(fname, 0, &data, &size)) {
-    stats_update(STATS_PREPROCESSOR);
-    return -1;
-  }
-  print_unified = debug;
-  unify(hash, (unsigned char*)data, size);
-  free(data);
-  return 0;
-}
diff --git a/src/unify.hpp b/src/unify.hpp
deleted file mode 100644 (file)
index 5decc0a..0000000
+++ /dev/null
@@ -1,25 +0,0 @@
-// Copyright (C) 2018-2019 Joel Rosdahl and other contributors
-//
-// See doc/AUTHORS.adoc for a complete list of contributors.
-//
-// This program is free software; you can redistribute it and/or modify it
-// under the terms of the GNU General Public License as published by the Free
-// Software Foundation; either version 3 of the License, or (at your option)
-// any later version.
-//
-// This program is distributed in the hope that it will be useful, but WITHOUT
-// ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
-// FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
-// more details.
-//
-// You should have received a copy of the GNU General Public License along with
-// this program; if not, write to the Free Software Foundation, Inc., 51
-// Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
-
-#pragma once
-
-#include "system.hpp"
-
-#include "hash.hpp"
-
-int unify_hash(struct hash* hash, const char* fname, bool print);
index f8d2d9caedbabaa10c6c77096249fbe126c0fb4f..168a480cdd3678f5c26916db911d7f9e0dc047ca 100644 (file)
@@ -472,22 +472,6 @@ base_tests() {
     expect_stat 'cache hit (preprocessed)' 2
     expect_stat 'cache miss' 1
 
-    # -------------------------------------------------------------------------
-    TEST "CCACHE_UNIFY"
-
-    echo '// a silly comment' >>test1.c
-    CCACHE_UNIFY=1 $CCACHE_COMPILE -c test1.c
-    expect_stat 'cache hit (preprocessed)' 0
-    expect_stat 'cache miss' 1
-
-    echo '// another silly comment' >>test1.c
-    CCACHE_UNIFY=1 $CCACHE_COMPILE -c test1.c
-    expect_stat 'cache hit (preprocessed)' 1
-    expect_stat 'cache miss' 1
-
-    $REAL_COMPILER -c -o reference_test1.o test1.c
-    expect_equal_object_files reference_test1.o test1.o
-
     # -------------------------------------------------------------------------
     TEST "CCACHE_NLEVELS"
 
index ae251dc5b0072f74adee007bef0a3cd73fe1eebe..c3e485fd9cbc0f856f939cf7a2205baf783b444c 100644 (file)
@@ -1,4 +1,4 @@
-// Copyright (C) 2011-2019 Joel Rosdahl and other contributors
+// Copyright (C) 2011-2020 Joel Rosdahl and other contributors
 //
 // See doc/AUTHORS.adoc for a complete list of contributors.
 //
@@ -67,7 +67,6 @@ TEST_CASE("Config: default values")
   CHECK(config.stats());
   CHECK(config.temporary_dir().empty());
   CHECK(config.umask() == std::numeric_limits<uint32_t>::max());
-  CHECK_FALSE(config.unify());
 }
 
 TEST_CASE("Config::update_from_file")
@@ -121,8 +120,7 @@ TEST_CASE("Config::update_from_file")
     " ,  no_system_headers,system_headers,clang_index_store\n"
     "stats = false\n"
     "temporary_dir = ${USER}_foo\n"
-    "umask = 777\n"
-    "unify = true"); // Note: no newline.
+    "umask = 777"); // Note: no newline.
 
   Config config;
   REQUIRE(config.update_from_file("ccache.conf"));
@@ -163,7 +161,6 @@ TEST_CASE("Config::update_from_file")
   CHECK_FALSE(config.stats());
   CHECK(config.temporary_dir() == fmt::format("{}_foo", user));
   CHECK(config.umask() == 0777);
-  CHECK(config.unify());
 }
 
 TEST_CASE("Config::update_from_file, error handling")
@@ -402,8 +399,7 @@ TEST_CASE("Config::visit_items")
     " clang_index_store\n"
     "stats = false\n"
     "temporary_dir = td\n"
-    "umask = 022\n"
-    "unify = true\n");
+    "umask = 022\n");
 
   Config config;
   config.update_from_file("test.conf");
@@ -457,7 +453,6 @@ TEST_CASE("Config::visit_items")
     "(test.conf) stats = false",
     "(test.conf) temporary_dir = td",
     "(test.conf) umask = 022",
-    "(test.conf) unify = true",
   };
 
   REQUIRE(received_items.size() == expected.size());