[DbgInfo] Fix StripDebugInfo

[tyb0807]

Summary

Hello,

As discussed in https://reviews.llvm.org/D46525, here is my attempt to remove debug intrinsics in StripDebugInfo.

[tyb0807]

UPD: There are 3 regression tests that declare functions starting with llvm.dbg. (e.g. declare void @llvm.dbg.func.start(metadata) nounwind readnone) and are failing:

• Linker/2009-09-03-mdnode.ll
• Transforms/SimplifyCFG/2003-08-17-FoldSwitch-dbg.ll
• Transforms/SimplifyCFG/dbginfo.ll

Should I change those functions' name so that the new code might work? Or should I change the if statement to if (F.getName() == "llvm.dbg.declare" || F.getName() == "llvm.dbg.value" || F.getName() == "llvm.dbg.addr") ?

[vsk]

Thanks for the patch.

lib/IR/DebugInfo.cpp

389

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();
}

test/DebugInfo/strip-intrinsic-dbg.ll

2

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.

[tyb0807]

Address review comments. Other comments inlined

[vsk]

lib/IR/DebugInfo.cpp

385

There's a new debug label instruction you should include: http://llvm.org/doxygen/classllvm_1_1DbgLabelInst.html

test/DebugInfo/strip-intrinsic-dbg.ll

3

Sorry to not have mentioned this earlier, but you can simplify this test further by removing all of the definitions, declaring the debug intrinsics, and running -strip-debug. E.g: "declare void @llvm.dbg.declare()", etc.

tools/opt/opt.cpp

207

Shouldn't -strip-debug do the right thing with debugify metadata?

[tyb0807]

The test is now simplified. Please tell me if you really want to remove -strip-debugify option and use -strip-debug instead.

Thanks for your time

[vsk]

In D46815#1100130, @tyb0807 wrote:

The test is now simplified. Please tell me if you really want to remove -strip-debugify option and use -strip-debug instead.

Thanks, this patch is looking pretty good! And yes, I think there should not be a -strip-debugify option. Strip-debug should simply do the right thing.

[vsk]

lib/IR/DebugInfo.cpp

383

You don't need to specify '4' here, please omit it. Also, just to double-check, has this been clang-formatted?

[tyb0807]

Addressed review comments. Thank you for the help

[vsk]

tools/opt/opt.cpp

759

Why is this change needed? It doesn't seem related to the StripDebugInfo utility.

[tyb0807]

Context added

[vsk]

tools/opt/opt.cpp

759

To clarify, I don't think this change belongs in this patch. There are two different issues here. The first is that the StripDebugInfo function doesn't remove declarations of debug intrinsics. And the second is that opt's -strip-debug mode runs too early.

Can you address only the first issue with this patch?

[tyb0807]

Sure, the patch is updated accordingly.

Thanks for the review!

[vsk]

LGTM. Thanks! Would you like me to commit this for you?

[tyb0807]

Yes please commit this on my behalf.

Thanks a lot

lib/IR/DebugInfo.cpp

389

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

test/DebugInfo/strip-intrinsic-dbg.ll

2

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.

tools/opt/opt.cpp

207

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?

759

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?

[vsk]

lib/IR/DebugInfo.cpp

389

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?

test/DebugInfo/strip-intrinsic-dbg.ll

2

-strip-debug should probably run later than it does; then we wouldn't need a separate -strip-debugify. Sounds like a useful follow-up!

[vsk]

Does opt -strip-dead-prototypes handle all of this?

[tyb0807]

Yes, you'll have to do opt -strip-debug -strip-dead-prototypes. What is done in strip-dead-prototypes is exactly what we do here though... I'm not sure what would cause the ThinLTO test fail...

[vsk]

I think we can close this, then. The functionality needed to test debug info invariance is already in tree. Take a look at https://reviews.llvm.org/rL333861.

[tyb0807]

Hmmm, sorry but I still don't understand how do we remove declare @llvm.dbg.* from the IR though? Or you think that this is not necessary?

