Page MenuHomePhabricator

[Clang][Coroutine][DebugInfo] remove useless debug info for coroutine parameters
ClosedPublic

Authored by dongAxis1944 on Feb 26 2021, 12:33 AM.

Details

Summary

In c++ coroutine, clang will emit different debug info variables for parameters and move-parameters.
The first one is the real parameters of the coroutine function, the other one just for copying parameters to the coroutine frame.

Considering the following c++ code:

struct coro {
  ...
};

coro foo(struct test & t) {
  ...
  co_await suspend_always();
  ...
  co_await suspend_always();
  ...
  co_await suspend_always();
}

int main(int argc, char *argv[]) {
  auto c = foo(...);
  c.handle.resume();
  ...
}

Function foo is the standard coroutine function, and it has only one parameter named t (ignoring this at first),
when we use the llvm code to compile this function, we can get the following ir:

!2921 = distinct !DISubprogram(name: "foo", linkageName: "_ZN6Object3fooE4test", scope: !2211, file: !45, li\
ne: 48, type: !2329, scopeLine: 48, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefi\
nition | DISPFlagOptimized, unit: !44, declaration: !2328, retainedNodes: !2922)
!2924 = !DILocalVariable(name: "t", arg: 2, scope: !2921, file: !45, line: 48, type: !838) 
...
!2926 = !DILocalVariable(name: "t", scope: !2921, type: !838, flags: DIFlagArtificial)

We can find there are two the same DIVariable named t in the same dwarf scope for foo.resume.
And when we try to use llvm-dwarfdump to dump the dwarf info of this elf, we get the following output:

0x00006684:   DW_TAG_subprogram
                DW_AT_low_pc    (0x00000000004013a0)
                DW_AT_high_pc   (0x00000000004013a8)
                DW_AT_frame_base        (DW_OP_reg7 RSP)
                DW_AT_object_pointer    (0x0000669c)
                DW_AT_GNU_all_call_sites        (true)
                DW_AT_specification     (0x00005b5c "_ZN6Object3fooE4test")

0x000066a5:     DW_TAG_formal_parameter
                  DW_AT_name    ("t")
                  DW_AT_decl_file       ("/disk1/yifeng.dongyifeng/my_code/llvm/build/bin/coro-debug-1.cpp")
                  DW_AT_decl_line       (48)
                  DW_AT_type    (0x00004146 "test")

0x000066ba:     DW_TAG_variable
                  DW_AT_name    ("t")
                  DW_AT_type    (0x00004146 "test")
                  DW_AT_artificial      (true)

The elf also has two 't' in the same scope.
But unluckily, it might let the debugger confused. And failed to print parameters for O0 or above.
This patch will make coroutine parameters and move parameters use the same DIVar and try to fix the problems that I mentioned before.

Test Plan:
check-clang

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

update path for code formtting

Most folks have other duties beside the contribution here and they may have a large queue of reviews. The community convention is one ping per week.

clang/lib/CodeGen/CGCoroutine.cpp
616
631

Cpitalize. Add period.

clang/lib/CodeGen/CGDebugInfo.cpp
4372

Capitalize

clang/lib/CodeGen/CGDebugInfo.h
168

Don’t duplicate function or class name at the beginning of the comment.

clang/lib/CodeGen/CodeGenFunction.cpp
1334

Capitalize. Add period.

clang/lib/CodeGen/CodeGenFunction.h
328

https://llvm.org/docs/CodingStandards.html
"Don’t duplicate function or class name at the beginning of the comment."

clang/test/CodeGenCoroutines/coro-dwarf.cpp
2

I'll create a patch making CC1 -fno-split-dwarf-inlining default align with the driver default.

Then you can delete the option.

-triple=x86_64-unknown-linux-gnu can usually be -triple=x86_64 (generic ELF behavior) because it is not GNU specific.

54

format this line.

68

Relative line numbers may be better. Otherwise, adding or deleting source lines will lead to a pain updating tests.

https://llvm.org/docs/CommandGuide/FileCheck.html#filecheck-pseudo-numeric-variables

Most folks have other duties beside the contribution here and they may have a large queue of reviews. The community convention is one ping per week.

ok, I get it. Thanks for remaining...

aprantl added inline comments.Mar 3 2021, 1:33 PM
clang/lib/CodeGen/CGCoroutine.cpp
610

Nit: Per LLVM Coding style this should be formatted as:

// Create mapping between parameters and copy-params for coroutine function.

615

Why this->Args and not Args? Are there other Args in scope?

616

FYI: you could use one of the zip functions in llvm::Support to write this as a range-based loop.

clang/lib/CodeGen/CGDebugInfo.cpp
4394

