Page MenuHomePhabricator

[IR] Add DISuprogram and DIE for func decl of an external
ClosedPublic

Authored by djtodoro on Apr 15 2019, 8:35 AM.

Details

Summary

In order to have call_origin to an external function we need DISubprogram for the function.

Authors: @asowda, @NikolaPrica, @djtodoro, @ivanbaev

Diff Detail

Event Timeline

djtodoro updated this revision to Diff 201507.May 27 2019, 5:13 AM

-Rebase
-Improve the verifier
-Add a test
-Document the spFlag
-Refactor the code

Can you explain or (link to a review that explains) why this flag is necessary? What are we avoiding/gaining by having it?

@aprantl Thanks again for your comments!

The patch is needed to describe a call site to an external function. Currently, we do not have all necessary info for the debug call site (particular debug subprogram is missing for the external function). Since we can describe call site parameters for calls like that, we are producing additional subprogram for the declaration of the extern function by marking it with the DISPFlagDeclForCallSite flag.
Basically, this is improvement of existing call site debug info production.

Please find the frontend part: D60714

The patch is needed to describe a call site to an external function. Currently, we do not have all necessary info for the debug call site (particular debug subprogram is missing for the external function). Since we can describe call site parameters for calls like that, we are producing additional subprogram for the declaration of the extern function by marking it with the DISPFlagDeclForCallSite flag.

I'm sorry if the answer to that question is already in your reply and I just failed to parse it: Why do we need the DISPFlagDeclForCallSite *flag* to differentiate the a DISubprogram. Isn't the fact that it is attached to a forward-declaration enough?

Secondly: since this is attached to a forward *declaration*, would it make sense to attach the unique (= not distinct) DISubprogram from the type hierarchy instead of creating another distinct node? What is the intended behavior when two LLVM modules containing the same forward declaration are llvm-linked during LTO?

djtodoro updated this revision to Diff 202174.May 30 2019, 6:30 AM

-Refactor the code
-Change the test case

I'm sorry if the answer to that question is already in your reply and I just failed to parse it: Why do we need the DISPFlagDeclForCallSite *flag* to differentiate the a DISubprogram. Isn't the fact that it is attached to a forward-declaration enough?

We could avoid the flag, but since existing LLVM infrastructure does not allow debug info for function declarations itself, we wanted to emphasize that we are creating it only for the purpose of call site’s debug info. The verifier to distinguish that the declaration will be used in that purpose uses the flag.

Secondly: since this is attached to a forward *declaration*, would it make sense to attach the unique (= not distinct) DISubprogram from the type hierarchy instead of creating another distinct node?

Oh, the test case from this patch was artificial one. We are actually creating it that way. There is no distinct subprograms for the declarations. Please see the test case updated.

What is the intended behavior when two LLVM modules containing the same forward declaration are llvm-linked during LTO?

LLVM linked file will have just one debug info subprogram instance for the declaration.

Prior to this patch Function declarations may not have a !dbg attachment, because there was no use-case for it. With call site debug info, I think it would make sense to revisit this rule. When I made my previous comment, I thought that the updated rule could be:

(1) function definitions may have a distinct DISubprogram attached (as before)
(2) function declarations may have a unique DISubprogram attached (new)

If I read the code in Verifier.cpp correctly, then it is legal for a unique DISubprogram to not have a unit, which would cause the declaration-DISubprograms to be uniqued during LTO.

so the (2) should probably be:

(2) function declarations may have a unique DISubprogram attached and it may not have a unit: field

I've CC'ed Duncan in case he can catch a logic error with this reasoning.

Prior to this patch Function declarations may not have a !dbg attachment, because there was no use-case for it. With call site debug info, I think it would make sense to revisit this rule. When I made my previous comment, I thought that the updated rule could be:

(1) function definitions may have a distinct DISubprogram attached (as before)
(2) function declarations may have a unique DISubprogram attached (new)

If I read the code in Verifier.cpp correctly, then it is legal for a unique DISubprogram to not have a unit, which would cause the declaration-DISubprograms to be uniqued during LTO.