[vsk]

Opt's -strip-dead-prototypes mode does exactly this.

echo "declare i32 @foo()" | ./bin/opt -strip-dead-prototypes -S -o -
(empty)

I don't think it's necessary to teach StripDebugInfo() to do the same thing.

vedant

[tyb0807]

I get it now. Thank you for your time. So anything else that needs to be fixed? Otherwise I'll tackle the verify-each mode

[vsk]

If you have spare cycles, could you try to take a look at debug value loss in instcombine? I started looking at this issue circa r323570 but got sidetracked with other work.

Anastasis has a great writeup on his blog about how to find bugs in this area. See: https://gramanas.github.io/posts/finding-debuginfo-loss/.

Here's one example you might look at:

$ ./bin/opt -enable-debugify -instcombine ../llvm/test/Transforms/InstCombine/zext.ll -S -o -
ERROR: Missing variable 5
...

Each missing variable corresponds to some DILocalVariable which has no corresponding llvm.dbg.value instruction. Ideally we'd be able to emit a debug intrinsic for each variable using simple and non-intrusive methods. Keep in mind that this may not always be possible.

thanks,
vedant

lib/IR/DebugInfo.cpp

if (!MDs.empty()) {
       GV.eraseMetadata(LLVMContext::MD_dbg);
       Changed = true;
     }
   }
 
   if (GVMaterializer *Materializer = M.getMaterializer())
     Materializer->setStripDebugInfo();
 
+  StringRef DbgIntrinsics[] = {"llvm.dbg.value", "llvm.dbg.declare",
+                               "llvm.dbg.addr", "llvm.dbg.label"};
+  for (StringRef FuncName : DbgIntrinsics) {
+    auto *F = M.getFunction(FuncName);
+    if (!F)
+      continue;
+    F->eraseFromParent();
+    Changed = true;
+  }
+
   return Changed;
 }
 
 namespace {
 
 /// Helper class to downgrade -g metadata to -gline-tables-only metadata.
 class DebugTypeInfoRemoval {
   DenseMap<Metadata *, Metadata *> Replacements;

test/DebugInfo/strip-intrinsic-dbg.ll

+; RUN: opt -S -strip-debug <%s | FileCheck %s
+
+; CHECK-NOT: llvm.dbg.
+declare void @llvm.dbg.declare(metadata, metadata, metadata)
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+declare void @llvm.dbg.addr(metadata, metadata, metadata)
+declare void @llvm.dbg.label(metadata, metadata, metadata)

tools/opt/opt.cpp

 static cl::alias
 QuietA("quiet", cl::desc("Alias for -q"), cl::aliasopt(Quiet));
 
 static cl::opt<bool>
 AnalyzeOnly("analyze", cl::desc("Only perform analysis, no optimization"));
 
 static cl::opt<bool> EnableDebugify(
     "enable-debugify",
     cl::desc(
         "Start the pipeline with debugify and end it with check-debugify"));
 
 static cl::opt<bool> DebugifyEach(
     "debugify-each",
     cl::desc(
         "Start each pass with debugify and end it with check-debugify"));
 
 static cl::opt<bool>
if (FPasses) {
     FPasses->doFinalization();
   }
 
   // Check that the module is well formed on completion of optimization
   if (!NoVerify && !VerifyEach)
     Passes.add(createVerifierPass());
 
   if (AddOneTimeDebugifyPasses)
-    Passes.add(createCheckDebugifyModulePass(false));
+    Passes.add(createCheckDebugifyModulePass(StripDebug));
 
   // In run twice mode, we want to make sure the output is bit-by-bit
   // equivalent if we run the pass manager again, so setup two buffers and
   // a stream to write to them. Note that llc does something similar and it
   // may be worth to abstract this out in the future.
   SmallVector<char, 0> Buffer;
   SmallVector<char, 0> FirstRunBuffer;
   std::unique_ptr<raw_svector_ostream> BOS;