[bugpoint] Add NoStripSymbols option.

[hintonda]

Summary

Add NoStripSymbols option that matches the behavior of -strip pass.

Much of this patch is a cut-n-paste of several static functions from
StripSymbols.cpp. A better option might be to make them non-static
and add their declarations to a header -- however, since I'm unsure
where they should go, I just cut-n-pasted them for now.

[vsk]

A simpler way to do this might be to call 'strip' on the module as an final cleanup, just like we do for GlobalDCE.

[hintonda]

In D43601#1015341, @vsk wrote:

A simpler way to do this might be to call 'strip' on the module as an final cleanup, just like we do for GlobalDCE.

So, always run it or use a flag?

[vsk]

In D43601#1015343, @hintonda wrote:

In D43601#1015341, @vsk wrote:

A simpler way to do this might be to call 'strip' on the module as an final cleanup, just like we do for GlobalDCE.

So, always run it or use a flag?

Gating the behavior behind an off-by-default flag sgtm.

[hintonda]

In D43601#1015344, @vsk wrote:

In D43601#1015343, @hintonda wrote:

In D43601#1015341, @vsk wrote:

A simpler way to do this might be to call 'strip' on the module as an final cleanup, just like we do for GlobalDCE.

So, always run it or use a flag?

Gating the behavior behind an off-by-default flag sgtm.

You mean like the other "disable-strip*" options, right?

It would be unfortunate to have this one behave differently.

[MatzeB]

But we don't want user flags for bugpoint, we want it to try both variants and keep the smaller one when it still reproduces the crash.

I don't really know the bugpoint code, but I would expect one of those ListReducer thingies getting implemented (like the existing ones in CrashDebugger.cpp) so that bugpoint will try out both: with and without debug info.

[MatzeB]

Okay reading the code a bit more seems like that is what you are doing.

In D43601#1015404, @MatzeB wrote:

But we don't want user flags for bugpoint, we want it to try both variants and keep the smaller one when it still reproduces the crash.

Okay reading the code some more, it seems like that is what you are doing here and the option is just to disable the extra try.

• You should split the OptionCategory changes into a separate commit.
• I suspect a lot of this code is duplicated with the -strip pass, would it be possible to refactor it into a utility function used by both?

tools/bugpoint/CrashDebugger.cpp

92

maybe this can use a ranged for too? Similar for many of the other for loops.

[hintonda]

In D43601#1015412, @MatzeB wrote:

Okay reading the code a bit more seems like that is what you are doing.

In D43601#1015404, @MatzeB wrote:

But we don't want user flags for bugpoint, we want it to try both variants and keep the smaller one when it still reproduces the crash.

Okay reading the code some more, it seems like that is what you are doing here and the option is just to disable the extra try.

• You should split the OptionCategory changes into a separate commit.

Yes, I'll submit that patch separately, and those changes will disappear. I can go ahead and remove them if you want -- just made it easier to see what was going on -- currently the options are just a big ball of mud and hard to grok...

• I suspect a lot of this code is duplicated with the -strip pass, would it be possible to refactor it into a utility function used by both?

Sure, but first I'm trying to pursue Vedant's suggestion of passing -strip-nondebug to performFinalCleanups(). However, I'm running into an issue I need to debug first. Will report back once I figure that out.

[hintonda]

tools/bugpoint/CrashDebugger.cpp

92

Sure, but this is part of the cut-n-paste. Let's see if I can make all of it go away before I start hacking it up...

[hintonda]

Address comments.

[hintonda]

In D43601#1015341, @vsk wrote:

A simpler way to do this might be to call 'strip' on the module as an final cleanup, just like we do for GlobalDCE.

Unfortunately this won't work -- we only want -strip-nondebug, but the logic in performFinalCleanups() does a lot more and is predicated on producing output -- which isn't always the case.

[MatzeB]

include/llvm/Transforms/IPO.h

34

This needs a doxygen comment.

lib/Transforms/IPO/StripSymbols.cpp

204

You could go for llvm::StripSymbolNames() instead of the namespace llvm {} if it's just this one function (just FYI I don't care either way).

test/BugPoint/strip-names-and-types.ll

17–18

This is also stripping symbols from SSA values and not just from types isn't it? The test should check at least a def and use of an SSA value then I think.

tools/bugpoint/CrashDebugger.cpp

73

This seems superfluous given that it only has a single caller.

[hintonda]

• Don't clone module when stripping.
• Address more comments.

[hintonda]

• Add doxygen comment.

[hintonda]

• Enhance test.

[hintonda]

This should only fire for crashes, but the test case is insufficient, and I don't have time right now to try to come up with one.

Also, I've come to believe hacking bugpoint like this is a bad idea -- I prefer replacement.

include/llvm/Transforms/IPO.h

 class ModuleSummaryIndex;
 class ModulePass;
 class Pass;
 class Function;
 class BasicBlock;
 class GlobalValue;
 class raw_ostream;
 
+/// This function strips symbols from functions and modules.  If PreserveDbgInfo
+/// is true, debugging information is not removed.
+bool StripSymbolNames(Module &M, bool PreserveDbgInfo);
+
 //===----------------------------------------------------------------------===//
 //
 // These functions removes symbols from functions and modules.  If OnlyDebugInfo
 // is true, only debugging information is removed from the module.
 //
 ModulePass *createStripSymbolsPass(bool OnlyDebugInfo = false);
 
 //===----------------------------------------------------------------------===//

lib/Transforms/IPO/StripSymbols.cpp