so the (2) should probably be:

(2) function declarations may have a unique DISubprogram attached and it may not have a unit: field

I've CC'ed Duncan in case he can catch a logic error with this reasoning.

That rule seems reasonable. The critical piece is that declarations will show up in ~every TU, and to be uniqued properly they can't point at anything identifying which TU they're coming from.

djtodoro updated this revision to Diff 202417.May 31 2019, 5:07 AM

-Remove the SP flag
-Attach a unique sp to a declaration for the purpose of call site dbg info

@aprantl , @dexonsmith Thanks for your comments!

Now, we avoid the sp flag and a unique sp is attached on to a declaration needed for call site debug info.
Please see the following example.


For the test1.c:

` ...

extern int fn1 (int a);

int fn2(int arg) {
  ...
  arg = fn1(s);
  printf("\n");

...`

LLVM IR will look like:

...
!0 = distinct !DICompileUnit(...,retainedTypes: !3)
!1 = !DIFile(filename: "test1.c", directory: "/dir")
...
!3 = !{!4, !8}
!4 = !DISubprogram(name: "fn1", scope: !1, file: !1, line: 3, type: !5, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized, retainedNodes: !2)
...
!8 = !DISubprogram(name: "printf", scope: !9, file: !9, line: 361, type: !10, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized, retainedNodes: !2)
!9 = !DIFile(filename: "path_to/include/stdio.h", directory: "")
...


For the test2.c:

` ...

extern int fn1 (int a);

int fn3(int arg3) {
  ...
  arg3 = fn1(k);
  printf("\n");

...`

LLVM IR will look like:

...
!0 = distinct !DICompileUnit(...,retainedTypes: !3)
!1 = !DIFile(filename: "test2.c", directory: "/dir")
...
!3 = !{!4, !8}
!4 = !DISubprogram(name: "fn1", scope: !1, file: !1, line: 3, type: !5, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized, retainedNodes: !2)
...
!8 = !DISubprogram(name: "printf", scope: !9, file: !9, line: 361, type: !10, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized, retainedNodes: !2)
!9 = !DIFile(filename: "path_to/include/stdio.h", directory: "")
...


By linking the llvm files together we will have:

...
!0 = distinct !DICompileUnit(...,retainedTypes: !3)
!1 = !DIFile(filename: "test1.c", directory: "/dir")
...
!3 = !{!4, !8}
!4 = !DISubprogram(name: "fn1", scope: !1, file: !1, line: 3, type: !5, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized, retainedNodes: !2)
...
!8 = !DISubprogram(name: "printf", scope: !9, file: !9, line: 361, type: !10, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized, retainedNodes: !2)
!9 = !DIFile(filename: "path_to/include/stdio.h", directory: "")
...
!15 = distinct !DICompileUnit(..., retainedTypes: !17)
!16 = !DIFile(filename: "test2.c", directory: "/dir")
!17 = !{!18, !8}
!18 = !DISubprogram(name: "fn1", scope: !16, file: !16, line: 3, type: !5, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized, retainedNodes: !2)
...

(2) function declarations may have a unique DISubprogram attached (new)

In addition, this will address only this.

aprantl accepted this revision.May 31 2019, 9:27 AM

Thanks!

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
672

Create DIEs for function declarations used for call site debug info.

lib/IR/Verifier.cpp
2228

LGTM!

This revision is now accepted and ready to land.May 31 2019, 9:27 AM
aprantl added inline comments.May 31 2019, 9:28 AM
lib/IR/Verifier.cpp
2228

Perhaps add a comment here:
// This is used for call site debug information.

Oh.. and could you please document this either in LangRef.rst or SourceLevelDebugging.rst?

djtodoro updated this revision to Diff 202892.Jun 4 2019, 3:31 AM

-Address suggestions
-Document this

Oh.. and could you please document this either in LangRef.rst or SourceLevelDebugging.rst?

Sure. Please find it in the latest patch.
Thanks for your comments!

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 26 2019, 11:09 PM
Herald added a subscriber: hiraditya. · View Herald Transcript