In LLVM we prefer early exits over nested ifs.

if (!PD)
 continue;
4406

This seems redundant?

clang/lib/CodeGen/CGDebugInfo.h
171

No need to repeat the name at the beginning of the comment.

547

What does this comment mean?

550

why this->?

552

LLVM coding style elides {} when there is single statement in the loop.

559

ParamDbgMappings.insert({Var, DILocalVar});

clang/test/CodeGenCoroutines/coro-dwarf.cpp
66

Try to not hardcode the metadata numbering (!7) in the CHECK lines. It will randomly change with other LLVM changes and then the test needs to be updated.
If you need to refer to it again, write

// CHECK: ![[SP:[0-9]+]] = distinct ...
// CHECK: ..., ![[SP]], ...

Most folks have other duties beside the contribution here and they may have a large queue of reviews. The community convention is one ping per week.

On the flip side, please *do* make use of that weekly ping, because everybody is very busy and it's quite easy to loose track of open reviews.

Most folks have other duties beside the contribution here and they may have a large queue of reviews. The community convention is one ping per week.

On the flip side, please *do* make use of that weekly ping, because everybody is very busy and it's quite easy to loose track of open reviews.

Thank you for your reminding.

fix code format

dongAxis1944 marked 18 inline comments as done.Mar 4 2021, 11:39 PM
dongAxis1944 added inline comments.Mar 4 2021, 11:58 PM
clang/lib/CodeGen/CGDebugInfo.cpp
4394

Because the if-statement is not in a single loop, so the current code style might be fine I think. Any ideas?

update patch for fixing duplicate variables

ChuanqiXu added inline comments.Mar 22 2021, 12:58 AM
clang/lib/CodeGen/CGDebugInfo.cpp
4373

We can rewrite these codes into one lambda to make it more clear.

jmorse added a subscriber: jmorse.Mar 24 2021, 7:01 AM

Drive-by review: some minor nits commented on, looks very reasonable overall.

IMO, the regression test shouldn't use CHECK-NEXT everywhere: it's encoding an ordering on what metadata is emitted, which can be fragile similar to the way that hardcoded metadata numbers are fragile. Would it be sufficient to:

  • Match the DISubprogram,
  • Match with CHECK-SAME the retained-nodes field on the same line,
  • Match against each expected DILocalVariable from the retain-nodes field

That should (AFAIUI) match without relying too much on ordering, and fail to match when there's an unexpected additional DILocalVariable in the retained-nodes field. As it stands, there could be any number of additional DILocalVariables after the CHECK-NEXT lines.

clang/lib/CodeGen/CGDebugInfo.cpp
4392
4400

Full sentences with full stops in other comments too please.

4403

I assume the do-while-0 loop is used to break out of; wouldn't a simpler if condition be sufficient?:

if (!isa<llvm::DISubprogram(Scope) || !Scope->isDistinct() || D->getScope() != Scope) {
  D = nullptr;
}
4407

Opinion: a little more context in this comment would help new readers, i.e. "If we couldn't find a move param DIVariable, create a new one."

aprantl added inline comments.Mar 24 2021, 4:13 PM
clang/lib/CodeGen/CGCoroutine.cpp
610

I know I'm being pendantic, but the sentence should end with a . :-)

reflect the code, and fix some code style and comment style

dongAxis1944 marked an inline comment as done.

fix some comment styles

dongAxis1944 marked 5 inline comments as done.Mar 24 2021, 9:42 PM
dongAxis1944 added inline comments.
clang/lib/CodeGen/CGCoroutine.cpp
610

Thanks for reviewing. Please feel free, I think it is useful to fix code styles. :-)

clang/lib/CodeGen/CGDebugInfo.cpp
4392

thanks for reviewing. I just rewrite the code.

4400

Thanks, I already fix syntax errors. :-)

The test now passes without the rest of the patch applied; as mentioned above, it needs to affirmatively identify the contents of retainedNodes to guarantee detection of extra entries. Alternately, use CHECK-NOT or --implicit-check-not to confirm the absence of the extra DILocalVariables.

That being said, looking at the output of clang @ 880822255e21 on this test, it looks like the retainedNodes field isn't populated for coroutines -- all the DISubprograms have "!{}" for the field. That feels like a bug to me, as any variables completely optimised out will not have their type / declaration information represented in the DWARF output. (I assume there isn't a design change with the representation of coroutine DISubprograms that explains this).

Specifically, in DISubprograms like:

