[PATCH] D46815: [DbgInfo] Fix StripDebugInfo

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
Comment at: lib/IR/DebugInfo.cpp:389
+    }
+  }
+
----------------
tyb0807 wrote:
> vsk wrote:
> > This takes linear time. There's a cheaper way to do this:
> > 
> > ```
> > StringRef DbgIntrinsics = {"llvm.dbg.value", ...};
> > for (StringRef Func : DbgIntrinsics) {
> >   auto *F = M.getFunction(Func);
> >   if (!F)
> >     continue;
> >   assert(F->use_empty());
> >   F->eraseFromParent();
> > }
> > ```
> Thanks, this is awesome!
> 
> However, the `use_empty()` call does break a test ( test/ThinLTO/X86/drop-debug-info.ll) though, so I remove it. Here's the stack trace if you'd like to take a look:
> ```
> $ opt -module-summary test/ThinLTO/X86/drop-debug-info.ll -o tmp
> $ llvm-lto -thinlto-action=thinlink -o tmp.bc tmp  test/ThinLTO/X86/Inputs/drop-debug-info.bc
> $ llvm-link tmp -summary-index=tmp.bc -import=globalfunc:test/ThinLTO/X86/Inputs/drop-debug-info.bc | llvm-dis
> 
> llvm-link: llvm/lib/IR/Value.cpp:367: void llvm::Value::assertModuleIsMaterializedImpl() const: Assertion `M->isMaterialized()' failed.
> Stack dump:
> #9 0x00007f3737f49805 llvm::Value::assertModuleIsMaterializedImpl() const /dsk/l1/misc/vusontuan/llvm/lib/IR/Value.cpp:369:0
> #10 0x00007f3737cbadde llvm::Value::assertModuleIsMaterialized() const /dsk/l1/misc/vusontuan/llvm/include/llvm/IR/Value.h:326:0
> #11 0x00007f3737cbadf8 llvm::Value::use_empty() const /dsk/l1/misc/vusontuan/llvm/include/llvm/IR/Value.h:330:0
> #12 0x00007f3737db689a llvm::StripDebugInfo(llvm::Module&) /dsk/l1/misc/vusontuan/llvm/lib/IR/DebugInfo.cpp:390:0
> #13 0x00007f3737d24b75 llvm::UpgradeDebugInfo(llvm::Module&) /dsk/l1/misc/vusontuan/llvm/lib/IR/AutoUpgrade.cpp:2881:0
> #14 0x00007f3735eecc40 llvm::FunctionImporter::importFunctions(llvm::Module&, llvm::StringMap<std::map<unsigned long, unsigned int, std::less<unsigned long>, std::allocator<std::pair<unsigned long const, unsigned int> > >, llvm::MallocAllocator> const&) /dsk/l1/misc/vusontuan/llvm/lib/Transforms/IPO/FunctionImport.cpp:973:0
> ```
I don't think it would be correct to remove a function from a module when there are still uses of it. Could you dig into this further to see if you can find out why all uses of the debug intrinsics aren't stripped out?


================
Comment at: test/DebugInfo/strip-intrinsic-dbg.ll:1
+; RUN: opt -S -strip-debug <%s | FileCheck %s
+
----------------
tyb0807 wrote:
> vsk wrote:
> > You could simplify the test with -debugify :).
> > 
> > RUN: opt -S < %s -debugify -strip-debug | ...
> > 
> > This way, there's no need to check in all the extra metadata lines.
> This does not work because in opt `StripDebugInfo` is called before any pass run...
> 
> I've added a `strip-debugify` option so that we can strip debugify metadata after enable-debugify check.
> 
> However, I do not see how to strip debugify metadata when pass `-check-debugify` to `opt`, since `CheckDebugify*Pass` is registered through `RegisterPass` that does not accept parameter to the constructor of the pass.
-strip-debug should probably run later than it does; then we wouldn't need a separate -strip-debugify. Sounds like a useful follow-up!


https://reviews.llvm.org/D46815