This is an archive of the discontinued LLVM Phabricator instance.

[Coro][Debuginfo] Add debug info to `_NoopCoro_ResumeDestroy` function
ClosedPublic

Authored by avogelsgesang on Aug 24 2022, 11:11 AM.

Details

Summary

With this commit, we now attach an DISubprogram to the LLVM-generated
_NoopCoro_ResumeDestroy function. Thereby, lldb can show a
std::coroutine_handle to a std::noop_coroutine as

continuation = coro frame = 0x555555560d98 {
  resume = 0x0000555555555c50 (a.out`_NoopCoro_ResumeDestroy)
  destroy = 0x0000555555555c50 (a.out`_NoopCoro_ResumeDestroy)
}

instead of

continuation = coro frame = 0x555555560d98 {
    resume = 0x0000555555555c50 (a.out`___lldb_unnamed_symbol211)
    destroy = 0x0000555555555c50 (a.out`___lldb_unnamed_symbol211)
}

I renamed the function from NoopCoro.ResumeDestroy to
_NoopCoro_ResumeDestroy because:

  • the leading _ makes sure this is a reserved name and should not clash with any user-provided names
  • the . was replaced by a _, so the name is now a valid identifier in C, making it allows me to type its name in the debugger

Diff Detail

Event Timeline

avogelsgesang created this revision.Aug 24 2022, 11:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 24 2022, 11:11 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
avogelsgesang requested review of this revision.Aug 24 2022, 11:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 24 2022, 11:11 AM
avogelsgesang edited the summary of this revision. (Show Details)

pacify clang-format

aprantl added inline comments.Aug 24 2022, 3:29 PM
llvm/lib/Transforms/Coroutines/CoroEarly.cpp
125

nit: . at the end

129

Is there a specific reason why you want to create a new CU here? I would just use the one from the function we're currently processing and mark the new DISubprogram as artificial.

jryans added a subscriber: jryans.Aug 24 2022, 3:41 PM

Hmm, the "X instead of Y" example in the change summary seems to have the same output on both sides... Maybe one side was lost when preparing the summary? (Apologies if there is a difference I'm not seeing... 😅)

avogelsgesang added inline comments.Aug 24 2022, 3:49 PM
llvm/lib/Transforms/Coroutines/CoroEarly.cpp
129

not really, it just felt the most natural to me. The user didn't write that function anywhere, hence I thought: the file name should read "<llvm-instrinsics>". Also, it seems there can be multiple compilation units attached to a single LLVM module, and I didn't know how to determine the one to which I should add this function

avogelsgesang edited the summary of this revision. (Show Details)Aug 24 2022, 4:00 PM

Hmm, the "X instead of Y" example in the change summary seems to have the same output on both sides...

good catch! Fixed. I forgot to recompile the old version of clang before creating the "before" snippet...

avogelsgesang marked an inline comment as done.

fixed comments

llvm/test/Transforms/Coroutines/coro-noop.ll
1

update comment...

ChuanqiXu added inline comments.Aug 24 2022, 7:02 PM
llvm/lib/Transforms/Coroutines/CoroEarly.cpp
129

It looks better to split these logic to another smaller function. And we should generate debug information only if the debug option -g enabled. I think we can look at the start of buildFrameDebugInfo to get this. Then we can reuse the CU.

avogelsgesang added inline comments.Aug 25 2022, 2:47 AM
llvm/lib/Transforms/Coroutines/CoroEarly.cpp
129

buildFrameDebugInfo looks at the current function to figure out if debug information should be created. This works nicely for coroutine splitting. For lowerCoroNoop, I don't think this is a good idea. The generated _NoopCoro_ResumeDestroy is shared across potentially many functions, some of which could have debug info, some of which won't

aprantl added inline comments.Aug 25 2022, 9:56 AM
llvm/lib/Transforms/Coroutines/CoroEarly.cpp
129

The user didn't write that function anywhere

The artificial flag on the subprogram and line 0 as a source location already signal that. But it should still part of the same translation/compile unit. That's how all other compiler-generated stubs / thunks / etc are also represented.

reuse compile unit instead of creating a new one

avogelsgesang marked 3 inline comments as done.Aug 25 2022, 4:21 PM
avogelsgesang added inline comments.
llvm/lib/Transforms/Coroutines/CoroEarly.cpp
129

I am reusing the existing CU now.

we should generate debug information only if the debug option -g enabled

If no compilation unit debug info exists, we now don't add debug info for _NoopCoro_ResumeDestroy, either

ChuanqiXu added inline comments.Aug 26 2022, 12:01 AM
llvm/lib/Transforms/Coroutines/CoroEarly.cpp
120

The double underscores look more like reserved.

129

I mean extract the if into a static function buildDebugInfoForNoopCoroutine or something else. I feel it would be better to read. The current one is slightly hard to read. Since the debug information may be noisy for people who want to visit how llvm handles coroutines.

avogelsgesang marked 2 inline comments as done.

move into separate function; use double underscore

Looks like there are many unrelated changes, it'd better to remove them.

llvm/lib/Transforms/Coroutines/CoroEarly.cpp
41

Unrelated change.

70–71

Unrelated change.

I guess you're using clang-format in a different way? Here is my way to do clang-format:

git diff -U0 --no-color --relative HEAD^ | clang/tools/clang-format/clang-format-diff.py -p1 -I

And set the clang-format at the trunk as the default clang-format.

106

We prefer short indentation.

114

Why do we set File as nullptr here? I feel like any DIFile is better here.

158–163

Unrelated changes. If we want to do such changes, it'd better to do it in a NFC patch.

llvm/test/Transforms/Coroutines/coro-noop.ll
25

I guess we need to update the test.

avogelsgesang marked 5 inline comments as done.
  • update test case
  • use <llvm-intrinsics> instead of nullptr DIFile
  • remove unrelated changes
avogelsgesang marked 2 inline comments as done.Aug 26 2022, 1:27 AM
ChuanqiXu added inline comments.Aug 26 2022, 1:36 AM
llvm/lib/Transforms/Coroutines/CoroEarly.cpp
111

How about using existing DIFile?

146–151

Looks like unrelated change too.

avogelsgesang marked an inline comment as done.
  • use existing DIFile
  • remove more unrelated changes
avogelsgesang marked an inline comment as done.Aug 26 2022, 2:04 AM
This revision is now accepted and ready to land.Aug 26 2022, 2:07 AM
This revision was landed with ongoing or failed builds.Aug 26 2022, 5:50 AM
This revision was automatically updated to reflect the committed changes.

merged to main despite test failures because those test failures were only timeouts and I don't see how those could be related to this change at hand here