[PATCH] D46815: [DbgInfo] Fix StripDebugInfo

[Son Tuan Vu via Phabricator via llvm-commits]

tyb0807 added a comment.

Yes please commit this on my behalf.

Thanks a lot



================
Comment at: lib/IR/DebugInfo.cpp:389
+    }
+  }
+
----------------
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
```


================
Comment at: test/DebugInfo/strip-intrinsic-dbg.ll:1
+; RUN: opt -S -strip-debug <%s | FileCheck %s
+
----------------
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.


================
Comment at: tools/opt/opt.cpp:207
+StripDebugify("strip-debugify",
+           cl::desc("Strip debugify metadata in enable-debugify after check."));
+
----------------
vsk wrote:
> Shouldn't -strip-debug do the right thing with debugify metadata?
No -strip-debug is used to strip debug info in the original IR so  it calls to `StripDebugInfo` before any pass run. So the debugify metadata is added after the stripping.

Or do you mean that I should use the same option to strip debugify metadata?


================
Comment at: tools/opt/opt.cpp:759
-    Passes.add(createCheckDebugifyModulePass(false));
+    Passes.add(createCheckDebugifyModulePass(StripDebug));
----------------
vsk wrote:
> Why is this change needed? It doesn't seem related to the StripDebugInfo utility.
This makes it possible to strip debugify metadata when -enable-debugify option enabled. I should have thought about this in the previous patch in fact...

Do you want me to remove this here and send another patch?


https://reviews.llvm.org/D46815