This is an archive of the discontinued LLVM Phabricator instance.

CodeGen: Fix missing debug loc due to alloca
ClosedPublic

Authored by yaxunl on Oct 18 2017, 2:09 PM.

Details

Summary

Builder save/restores insertion pointer when emitting addr space cast
for alloca, but does not save/restore debug loc, which causes verifier
failure for certain call instructions.

This patch fixes that.

Diff Detail

Event Timeline

yaxunl created this revision.Oct 18 2017, 2:09 PM
rjmccall edited edge metadata.Oct 23 2017, 1:07 AM

If this is something we generally need to be doing in all the places we temporarily save and restore the insertion point, we should fix the basic behavior of saveIP instead of adding explicit code to a bunch of separate places. Can we just override saveIP() on CGBuilder to return a struct that also includes the current debug location?

If this is something we generally need to be doing in all the places we temporarily save and restore the insertion point, we should fix the basic behavior of saveIP instead of adding explicit code to a bunch of separate places. Can we just override saveIP() on CGBuilder to return a struct that also includes the current debug location?

IRBuilderBase::InsertPointGuard does that. Will use it instead.

yaxunl updated this revision to Diff 119918.Oct 23 2017, 12:38 PM

Use InsertPointGuard and simplify test.

The code looks fine to me. The test seems very vague to me, but I'd like Dave to weigh in on that, because I'm not sure it's reasonable to test the exact pattern here.

Also, Dave, I don't know what the IR invariants around debug info are. Is this something we should be updating all of our uses of saveIP() to do if there's a possibility of emitting arbitrary code while the IP is saved?

Anytime the code between saveIP() and restoreIP() could set the current debug location, it needs to be saved/restored along with the insertion point. I have to say the problem is not obvious to me here, so maybe saveIP/restoreIP should be changed (or eliminated in favor of always using InsertPointGuard). I'm not seeing a lot of places where saveIP/restoreIP are used.

The test looks like all it's doing is verifying both calls have a debug location at all. It could verify that both calls have the _same_ debug location, which I would find much more robust and convincing.

Anytime the code between saveIP() and restoreIP() could set the current debug location, it needs to be saved/restored along with the insertion point. I have to say the problem is not obvious to me here, so maybe saveIP/restoreIP should be changed (or eliminated in favor of always using InsertPointGuard). I'm not seeing a lot of places where saveIP/restoreIP are used.

Yeah, not too many. I'm not seeing any real reason not to just switch Clang over to always use InsertPointGuard. (Yaxun, you don't need to do this; I'm just making a note to myself.)

John.

yaxunl updated this revision to Diff 120076.Oct 24 2017, 8:01 AM

Revised the test by Paul's comments.

rjmccall accepted this revision.Oct 24 2017, 10:42 AM

Okay, LGTM.

This revision is now accepted and ready to land.Oct 24 2017, 10:42 AM
This revision was automatically updated to reflect the committed changes.