static void findUsedValues(GlobalVariable *LLVMUsed,
   ConstantArray *Inits = cast<ConstantArray>(LLVMUsed->getInitializer());
 
   for (unsigned i = 0, e = Inits->getNumOperands(); i != e; ++i)
     if (GlobalValue *GV =
           dyn_cast<GlobalValue>(Inits->getOperand(i)->stripPointerCasts()))
       UsedValues.insert(GV);
 }
 
-/// StripSymbolNames - Strip symbol names.
-static bool StripSymbolNames(Module &M, bool PreserveDbgInfo) {
+bool llvm::StripSymbolNames(Module &M, bool PreserveDbgInfo) {
 
   SmallPtrSet<const GlobalValue*, 8> llvmUsedValues;
   findUsedValues(M.getGlobalVariable("llvm.used"), llvmUsedValues);
   findUsedValues(M.getGlobalVariable("llvm.compiler.used"), llvmUsedValues);
 
   for (Module::global_iterator I = M.global_begin(), E = M.global_end();
        I != E; ++I) {
     if (I->hasLocalLinkage() && llvmUsedValues.count(&*I) == 0)

test/BugPoint/strip-names-and-types.ll

+; REQUIRES: loadable_module
+; RUN: bugpoint %s -output-prefix %t -silence-passes -safe-run-llc
+; RUN: llvm-dis %t-reduced-simplified.bc -o - | FileCheck %s
+; RUN: bugpoint %s -output-prefix %t -disable-strip-symbols -silence-passes -safe-run-llc
+; RUN: llvm-dis %t-reduced-simplified.bc -o - | FileCheck --check-prefix=DISABLE %s
+
+%type0 = type { i32, i64 }
+
+define i1 @foo(i1 %A) {
+  ret i1 %A
+}
+
+; CHECK-LABEL: define i32 @test
+define i32 @test() {
+  %A = icmp eq %type0* undef, undef
+; CHECK: icmp eq %0* undef, undef
+; DISABLE: icmp eq %type0* undef, undef
+  call i1 @foo(i1 %A)
+; CHECK: %2 = call i1 @foo(i1 %1)
+; DISABLE: call i1 @foo(i1 %A)
+  ret i32 0
+}

tools/bugpoint/CrashDebugger.cpp

 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/LegacyPassManager.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/ValueSymbolTable.h"
 #include "llvm/IR/Verifier.h"
 #include "llvm/Pass.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileUtilities.h"
+#include "llvm/Transforms/IPO.h"
 #include "llvm/Transforms/Scalar.h"
 #include "llvm/Transforms/Utils/BasicBlockUtils.h"
 #include "llvm/Transforms/Utils/Cloning.h"
 #include "llvm/Transforms/Utils/Local.h"
 #include <set>
 using namespace llvm;
 
 namespace {
cl::opt<bool> NoNamedMDRM("disable-namedmd-remove",
                           cl::desc("Do not remove global named metadata"),
                           cl::init(false));
 cl::opt<bool> NoStripDebugInfo("disable-strip-debuginfo",
                                cl::desc("Do not strip debug info metadata"),
                                cl::init(false));
 cl::opt<bool> NoStripDebugTypeInfo("disable-strip-debug-types",
                                cl::desc("Do not strip debug type info metadata"),
                                cl::init(false));
+cl::opt<bool> NoStripSymbols("disable-strip-symbols",
+                             cl::desc("Do not strip symbol and type names"),
+                             cl::init(false));
 cl::opt<bool> VerboseErrors("verbose-errors",
                             cl::desc("Print the output of crashing program"),
                             cl::init(false));
 }
 
 namespace llvm {
 class ReducePassList : public ListReducer<std::string> {
   BugDriver &BD;
 
 public:
   ReducePassList(BugDriver &bd) : BD(bd) {}
 
   // Return true iff running the "removed" passes succeeds, and running the
   // "Kept" passes fail when run on the output of the "removed" passes.  If we
   // return true, we update the current module of bugpoint.
   Expected<TestResult> doTest(std::vector<std::string> &Removed,
                               std::vector<std::string> &Kept) override;
 };
 }
 
 Expected<ReducePassList::TestResult>
 ReducePassList::doTest(std::vector<std::string> &Prefix,
                        std::vector<std::string> &Suffix) {
   std::string PrefixOutput;
   std::unique_ptr<Module> OrigProgram;
   if (!Prefix.empty()) {
     outs() << "Checking to see if these passes crash: "
            << getPassesString(Prefix) << ": ";
     if (BD.runPasses(BD.getProgram(), Prefix, PrefixOutput))
       return KeepPrefix;
 
     OrigProgram = std::move(BD.Program);
 
static Error DebugACrash(BugDriver &BD, BugTester TestFn) {
   if (!NoStripDebugInfo && !BugpointIsInterrupted) {
     outs() << "\n*** Attempting to strip the debug info: ";
     stripMetadata(StripDebugInfo);
   }
   if (!NoStripDebugTypeInfo && !BugpointIsInterrupted) {
     outs() << "\n*** Attempting to strip the debug type info: ";
     stripMetadata(stripNonLineTableDebugInfo);
   }
+  if (!NoStripSymbols && !BugpointIsInterrupted) {
+    outs() << "\n*** Attempting to strip symbol and type names: ";
+    StripSymbolNames(BD.getProgram(), true);
+  }
 
   if (!NoNamedMDRM) {
     if (!BugpointIsInterrupted) {
       // Try to reduce the amount of global metadata (particularly debug info),
       // by dropping global named metadata that anchors them
       outs() << "\n*** Attempting to remove named metadata: ";
       std::vector<std::string> NamedMDNames;
       for (auto &NamedMD : BD.getProgram().named_metadata())