!2 = !{}
!140 = distinct !DISubprogram(name: "f_coro", linkageName: "_Z6f_coroi8MoveOnly11MoveAndCopy", scope: !8, file: !8, line: 60, type: !9, scopeLine: 60, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
!143 = !DILocalVariable(name: "mcParam", scope: !140, type: !28, flags: DIFlagArtificial)
!144 = !DILocalVariable(name: "moParam", scope: !140, type: !12, flags: DIFlagArtificial)
!145 = !DILocalVariable(name: "val", scope: !140, type: !11, flags: DIFlagArtificial)

retainedNodes should be metadata with contents !{!143, !144, !145}

dongAxis1944 marked 3 inline comments as done.Mar 31 2021, 12:05 AM

The test now passes without the rest of the patch applied; as mentioned above, it needs to affirmatively identify the contents of retainedNodes to guarantee detection of extra entries. Alternately, use CHECK-NOT or --implicit-check-not to confirm the absence of the extra DILocalVariables.

That being said, looking at the output of clang @ 880822255e21 on this test, it looks like the retainedNodes field isn't populated for coroutines -- all the DISubprograms have "!{}" for the field. That feels like a bug to me, as any variables completely optimised out will not have their type / declaration information represented in the DWARF output. (I assume there isn't a design change with the representation of coroutine DISubprograms that explains this).

Specifically, in DISubprograms like:

!2 = !{}
!140 = distinct !DISubprogram(name: "f_coro", linkageName: "_Z6f_coroi8MoveOnly11MoveAndCopy", scope: !8, file: !8, line: 60, type: !9, scopeLine: 60, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
!143 = !DILocalVariable(name: "mcParam", scope: !140, type: !28, flags: DIFlagArtificial)
!144 = !DILocalVariable(name: "moParam", scope: !140, type: !12, flags: DIFlagArtificial)
!145 = !DILocalVariable(name: "val", scope: !140, type: !11, flags: DIFlagArtificial)

retainedNodes should be metadata with contents !{!143, !144, !145}

Thanks, I will update the patch later.

Use CHECK-NOT to confirm the absence of the extra DILocalVariables.

The test now passes without the rest of the patch applied; as mentioned above, it needs to affirmatively identify the contents of retainedNodes to guarantee detection of extra entries. Alternately, use CHECK-NOT or --implicit-check-not to confirm the absence of the extra DILocalVariables.

That being said, looking at the output of clang @ 880822255e21 on this test, it looks like the retainedNodes field isn't populated for coroutines -- all the DISubprograms have "!{}" for the field. That feels like a bug to me, as any variables completely optimised out will not have their type / declaration information represented in the DWARF output. (I assume there isn't a design change with the representation of coroutine DISubprograms that explains this).

Specifically, in DISubprograms like:

!2 = !{}
!140 = distinct !DISubprogram(name: "f_coro", linkageName: "_Z6f_coroi8MoveOnly11MoveAndCopy", scope: !8, file: !8, line: 60, type: !9, scopeLine: 60, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
!143 = !DILocalVariable(name: "mcParam", scope: !140, type: !28, flags: DIFlagArtificial)
!144 = !DILocalVariable(name: "moParam", scope: !140, type: !12, flags: DIFlagArtificial)
!145 = !DILocalVariable(name: "val", scope: !140, type: !11, flags: DIFlagArtificial)

retainedNodes should be metadata with contents !{!143, !144, !145}

As you said, I rerun the test of this patch, it seems the retainedNodes are always empty.
Is this unit test wrong?

Thanks in advance.

aprantl added inline comments.Apr 8 2021, 9:57 AM
clang/lib/CodeGen/CGDebugInfo.cpp
4374

"we would love to" makes it sound like a FIXME, but I think you mean that we are actually going to do this in the following block?

4379

Should we call this RemapCoroArgToLocalVar or something more descriptive?

dongAxis1944 marked an inline comment as done.

update common and fix code style as suggested

dongAxis1944 marked 3 inline comments as done.Apr 9 2021, 12:00 AM
dongAxis1944 added inline comments.
clang/lib/CodeGen/CGDebugInfo.cpp
4374

Thanks, I will use the "we are going to" instead of "we would love to"

4379

RemapCoroArgToLocalVar sounds great, thanks

4407

Thanks, I already add comments for this code as you suggested.

dongAxis1944 edited the summary of this revision. (Show Details)Apr 9 2021, 12:16 AM
dongAxis1944 marked 3 inline comments as done.

fix compile error

use isa_and_nonnull instead of isa....

aprantl accepted this revision.Apr 9 2021, 12:55 PM
This revision is now accepted and ready to land.Apr 9 2021, 12:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 11 2021, 8:14 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

NB, while I flagged that CHECK-NEXT added too much ordering, I forgot that CHECK still requires some ordering. It causes us some difficulties downstream; there's a follow-up patch here D100298 for consideration.