Page MenuHomePhabricator

[DbgInfo] Fix StripDebugInfo
AbandonedPublic

Authored by tyb0807 on May 13 2018, 3:59 PM.

Details

Reviewers
vsk
Summary

Hello,

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

Diff Detail

Event Timeline

tyb0807 created this revision.May 13 2018, 3:59 PM

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 requested changes to this revision.May 14 2018, 5:39 PM

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.

This revision now requires changes to proceed.May 14 2018, 5:39 PM
tyb0807 updated this revision to Diff 146793.May 15 2018, 5:19 AM
tyb0807 marked 2 inline comments as done.

Address review comments. Other comments inlined

vsk added inline comments.May 15 2018, 11:21 AM
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 ↗(On Diff #146793)

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

tyb0807 updated this revision to Diff 146917.May 15 2018, 2:25 PM
tyb0807 marked 2 inline comments as done.

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 added a comment.May 15 2018, 2:28 PM

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 added inline comments.May 15 2018, 2:36 PM
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 updated this revision to Diff 147018.May 16 2018, 2:23 AM
tyb0807 marked 2 inline comments as done.

Addressed review comments. Thank you for the help

vsk added inline comments.May 16 2018, 9:58 AM
tools/opt/opt.cpp
759 ↗(On Diff #147018)

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

tyb0807 updated this revision to Diff 147452.May 18 2018, 2:35 AM
tyb0807 marked an inline comment as done.

Context added

vsk added inline comments.May 18 2018, 9:11 AM
tools/opt/opt.cpp
759 ↗(On Diff #147018)

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 updated this revision to Diff 147532.May 18 2018, 9:19 AM

Sure, the patch is updated accordingly.

Thanks for the review!

vsk accepted this revision.May 18 2018, 11:06 AM

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

This revision is now accepted and ready to land.May 18 2018, 11:06 AM

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
759 ↗(On Diff #147018)

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?

207 ↗(On Diff #146793)

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?

vsk added inline comments.May 24 2018, 11:08 AM
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 requested changes to this revision.May 24 2018, 11:08 AM
This revision now requires changes to proceed.May 24 2018, 11:08 AM
vsk added a comment.Jun 3 2018, 3:56 PM

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

tyb0807 added a comment.EditedJun 4 2018, 6:26 AM

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 added a comment.Jun 4 2018, 10:11 AM

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.

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 added a subscriber: tyb0807.Jun 4 2018, 10:32 AM

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

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 added a subscriber: gramanas.Jun 4 2018, 4:55 PM

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

tyb0807 abandoned this revision.Jun 28 2018, 6:46 AM