some passes removed?

[wisechengyi]

At sha 15b6504b8b9d404f8f480e88136959f38fd3760e, if I added the following passes based on https://github.com/facebook/redex/blob/master/config/default.config, it will throw the following error:

$ git diff
diff --git a/twitter.json b/twitter.json
index 5346d36..f0d8d10 100644
--- a/twitter.json
+++ b/twitter.json
@@ -6,10 +6,15 @@
       "SynthPass",
       "FinalInlinePass",
       "SimpleInlinePass",
+      "PeepholePassV2",
+      "LocalConstantPropagationPass",
+      "RedundantMoveEliminationPass",
       "LocalDcePass",
+      "RemoveGotosPass",
       "DelSuperPass",
       "SingleImplPass",
       "StaticReloPass",
+      "ReorderInterfacesPass",
       "RemoveEmptyClassesPass",
       "ShortenSrcStringsPass"
     ]

error:

libredex/PassManager.cpp:160: void PassManager::activate_pass(const char *, const Json::Value &): assertion `false' failed.
No pass named PeepholePassV2!
0   redex-all                           0x000000010cdaadde _Z15crash_backtracev + 46
1   redex-all                           0x000000010cdaaeed _Z11assert_failPKcS0_jS0_S0_z + 173
2   redex-all                           0x000000010cdea7cc _ZN11PassManager13activate_passEPKcRKN4Json5ValueE + 140
3   redex-all                           0x000000010cdeaa1a _ZN11PassManagerC2ERKNSt3__16vectorIP4PassNS0_9allocatorIS3_EEEERKN5redex21ProguardConfigurationERKN4Json5ValueE + 362
4   redex-all                           0x000000010cd9cb70 main + 3968
5   libdyld.dylib                       0x00007fff9a7b6235 start + 1
6   ???                                 0x0000000000000033 0x0 + 51
libc++abi.dylib: terminating with uncaught exception of type std::runtime_error: Redex assertion failure
0   redex-all                           0x000000010cdaadde _Z15crash_backtracev + 46
1   redex-all                           0x000000010cdaae1d _Z23crash_backtrace_handleri + 13
2   libsystem_platform.dylib            0x00007fff9a9c5b3a _sigtramp + 26
3   ???                                 0x00000004a36b4240 0x0 + 19921584704
4   libsystem_c.dylib                   0x00007fff9a84a420 abort + 129
5   libc++abi.dylib                     0x00007fff993a084a __cxa_bad_cast + 0
6   libc++abi.dylib                     0x00007fff993c5c37 _ZL25default_terminate_handlerv + 243
7   libobjc.A.dylib                     0x00007fff99ed3713 _ZL15_objc_terminatev + 124
8   libc++abi.dylib                     0x00007fff993c2d69 _ZSt11__terminatePFvvE + 8
9   libc++abi.dylib                     0x00007fff993c27de _ZN10__cxxabiv1L22exception_cleanup_funcE19_Unwind_Reason_CodeP17_Unwind_Exception + 0
10  redex-all                           0x000000010cdaaf1f _Z11assert_failPKcS0_jS0_S0_z + 223
11  redex-all                           0x000000010cdea7cc _ZN11PassManager13activate_passEPKcRKN4Json5ValueE + 140
12  redex-all                           0x000000010cdeaa1a _ZN11PassManagerC2ERKNSt3__16vectorIP4PassNS0_9allocatorIS3_EEEERKN5redex21ProguardConfigurationERKN4Json5ValueE + 362
13  redex-all                           0x000000010cd9cb70 main + 3968
14  libdyld.dylib                       0x00007fff9a7b6235 start + 1
15  ???                                 0x0000000000000033 0x0 + 51

[justinjhendrick]

Well that's super weird... looking

[justinjhendrick]

I'm not seeing the same issue. You're rebuilding redex when you move to a different commit, right?

[justinjhendrick]

(that doesn't really make sense though because PeepholePassV2 was introduced a while ago)

Could you send me your config file, please?

[wisechengyi]

I'm not seeing the same issue. You're rebuilding redex when you move to a different commit, right?

Yes. Did git clean -fdx first then the build. The build was on mac. I will verify it on centos

Config is just https://github.com/facebook/redex/blob/master/config/default.config.

[justinjhendrick]

I've found the issue https://github.com/facebook/redex/blob/master/libredex/PassManager.cpp#L154 compares strings by their pointers. Fixing. Thanks for finding this.

[justinjhendrick]

Actually, it was already calling std::string::operator==(char*, std::string); So I don't think this will fix it, but hey. It's worth a try:

diff --git a/libredex/PassManager.cpp b/libredex/PassManager.cpp
--- a/libredex/PassManager.cpp
+++ b/libredex/PassManager.cpp
@@ -40,7 +40,7 @@
   if (config["redex"].isMember("passes")) {
     auto passes = config["redex"]["passes"];
     for (auto& pass : passes) {
-      activate_pass(pass.asString().c_str(), config);
+      activate_pass(pass.asString(), config);
     }
   } else {
     // If config isn't set up, run all registered passes.
@@ -58,7 +58,7 @@
   if (config["redex"].isMember("passes")) {
     auto passes = config["redex"]["passes"];
     for (auto& pass : passes) {
-      activate_pass(pass.asString().c_str(), config);
+      activate_pass(pass.asString(), config);
     }
   } else {
     // If config isn't set up, run all registered passes.
@@ -149,7 +149,7 @@
   }
 }

-void PassManager::activate_pass(const char* name, const Json::Value& cfg) {
+void PassManager::activate_pass(const std::string& name, const Json::Value& cfg) {
   for (auto pass : m_registered_passes) {
     if (name == pass->name()) {
       m_activated_passes.push_back(pass);
@@ -157,7 +157,7 @@
       return;
     }
   }
-  always_assert_log(false, "No pass named %s!", name);
+  always_assert_log(false, "No pass named %s!", name.c_str());
 }

 void PassManager::incr_metric(const std::string& key, int value) {
diff --git a/libredex/PassManager.h b/libredex/PassManager.h
--- a/libredex/PassManager.h
+++ b/libredex/PassManager.h
@@ -51,7 +51,7 @@
   void set_testing_mode() { m_testing_mode = true; }

  private:
-  void activate_pass(const char* name, const Json::Value& cfg);
+  void activate_pass(const std::string& name, const Json::Value& cfg);

   Json::Value m_config;
   std::vector<Pass*> m_registered_passes;

[wisechengyi]

operator overloading in c++. good times :)

[wisechengyi]

Hi @justinjhendrick please see https://github.com/wisechengyi/redex_repro/tree/master/ubuntu/228 for repro

Readme is at top level. Thank you!

[justinjhendrick]

I can repro! YAY! finally

Alright, time to debug it, haha.

[wisechengyi]

Great! It's possible to hit #192 once this is issue is fixed. Just heads up so you are not confused by a different exception message.

[justinjhendrick]

Okay. I've found the issue. Basically, the Makefile is out of date. When we add new files and directories, we need to add them to Makefile.am. I'm going to rewrite the Makefile with ample use of find so this won't happen again in the future.

Specifically, we created PeepholeV2 and forgot to add it here https://github.com/facebook/redex/blob/master/Makefile.am#L139

[wisechengyi]

Thanks for the quick turn around! Not top priority, although the fix does not seem to be very maintainable going forward since all the files are hardcoded. We could potentially use something like https://github.com/wisechengyi/Particle-System/blob/master/418/mp4/Makefile#L14-L25 in the Makefile.

[justinjhendrick]

make allows for wildcards, but automake does not. In this commit I added a test running on our commits internally that should catch missing files in Makefile.am