Page MenuHomePhabricator

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

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

Details

Diff Detail

Repository
rL LLVM

Event Timeline

aprantl added inline comments.Apr 15 2019, 8:43 PM
lib/CodeGen/CGDebugInfo.cpp
3596 ↗(On Diff #195194)

Does that mean that there is another call to setSubprogram elsewhere that becomes redundant now?

lib/CodeGen/CGExpr.cpp
4798 ↗(On Diff #195194)

From a layering perspective, all logic that knows about DWARF should really live in CGDebugInfo and there should be a higher-level and much simpler callback here.

Also I wonder: Is this the only place where calls are emitted?
What about ObjC message sends, for example?

djtodoro marked 2 inline comments as done.Apr 17 2019, 3:06 AM
djtodoro added inline comments.
lib/CodeGen/CGDebugInfo.cpp
3596 ↗(On Diff #195194)

There is another call to setSubprogram whenever start of a function gets emitted (in CGDebugInfo::EmitFunctionStart).

In general, extern functions don't have function definition in a current compile unit, so in those situations setSubprogram should be called just at this place. But, we found that in some situations, even if a function is extern, function definition of the function gets created, so in that case we would firstly set SP at this place and than replace it in CGDebugInfo::EmitFunctionStart.

lib/CodeGen/CGExpr.cpp
4798 ↗(On Diff #195194)

We only tested this functionality (debug user experience) within GDB (by using GNU extensions).
Since we are adding DWARF 5 (and LLDB will use call_site_parameters for printing @entry) this definitely should be extended for it.

Also I wonder: Is this the only place where calls are emitted?

I think so.

What about ObjC message sends, for example?

We have not yet tested this for ObjC. We should at some point, but I think this all functionality should work for ObjC as well?

djtodoro updated this revision to Diff 195529.Apr 17 2019, 3:13 AM

-Format
-Support DWARF 5 as well

Thanks for all your responses. I think the only thing left to do here is to factor out all the CGDebugInfo-related code into a CGDebugInfo::handleFunctionCall() (or similarly named) method, so all the debug info code is contained in CGDebugInfo.cpp.

@aprantl Sure. Thanks for your comments!

djtodoro updated this revision to Diff 196208.Apr 23 2019, 4:37 AM
djtodoro added a reviewer: petarj.

-Move debug info related code into CGDebugInfo::HandleFunctionCall()
-Add test case

djtodoro added a subscriber: petarj.

(I apologize if I keep asking the same questions over and over again.)
If I understand correctly, this causes clang to emit a DISubprogram with SPFlagDeclForCallSite set for each call site.

  • This is needed because DW_AT_call_parameter needs to link to the DW_TAG_formal_parameter DIE in the called DW_TAG_subprogram. Correct?
  • Why is that flag not being tested for in the test case?
  • What happens when we emit a function body *after* we emitted a DISubprogram+ SPFlagDeclForCallSite for the same function? Do we get two DISubprograms, or one, and does it have the SPFlagDeclForCallSite flag set?
lib/CodeGen/CGDebugInfo.h
412 ↗(On Diff #196208)

All the other methods are called "Emit"-something.

djtodoro marked an inline comment as done.Apr 25 2019, 7:57 AM

@aprantl Since this is a very big feature, we want to clarify everything as well, so everything is OK. :)

Actually, I think that we have a small misunderstanding about this part. SPFlagDeclForCallSite is used just to mark a DISubprogram as declaration for call site. It will be emitted only in the case if subprogram of a function is not there at the moment (if (!Func->getSubprogram()). So, it is the case only for extern functions. We are doing this in order to avoid ‘DW_TAG_call_site’ with no ‘call_reference’ attribute set. GCC does the same.

This part improves existing call site debug info in LLVM.

lib/CodeGen/CGDebugInfo.h
412 ↗(On Diff #196208)

What about EmitFuncDeclForCallSite()?

Actually, I think that we have a small misunderstanding about this part. SPFlagDeclForCallSite is used just to mark a DISubprogram as declaration for call site. It will be emitted only in the case if subprogram of a function is not there at the moment (if (!Func->getSubprogram()). So, it is the case only for extern functions.

That's what wasn't quite clear to me: When the call site of a (internal or linkonce-odr) function is visited before the function bode is code-generated, what happens? Do we end up with two DISubprograms, one with a forward decl and one with the real definition? Will we end up with a DISubprogram with a definition *and* an SPFlagDeclForCallSite?

We are doing this in order to avoid ‘DW_TAG_call_site’ with no ‘call_reference’ attribute set. GCC does the same.

Are you saying that without the extra forward declarations emitted by this patch, we would generate DW_TAG_call_site entries that don't have a DW_AT_call_origin attribute? Or something else?

This part improves existing call site debug info in LLVM.

IIUC, that makes sense to me.

That's what wasn't quite clear to me: When the call site of a (internal or linkonce-odr) function is visited before the function bode is code-generated, what happens? Do we end up with two DISubprograms, one with a forward decl and one with the real definition? Will we end up with a DISubprogram with a definition *and* an SPFlagDeclForCallSite?

At this place functions are already visited, so every function with its body has DISubprogram set up. But, if somewhere out there is the example like that, we will have two DISubprograms generated.

Are you saying that without the extra forward declarations emitted by this patch, we would generate DW_TAG_call_site entries that don't have a DW_AT_call_origin attribute? Or something else?

Yes, you understand it right. We see that it is missing in some cases by using the latest version of LLVM.

That's what wasn't quite clear to me: When the call site of a (internal or linkonce-odr) function is visited before the function bode is code-generated, what happens? Do we end up with two DISubprograms, one with a forward decl and one with the real definition? Will we end up with a DISubprogram with a definition *and* an SPFlagDeclForCallSite?

At this place functions are already visited, so every function with its body has DISubprogram set up. But, if somewhere out there is the example like that, we will have two DISubprograms generated.

Are you saying that without the extra forward declarations emitted by this patch, we would generate DW_TAG_call_site entries that don't have a DW_AT_call_origin attribute? Or something else?

Yes, you understand it right. We see that it is missing in some cases by using the latest version of LLVM.

Good :-)
IIUC, the Verifier should reject any DIExpressions containing DW_OP_entry_values, because it doesn't know about DW_OP_reg operations. That means you are not calling the verifier on any of the expressions generated by LiveDebugValues. Could we add an assert(DIExpr->isValid()) somewhere there? Or alternatively shouldn't the machine verifier also call DIExpression::isValid on all DBG_VALUEs? The latter would be even better.

Sorry! I commented on the wrong review...

djtodoro updated this revision to Diff 199215.May 13 2019, 1:38 AM

-Rename the method for emitting a declaration SP for a call site
-Rebase

aprantl added inline comments.May 13 2019, 10:19 AM
lib/CodeGen/CGDebugInfo.cpp
3611 ↗(On Diff #199215)

Perhaps add a comment explaining what (and why) is being done in this function?

3611 ↗(On Diff #199215)

move this into the early-exit condition above?

3612 ↗(On Diff #199215)

ditto

3613 ↗(On Diff #199215)

what can this be instead of a FuncDecl? Just asking whether the dyn_cast is necessary.

djtodoro updated this revision to Diff 199789.May 16 2019, 4:58 AM
djtodoro marked 4 inline comments as done.

-Address suggestions

aprantl accepted this revision.May 16 2019, 3:23 PM

LGTM with three nitpicks inside.

lib/CodeGen/CGDebugInfo.cpp
3589 ↗(On Diff #199789)
llvm::DISubroutineType *SubroutineType = nullptr;
if (!IsDeclForCallSite)
  SubroutineType = getOrCreateFunctionType(D, FnType, Unit);

Can you explain (in a comment) why? Is this not necessary, not safe, not possible?

lib/CodeGen/CGDebugInfo.h
407 ↗(On Diff #199789)

Please add a comment explaining when the last parameter is supposed to be set.

This revision is now accepted and ready to land.May 16 2019, 3:23 PM
djtodoro marked 2 inline comments as done.May 21 2019, 6:48 AM
djtodoro added inline comments.
lib/CodeGen/CGDebugInfo.cpp
3589 ↗(On Diff #199789)

Actually, it is possible to create the subroutine type even in the case of declaration for call site, but we need a correct callee FnType. I will update this patch with the small fix.
Thanks again for your comments!

lib/CodeGen/CGDebugInfo.h
407 ↗(On Diff #199789)

Sure. We will add that.

djtodoro updated this revision to Diff 200954.May 23 2019, 6:19 AM

-Address suggestions
-Use the right FnType needed for production of the subroutine type

djtodoro updated this revision to Diff 203337.Jun 6 2019, 6:00 AM

-Handle the TargetDecl in more secure way (the same as the other code in the function)

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