Bug 20788 - bugpoint does not respect the "Alias must point to a definition"

[npjdesres]

Summary

Fix bug 20788.

GlobalAliases may reference function definitions, but not function declarations.

bugpoint would sometimes create invalid IR by deleting a function's body (thus mutating a function definition into a declaration) without first 'fixing' any GlobalAliases that reference that function definition.

This patch iteratively prevents that issue. Before deleting a function's body, it scans the module for GlobalAliases which reference that function. When found, it eliminates them using replaceAllUsesWith.

[hfinkel]

This generally looks good, thanks!; you seem unfamiliar with our general coding conventions, so I've made a number of style comments.

tools/bugpoint/ExtractFunction.cpp

187

declaration -> definition

189

space after for

193

space after for

193

i -> I

194

space after if, no spaced around expression
(same below)

199

General convention would be to use 'e' instead of 'N' here.

200

No spaces around parameter expression.

[npjdesres]

LLVM style guidelines

[npjdesres]

How should I recruit an appropriate reviewer?

[• dberlin]

So, once the issues are fixed, i actually think this is pretty
obviously correct.
I have a bunch of code i will try it on that currently does not work
with bugpoint for this very reason.

[npjdesres]

I thought my updated diff #25062 fixed the all issues. Are there other issues?

[• dberlin]

I have a .bc file (old, admittedly, but passes opt -verify), on which
bugpoint does not work, even with this patch.

After a bunch of cutdowns, it gets:
Checking for crash with only these blocks: .lr.ph .loopexit
_ZN4llvm11SmallVectorIPN5clang6driver6ActionELj3EED1Ev.exit: Alias
must point to a definition
void (%"class.clang::DiagnosticNoteRenderer"*)*
@_ZN5clang22DiagnosticNoteRendererD1Ev
Alias must point to a definition
void (%"class.clang::DiagnosticNoteRenderer"*)*
@_ZN5clang22DiagnosticNoteRendererD2Ev
Alias must point to a definition
void (%"class.llvm::ReturnInst"*)* @_ZN4llvm10ReturnInstD1Ev
Alias must point to a definition

...
etc

I've put it here:
https://drive.google.com/file/d/0BzaAhp8ViDJ6elZEWXF4TVgydU0/view?usp=sharing

(I just made a pass crash if F.size() < 50, and ran it to get bugpoint
to start going :P)

[npjdesres]

@dberlin: your testcase demonstrates that a GlobalAlias may refer to any kind of ConstantExpr, whereas my previous patch incorrectly assumed that the aliasee must be an llvm::Function.

Consequently, the test on line 194 needs a ->stripPointerCasts(). My latest patch handles that testcase correctly.

@dberlin: you mentioned you have many test cases. Would you be so kind as to run this latest patch on them? Thanks,

[• dberlin]

I still get the same crash on bugpoint with this patch, though
slightly further along

Checking for crash with only these blocks: ._crit_edge
_ZN12_GLOBAL__N_111StmtPrinter9PrintExprEPN5clang4ExprE.exit ... <25
total>: Alias must point to a definition
void (%"class.clang::DiagnosticNoteRenderer"*)*
@_ZN5clang22DiagnosticNoteRendererD1Ev
Alias must point to a definition
void (%"class.clang::DiagnosticNoteRenderer"*)*
@_ZN5clang22DiagnosticNoteRendererD2Ev
Alias must point to a definition
void (%"class.llvm::ReturnInst"*)* @_ZN4llvm10ReturnInstD1Ev
Alias must point to a definition
void (%"class.llvm::ReturnInst"*)* @_ZN4llvm10ReturnInstD2Ev
Alias must point to a definition
void (%"class.llvm::AllocaInst"*)* @_ZN4llvm10AllocaInstD1Ev
Alias must point to a definition
void (%"class.llvm::AllocaInst"*)* @_ZN4llvm10AllocaInstD2Ev
Alias must point to a definition
void (%"class.llvm::ModulePass"*)* @_ZN4llvm10ModulePassD1Ev
Alias must point to a definition
void (%"class.llvm::ModulePass"*)* @_ZN4llvm10ModulePassD2Ev
Alias must point to a definition
void (%"class.llvm::ImmutablePass"*)* @_ZN4llvm13ImmutablePassD1Ev
Alias must point to a definition
void (%"class.llvm::ImmutablePass"*)* @_ZN4llvm13ImmutablePassD2Ev
Alias must point to a definition
void (%"class.llvm::raw_null_ostream"*)* @_ZN4llvm16raw_null_ostreamD1Ev
Alias must point to a definition
void (%"class.llvm::raw_null_ostream"*)* @_ZN4llvm16raw_null_ostreamD2Ev
./opt: bugpoint-input-b1bae24.bc: error: input module is broken!
simplifycfg failed!

[npjdesres]

@dberlin: I'm unable to recreate your crash. However, I have a hunch that it pertains to aliases to global variables, rather than aliases to functions.

The new patch (attached) eliminates aliases before calls to GV->setInitializer(nullptr).

[• dberlin]

This one works properly for me on that file.

So LGTM.

[npjdesres]

Sorry, I had forgotten about this diff.

LGTM too. I do not have commit privs.

[hfinkel]

Rebased, and committed: r254171.

tools/bugpoint/ExtractFunction.cpp

std::unique_ptr<Module> BugDriver::extractLoop(Module *M) {
   return NewM;
 }
 
 
 // DeleteFunctionBody - "Remove" the function by deleting all of its basic
 // blocks, making it external.
 //
 void llvm::DeleteFunctionBody(Function *F) {
+  // First, check whether a GlobalAlias references this definition.
+  // GlobalAlias MAY NOT reference declarations.
+  for (;;) {
+    // 1. Find aliases
+    SmallVector<GlobalAlias*,1> aliases;
+    Module *M = F->getParent();
+    for (Module::alias_iterator I=M->alias_begin(), E=M->alias_end(); I!=E; ++I)
+      if (I->getAliasee() == F)
+        aliases.push_back(&*I);
+    if (aliases.empty())
+      break;
+    // 2. Resolve aliases
+    for (unsigned i=0, e=aliases.size(); i<e; ++i) {
+      aliases[i]->replaceAllUsesWith(aliases[i]->getAliasee());
+      aliases[i]->eraseFromParent();
+    }
+    // 3. Repeat until no more aliases found; there might
+    // be an alias to an alias...
+  }
+
   // delete the body of the function...
   F->deleteBody();
   assert(F->isDeclaration() && "This didn't make the function external!");
 }
 
 /// GetTorInit - Given a list of entries for static ctors/dtors, return them
 /// as a constant array.
 static Constant *GetTorInit(std::vector<std::pair<Function*, int> > &TorList) {