Page MenuHomePhabricator

[coro] Preserve scope line for compiler generated functions
ClosedPublic

Authored by kastiglione on May 13 2021, 9:15 AM.

Details

Summary

Coro-split functions with an active suspend point have their scope line set to
the line of the suspend point. However for compiler generated functions, this
results in debug info with unconventional results: a file named
<compiler-generated> with a non-zero line number. The convention for
<compiler-generated> is that the line number is zero.

This change propagates the scope line only for non-compiler generated
functions.

Diff Detail

Event Timeline

kastiglione created this revision.May 13 2021, 9:15 AM
kastiglione requested review of this revision.May 13 2021, 9:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 13 2021, 9:15 AM
aprantl requested changes to this revision.May 14 2021, 1:01 PM

<compiler-generated> is just something the Swift frontend picks as filename but LLVM shouldn't treat it as special.

A better way to fix this is to

  • either set scopeline and filename together or leave the scopeline alone if the original scopeline is 0

and

  • fix the frontend to not emit real source locations into an artificial compiler-generated function (it puzzles me how this happens in the first place)
This revision now requires changes to proceed.May 14 2021, 1:01 PM

Change logic to be based on current scope line, not current filename.

I've updated this to follow this advice:

leave the scopeline alone if the original scopeline is 0

Adrian also suggested "set scopeline and filename together" but there I see no (existing) API for setting the filename. It could be that I've overlooked it, but I think it's better to not set the filename if reasonable.

kastiglione added inline comments.May 17 2021, 12:59 PM
llvm/lib/Transforms/Coroutines/CoroSplit.cpp
874–875

@aprantl do you think it's worth checking flags as well? I noticed in the cases where ScopeLine was zero, that flags was FlagArtificial. Should this logic check that too?

This is good, but we should add a test for this. There are already one or two tests that are affected by this, so you can probably just modify one of them to use line 0.

llvm/lib/Transforms/Coroutines/CoroSplit.cpp
874–875

Would SP->getFile() == DL->getFile() also work as a condition? That would maybe be the closest to what we really want to check. Otherwise checking for 0 sounds good to me.

kastiglione added inline comments.May 17 2021, 5:48 PM
llvm/lib/Transforms/Coroutines/CoroSplit.cpp
874–875

That condition works too.

only propagate scope line when files are the same

aprantl accepted this revision.May 21 2021, 7:19 PM

Looks good (with a testcase). It should be easy to adapt an existing testcase for this by artificially changing a suspend point's !dbg location to use a different file name.

This revision is now accepted and ready to land.May 21 2021, 7:19 PM
aprantl added inline comments.May 21 2021, 7:21 PM
llvm/lib/Transforms/Coroutines/CoroSplit.cpp
868

Instead of this sentence we could just say: ... to ensure line number and file name belong together.

kastiglione added a comment.EditedThu, May 27, 3:18 PM

following up here, looks like test/Transforms/Coroutines/coro-async.ll is the test that has the right "existing testcase" however it looks like I need to duplicate the whole thing and then tweak it. That test covers the SP->getFile() == DL->getFile() case, and if I change it by "artificially changing a suspend point's !dbg location to use a different file name", then it will instead test cover the inverted case (SP->getFile() != DL->getFile()).

Is there a better way than copying the test and modifying the one small part?

Could you attach new debug info to one of the other functions in this file? Or is there only a single .resume function being generated for the entire test?

There is a second .resume.1, thanks I hadn't noticed that, I'll see if I can use that.

Test new logic in coro-async.ll

Thanks! LGTM

thanks for your help. As you suggested, using a #line directive and viewing the IR generated by clang, led to showing how to change the file via DILexicalBlockFile.