Re: [PATCH] D33705: [CGVTables] Finalize SP before attempting to clone it
For some reason your comments aren't showing up on Phabricator, so replying via email - hope everybody sees it. If SPs are now all finalized early - I'm assuming there's some other code > that finalizes SPs that's no longer needed? > Not all of them, just the one before cloning. I wanted to make the minimal change possible here, since this already seems like a bit of a hack to work around the fact that we can't properly forward variadic function calls at the IR level. > Also - this seems like a layering/separation issue - does other code call > into the DI as casually/explicitly as this code being added to CGVTables? > (Or should the callback go through some more generic entry point in > CGDebugInfo to delegate/handle that?) & is there nothing in CGDebugInfo > that is already called back in a timely fashion to handle this? I'm > guessing what's needed is a "IR has finished being generated for this > function, opt/codegen is about to start"? That seems like a good generic > callback & probably shouldn't be coming from something called CGVTables? > (presuming there's a more general place to put that) > I suppose there's CGDebugInfo::EmitFunctionEnd, so if we did want to finalize every SP as soon as we're done with it, we could put an explicit call to that extent there. Frankly though I don't really know the clang code too well, I just want to unbreak LLVM ;). ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D33705: [CGVTables] Finalize SP before attempting to clone it
I'll let Adrian weigh in - but I suspect, if possible, it'd be better to just finalize them all once IRGen for the function finishes. On Tue, May 30, 2017 at 6:02 PM Keno Fischer wrote: > For some reason your comments aren't showing up on Phabricator, so > replying via email - hope everybody sees it. > > If SPs are now all finalized early - I'm assuming there's some other code >> that finalizes SPs that's no longer needed? >> > > Not all of them, just the one before cloning. I wanted to make the minimal > change possible here, since this already seems like a bit of a hack to work > around the fact that we can't properly forward variadic function calls at > the IR level. > > >> Also - this seems like a layering/separation issue - does other code call >> into the DI as casually/explicitly as this code being added to CGVTables? >> (Or should the callback go through some more generic entry point in >> CGDebugInfo to delegate/handle that?) & is there nothing in CGDebugInfo >> that is already called back in a timely fashion to handle this? I'm >> guessing what's needed is a "IR has finished being generated for this >> function, opt/codegen is about to start"? That seems like a good generic >> callback & probably shouldn't be coming from something called CGVTables? >> (presuming there's a more general place to put that) >> > > I suppose there's CGDebugInfo::EmitFunctionEnd, so if we did want to > finalize every SP as soon as we're done with it, we could put an explicit > call to that extent there. Frankly though I don't really know the clang > code too well, I just want to unbreak LLVM ;). > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D33705: [CGVTables] Finalize SP before attempting to clone it
If SPs are now all finalized early - I'm assuming there's some other code
that finalizes SPs that's no longer needed?
Also - this seems like a layering/separation issue - does other code call
into the DI as casually/explicitly as this code being added to CGVTables?
(Or should the callback go through some more generic entry point in
CGDebugInfo to delegate/handle that?) & is there nothing in CGDebugInfo
that is already called back in a timely fashion to handle this? I'm
guessing what's needed is a "IR has finished being generated for this
function, opt/codegen is about to start"? That seems like a good generic
callback & probably shouldn't be coming from something called CGVTables?
(presuming there's a more general place to put that)
On Tue, May 30, 2017 at 5:30 PM Keno Fischer via Phabricator <
revi...@reviews.llvm.org> wrote:
> loladiro created this revision.
>
> LLVM commit https://reviews.llvm.org/D33655 was reverted, because it
> exposed an assertion failure,
> in `GenerateVarArgsThunk`. That function attempts to clone a function
> before clang is entirely done generating the debug info for the current
> compliation unit. That is not a valid use of the API, because it expects
> that there are no temporary MD nodes in the nodes that it needs to clone.
> However, until now, this did not cause any problems (though could have
> caused assertions in the backend due to mismatched debug info metadata).
>
> Attempt to rectify this by finalizing the SP before attempting to clone it.
> The requisite LLVM API is being introduced in
> https://reviews.llvm.org/D33704.
>
>
> https://reviews.llvm.org/D33705
>
> Files:
> lib/CodeGen/CGDebugInfo.h
> lib/CodeGen/CGVTables.cpp
>
>
> Index: lib/CodeGen/CGVTables.cpp
> ===
> --- lib/CodeGen/CGVTables.cpp
> +++ lib/CodeGen/CGVTables.cpp
> @@ -152,6 +152,12 @@
>llvm::Value *Callee = CGM.GetAddrOfFunction(GD, Ty, /*ForVTable=*/true);
>llvm::Function *BaseFn = cast(Callee);
>
> + // We may not clone a subprogram that contains temporary metadata, so
> finalize
> + // it now - we will not be modifying it.
> + if (CGDebugInfo *DI = CGM.getModuleDebugInfo())
> +if (llvm::DISubprogram *SP = BaseFn->getSubprogram())
> + DI->finalizeSP(SP);
> +
>// Clone to thunk.
>llvm::ValueToValueMapTy VMap;
>llvm::Function *NewFn = llvm::CloneFunction(BaseFn, VMap);
> Index: lib/CodeGen/CGDebugInfo.h
> ===
> --- lib/CodeGen/CGDebugInfo.h
> +++ lib/CodeGen/CGDebugInfo.h
> @@ -308,6 +308,7 @@
>~CGDebugInfo();
>
>void finalize();
> + void finalizeSP(llvm::DISubprogram *SP) { DBuilder.finalizeSP(SP); }
>
>/// Module debugging: Support for building PCMs.
>/// @{
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
