Page MenuHomePhabricator

[Debugify] Make the debugify aware of the original (-g) Debug Info
ClosedPublic

Authored by djtodoro on Jun 25 2020, 6:31 AM.

Details

Summary

As discussed on the RFC [0], I am sharing the set of patches that enables checking of original Debug Info metadata preservation in optimizations. The proof-of-concept/proposal can be found at [1].

The implementation from the [1] was full of duplicated code, so this set of patches tries to merge this approach into the existing debugify utility.

For example, the utility pass in the original-debuginfo-check mode could be invoked as follows:

$ opt -verify-debuginfo-preserve -pass-to-test sample.ll

Since this is very initial stage of the implementation, there is a space for improvements such as:

  • Add support for the new pass manager
  • Add support for metadata other than DILocations and DISubprograms

[0] https://groups.google.com/forum/#!msg/llvm-dev/QOyF-38YPlE/G213uiuwCAAJ
[1] https://github.com/djolertrk/llvm-di-checker

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
djtodoro updated this revision to Diff 277777.Jul 14 2020, 4:30 AM
  • Rebase
  • TODO: Consider another name for mode. 'original' might be too sharp.

Hi @vsk, is this in better shape now? :)

Perhaps worth pinging @dsanders or anyone else who wrote bits of the original debugify support? I don't have any real context for it, so am not the best reviewer here.

@dblaikie Thanks for your comment!

aprantl added inline comments.Sep 11 2020, 7:34 AM
llvm/docs/HowToUpdateDebugInfo.rst
220–221

Can we call this

The ``debugify`` utility pass

?

I kind of expect a utility to be a standalone executable.

230

I think that this is a useful feature, but from the user's perspective the naming is confusing.

The name debugify implies that that the tool is taking input and then adds something debug-related to it. That clashes with preserving original information.

My suggestion is to either rename debugify to something more generic that encompasses both use-cases, or to share the implementation in one class, but give it two separate user-facing names. Maybe separating out the debugify action from the check action would also work (--enable-debugify --enable-debuginfo-(preservation-)verification).

llvm/include/llvm/Transforms/Utils/Debugify.h
48

Why is this a std::map and not a DenseMap? Do we need the map's sorted-ness?

vsk added a comment.Sep 14 2020, 10:51 AM

@djtodoro sorry for the delay here. This looks much nicer now.

llvm/docs/HowToUpdateDebugInfo.rst
305

If 'synthetic/original' is not literally a string that can be passed to the -enable-debugify cl::opt, I think this example will be misleading. Perhaps we can sidestep the issue by picking a default behavior for -enable-debugify?

llvm/include/llvm/Transforms/Utils/Debugify.h
52

s/and its/to its associated/?

54

nit: 'about whether'?

160

This can be simplified by just storing a single DebugifyMode enum.

200

We shouldn't need two sets of super::add calls here. We should simply pass in the DebugifyMode enum, as well as the rest of the optional arguments that might be needed in original mode.

205

Ditto, I don't think we need two sets of super::add calls here either.

llvm/lib/Transforms/Utils/Debugify.cpp
310

Does this need to be const DebugFnMap &?

316

Early continue?

322

No need to check if (Preserved) before assigning it -- we can leave that sort of optimization to the compiler.

323

Early continue here as well?

331

Ditto.

349

Early continue?

355

Is Instr an llvm::Instruction *? I wonder whether it is sound to assume that pointer equality for llvm::Instruction implies actual equality, if we're comparing pointers before/after a pass runs. It's possible for a pass to delete and then create instructions: that could result in pointer reuse/recycling.

357

This might need to be relaxed to a warning, since we do specifically recommend that pass authors drop debug locations in certain transformations.

411

There can be a single map insertion call here, the branch is only needed to pretty-print SP when it's non-null.

djtodoro marked 4 inline comments as done.Sep 17 2020, 4:05 AM

@aprantl @vsk Thanks a lot for the reviews!

llvm/docs/HowToUpdateDebugInfo.rst
220–221

I agree with that.

230

Sure, I'll try to find a name for the pass(es) that will cover both use-cases. Thanks!

305

I'll try to make some changes to avoid ambiguity here.

llvm/include/llvm/Transforms/Utils/Debugify.h
48

Oh, I've forgotten why I have (initially) used the std::map here, but yes, we don't need the balanced data structure for this, indeed. Thanks!

52

I agree, thanks.

54

I agree, thanks.

160

Sure.

200

Good point. I was planning to do the final refactoring, but I haven't had time for that, thanks for catching this!

llvm/lib/Transforms/Utils/Debugify.cpp
310

Yes, I'll add it to some other spots as well.

316

Sure, thanks.

322

I agree, thanks.

355

It is an llvm::Instruction *. I see... any idea how to address that?
(I was trying to avoid having to make a temp object of the instr...)

357

Agree.. We can extend this (one day) to catch the cases where we actually allow dropping of the di locations, but until then, it is ok to be a warning.

djtodoro updated this revision to Diff 292465.Sep 17 2020, 5:23 AM
djtodoro edited the summary of this revision. (Show Details)
  • Addressing comments
  • Refactoring
  • Rename debugify passes into more general name (in order to support original debug info check) - VerifyDIPreserve
  • Rename the options
  • Fix the test cases
  • Update the docs
  • Rebasing
vsk added inline comments.Sep 24 2020, 1:59 PM
llvm/lib/Transforms/Utils/Debugify.cpp
355

I dug into this a bit and unfortunately it looks like a thorny issue. It looks like Instructions are (de)allocated via User::operator new and User::operator delete, and these essentially call into the system allocator. Relying on pointer equality to identify a unique instruction before/after a pass modifies the IR might necessitate e.g. changing the delete implementation to never free memory. That might not be practical -- it's certainly a big hammer.

It's possible that there's some alternative approach, but I'm not sure whether they'd be effective. One idea might be to use metadata to uniquely identify instructions (not !dbg metadata, of course :), and to teach the dropUnknownNonDebugMetadata utility to preserve that "inst ID" metadata. (I'd be curious to get some other opinions on whether that's workable.)

djtodoro added inline comments.Sep 25 2020, 2:57 AM
llvm/lib/Transforms/Utils/Debugify.cpp
355

Thanks! I have also taken a look, and I agree that the gymnastic with the new/delete operators won't be practical choice.

Using some special kind of metadata can help us for this purpose. The "inst ID" metadata sounds good to me, since we are going to use it for this purpose only. We can call it !debug-instr-number, similar to the LiveDebugValues/InstrRef (D85741) work.

Let me try to implement a prototype for it, so we can see what are the pros && cons.

StephenTozer added inline comments.
llvm/include/llvm/Transforms/Utils/VerifyDIPreserve.h
165 ↗(On Diff #292465)

Minor, this first condition would look better as Mode != VerifyDIPreserveMode::NoVerify in my opinion.

llvm/lib/Transforms/Utils/Debugify.cpp
325

Minor, attacched -> attached.

355

The metadata approach should work, although as discussed it may require changes to dropUnknownNonDebugMetadata. It might be a little messy though; I think it would also be viable to take the following approach: In the "before" step, populate a map of Instruction* Instr -> WeakVH(Instr). With this map, you can check Map[Instr] != nullptr to determine whether Instr was deleted at any point in the pass; if it wasn't deleted, then you can simply use raw pointer comparison. This avoids the need to fiddle with the internals of Instruction that could potentially affect performance or the behaviour of other code.

I think it worth adding that there will be some noise with this approach; the DebugLoc metadata for any new generated instructions will be dependent on the DebugLoc metadata for the instruction they're based on. For example, cloning an instruction , replacing a conditional branch with an unconditional one, reassociating arithmetic, etc. In each of these cases, we will get a warning from the preservation pass that a new instruction did not receive a DILocation, even though the pass that generated it had no responsibility for it. It's technically correct, but it may result in a continuously increasing amount of noise from pass-to-pass, with the worst case being that there are no DebugLocs left at the start of a pass so that we get an error for every instruction created even if the pass itself is handling DebugLocs correctly.

Unfortunately I don't think there's a way around this issue; if we had a way of programmatically determining which Instruction(s) that a new instruction should derive its DebugLoc from, then we wouldn't have as much need for debugify to begin with.

359

Minor, hadLoc -> HadLoc, although I think this could also just be inlined into the if condition.

362

As above, attacched -> attached.

djtodoro added inline comments.Sep 25 2020, 7:14 AM
llvm/lib/Transforms/Utils/Debugify.cpp
355

@StephenTozer thanks for the comment.

The metadata approach should work, although as discussed it may require changes to dropUnknownNonDebugMetadata. It might be a little messy though; I think it would also be viable to take the following approach: In the "before" step, populate a map of Instruction* Instr -> WeakVH(Instr). With this map, you can check Map[Instr] != nullptr to determine whether Instr was deleted at any point in the pass; if it wasn't deleted, then you can simply use raw pointer comparison. This avoids the need to fiddle with the internals of Instruction that could potentially affect performance or the behaviour of other code.

Interesting, but I think we'd still have the problem with the pointer reusing?

I still believe that we can use the approach from D85741, by using just simple unsigned IDs (I am making a prototype patch for it).

I think it worth adding that there will be some noise with this approach; the DebugLoc metadata for any new generated instructions will be dependent on the DebugLoc metadata for the instruction they're based on. For example, cloning an instruction , replacing a conditional branch with an unconditional one, reassociating arithmetic, etc. In each of these cases, we will get a warning from the preservation pass that a new instruction did not receive a DILocation, even though the pass that generated it had no responsibility for it. It's technically correct, but it may result in a continuously increasing amount of noise from pass-to-pass, with the worst case being that there are no DebugLocs left at the start of a pass so that we get an error for every instruction created even if the pass itself is handling DebugLocs correctly.

Actually, in the verify-each-pass-mode, it will point to the first pass that "caused" the issue with the instruction.

StephenTozer added inline comments.Sep 25 2020, 7:36 AM
llvm/lib/Transforms/Utils/Debugify.cpp
355

Interesting, but I think we'd still have the problem with the pointer reusing?

WeakVH will be nulled if the value it references is deleted; even if a new instruction is created that uses the same memory, the WeahVH object will still be equal to a nullptr.

Actually, in the verify-each-pass-mode, it will point to the first pass that "caused" the issue with the instruction.

I'm still reading the patch, so I can't be certain that I understand everything yet, but I don't think what you're describing is possible. I do understand that if we drop the DebugLoc for an instruction, then we will get an error in the pass that dropped that DebugLoc, but any subsequent passes will not show an error for that instruction because it had no DebugLoc to lose.

What I'm referring to is when we create a new instruction that should derive its DebugLoc from one or more existing instructions, but those instructions have none. For example:

We have a basic block with a conditional branch instruction with a debug location attached:

while.body:                                       ; preds = %while.body, %while.body.lr.ph
    br i1 %tobool, label %while.cond.while.cond1.preheader_crit_edge, label %while.body, !dbg !57

Some broken pass causes the debug location to be dropped; this is an error, and the DIVerifier reports it:

while.body:                                       ; preds = %while.body, %while.body.lr.ph
    br i1 %tobool, label %while.cond.while.cond1.preheader_crit_edge, label %while.body

The Jump Threading pass then determines that %tobool will always be false, and replaces the conditional branch with an unconditional branch that uses the cond branch's DebugLoc:

while.body:                                       ; preds = %while.body, %while.body.lr.ph
    br label %while.body

What should happen here? The jump threading pass has generated a new instruction with no DebugLoc attached, so DIVerifier will report it as an error. However, there is no error in the Jump Threading pass, it just has no DebugLoc to assign to the new instruction (assigning a Line 0 in this case would also be incorrect).

The more DebugLocs we lose, the more often these false errors will appear as every similar generated instruction has no way to generate a DebugLoc. I also believe there is no general method for recognizing the difference between "this new instruction has no DebugLoc because we have no way to generate one", versus "this new instruction has no DebugLoc because we didn't copy/calculate it correctly (i.e. a bug)".

djtodoro added inline comments.Sep 25 2020, 7:56 AM
llvm/lib/Transforms/Utils/Debugify.cpp
355

WeakVH will be nulled if the value it references is deleted; even if a new instruction is created that uses the same memory, the WeahVH object will still be equal to a nullptr.

Cool, I'll try it then. Thanks.

I'm still reading the patch, so I can't be certain that I understand everything yet, but I don't think what you're describing is possible. I do understand that if we drop the DebugLoc for an instruction, then we will get an error in the pass that dropped that DebugLoc, but any subsequent passes will not show an error for that instruction because it had no DebugLoc to lose.

What I'm referring to is when we create a new instruction that should derive its DebugLoc from one or more existing instructions, but those instructions have none. For example:

We have a basic block with a conditional branch instruction with a debug location attached:

while.body: ; preds = %while.body, %while.body.lr.ph

br i1 %tobool, label %while.cond.while.cond1.preheader_crit_edge, label %while.body, !dbg !57

Some broken pass causes the debug location to be dropped; this is an error, and the DIVerifier reports it:

while.body: ; preds = %while.body, %while.body.lr.ph

br i1 %tobool, label %while.cond.while.cond1.preheader_crit_edge, label %while.body

The Jump Threading pass then determines that %tobool will always be false, and replaces the conditional branch with an unconditional branch that uses the cond branch's DebugLoc:

while.body: ; preds = %while.body, %while.body.lr.ph

br label %while.body

What should happen here? The jump threading pass has generated a new instruction with no DebugLoc attached, so DIVerifier will report it as an error. However, there is no error in the Jump Threading pass, it just has no DebugLoc to assign to the new instruction (assigning a Line 0 in this case would also be incorrect).

The more DebugLocs we lose, the more often these false errors will appear as every similar generated instruction has no way to generate a DebugLoc. I also believe there is no general method for recognizing the difference between "this new instruction has no DebugLoc because we have no way to generate one", versus "this new instruction has no DebugLoc because we didn't copy/calculate it correctly (i.e. a bug)".

Oh, sorry, I haven't understood at first... Yes, but as you have said, it is not possible to catch with the utility pass like this.

I think wait for someone with more experience in this area to give approval, but aside from the outstanding comments, LGTM.

djtodoro updated this revision to Diff 294655.Sep 28 2020, 4:49 AM

-Addressing the latest comments
-Rebasing

LGTM, though recommend waiting for a more experienced reviewer to give approval.

vsk added inline comments.Sep 28 2020, 10:25 AM
llvm/docs/HowToUpdateDebugInfo.rst
230

@aprantl @djtodoro -- let's not change any names in this patch, please.

The 'opt -debugify' name has been referenced in written communication since 2017 (D40512). Changing it would be disruptive (e.g., we've already recorded a talk that references this name extensively).

If you strongly feel that the old name should be changed, it should at least happen in a separate patch. This patch is already large, and it's difficult to determine what needs review. The name change exacerbates this issue.

vsk added a comment.Sep 28 2020, 10:28 AM

I'd like to give this another pass, but the name change has made the patch too large. More inline --

djtodoro added inline comments.Sep 29 2020, 7:46 AM
llvm/docs/HowToUpdateDebugInfo.rst
230

OK, I'll return back the name, and we can defer the discussion about the name changing for later. That will make the review process easier.

djtodoro updated this revision to Diff 295518.Oct 1 2020, 3:37 AM
  • Return the names

(TODO: rebase the rest of the patches from the stack once this is finished)

aprantl added inline comments.Wed, Jan 27, 8:47 AM
llvm/test/DebugInfo/debugify-each-original-dbginfo.ll
3

This test looks like it could be very costly to maintain. If I understand correctly, it is depending on a bug in InstCombine that isn't further specified, but it's running opt with the full -O2 pipeline on all targets. What guarantees that the bug is actually triggered? Will it trigger on, e.g., an AArch64 host *and* on an x86_64 host? What if someone fixes the bug, or worse, if the pipeline changes, such that the bug isn't triggered any more — how should they update this testcase?
It would be better do do something very targeted. For example we could implement (in a unit test) a extremely simple pass that just deletes debug info intrinsics and run the check on that. This way we we will get a deterministic behavior on all targets and future compiler versions.

dblaikie added inline comments.Wed, Jan 27, 11:27 AM
llvm/test/DebugInfo/debugify-each-original-dbginfo.ll
3

Strong +1 here. Thanks for pointing this out, @aprantl.

djtodoro added inline comments.Thu, Jan 28, 7:37 AM
llvm/test/DebugInfo/debugify-each-original-dbginfo.ll
3

Thanks! Nice! I totally agree!
I'll make a unit test with the pass.

djtodoro updated this revision to Diff 320417.Mon, Feb 1, 1:55 AM
  • Rebasing on top of trunk
  • Adding a unit test with a pass that deletes debug info metadata
aprantl added inline comments.Mon, Feb 1, 5:45 PM
llvm/docs/HowToUpdateDebugInfo.rst
341

How about: In addition to automatically generating debug info, the checks provided by the debugify utility pass can also be used to test the preservation of pre-existing debug info metadata.

llvm/test/DebugInfo/debugify-original-dbginfo.ll
6

This test has the same fundamental problem: It may fail on a different target, when instcombine is updated, ...
Can you do the same unittest trick here? (perhaps with an idempotent transformation?)

djtodoro added inline comments.Tue, Feb 2, 3:21 AM
llvm/docs/HowToUpdateDebugInfo.rst
341

Looks good to me. :)

llvm/test/DebugInfo/debugify-original-dbginfo.ll
6

Sure.

This test has the same fundamental problem: It may fail on a different target, when instcombine is updated, ...

just curious -- I thought that all targets should handle Debug Info on the IR level the same way, but obviously that is not true?

djtodoro updated this revision to Diff 320731.EditedTue, Feb 2, 3:23 AM
  • Improving the unit test with positive result of debugify
aprantl added inline comments.Tue, Feb 2, 5:57 PM
llvm/test/DebugInfo/debugify-original-dbginfo.ll
6

just curious -- I thought that all targets should handle Debug Info on the IR level the same way, but obviously that is not true?

My understanding is that InstCombine has hooks for targets to apply target-specific combines that are profitable only on that architecture, but I'm far from an expert here :-)

The main problem is that this RUN line basically asserts that "instcombine behaves correctly in terms of debug info". While this may be true at the moment, at some point in the future someone will make an unrelated change to the IR, or to instcombine, or add a new target that will cause this test to break, but there is no indication in this test for how to fix it.
I think this test as it is might be okay inside the InstCombine subdirectory, to cement the fact that whatever InstCombine rules apply here behave correctly, but it's not so good as a test for the debugify pass, because it depends on the whole complex InstCombine machinery to behave in a specific way.

djtodoro added inline comments.Wed, Feb 3, 1:13 AM
llvm/test/DebugInfo/debugify-original-dbginfo.ll
6

got it, thanks for the explanation -- I've learned something new! :)

Is this ok now?

aprantl accepted this revision.Fri, Feb 12, 3:35 PM

I think you've addressed all feedback. Thanks for you patience!

This revision is now accepted and ready to land.Fri, Feb 12, 3:35 PM