[PATCH] D79967: Fix debug info for NoDebug attr

[David Blaikie via Phabricator via cfe-commits]

dblaikie added subscribers: djtodoro, dexonsmith.
dblaikie added a comment.

In D79967#2041595 <https://reviews.llvm.org/D79967#2041595>, @yaxunl wrote:

> In D79967#2039153 <https://reviews.llvm.org/D79967#2039153>, @dblaikie wrote:
>
> > Could you check the commit history for this feature and rope in some folks who added the function declaration work (it's for debug call sites) - maybe @vsk is the right person, or knows who is, to check this is the right fix for it/doesn't adversely affect the feature this code was added to implement.
>
>
> Anders Carlsson introduced support of nodebug attribute by the following two commits:
>
> https://github.com/llvm/llvm-project/commit/76187b4d689a6ce601f2055b3dad5be6a4ab1012
>  https://github.com/llvm/llvm-project/commit/63784f4e5e125b7a81c92c2cf176797ce67b2e6d
>
> However, it seems he is not in Phabricator.
>
> Based on documentation of nodebug attribute:
>
> https://clang.llvm.org/docs/AttributeReference.html#nodebug
>
> clang should not emit any debug information for functions with nodebug attr.


Oh, sorry - I didn't mean the nodebug attribute feature I meant the "emitting function declarations" feature - looks like @vsk and @djtodoro have worked on that support & I'd like them to chime in on this.

> When control flow goes to
> 
> https://github.com/llvm/llvm-project/blob/5b0502dff5b6f510fbf823059faa042dd1523cef/llvm/lib/CodeGen/LexicalScopes.cpp#L53
> 
> Since getUnit() is nullptr, clang crashes.

Hmm, this sort of seems like it might lead to other bugs - @vsk wouldn't this assert/fail under LTO too? (we might emit a DISubprogram declaration for a function in one translation unit, but if another translation unit that defines that function is emitted without debug info - would the DISubprogram declaration (with no unit field) get attached to the definition and lead to this sort of crash?)? hmm, nope, because these things are never attached to definitions usually. So then a side question becomes "how did this end up attached to a definition?" (I guess "because there was a definition")

@vsk - we might need to have some broader design discussion. I see that these function declarations end up in the CU's retainedTypes field (I might've been involved in the discussion that got them there? I'm not sure - either way, in retrospect/now that I see it, I have concerns). This means the declarations persist through LTO and don't get deduplicated against the definitions - that's not ideal/will bloat up LTO and ThinLTO bitcode, and persist through other optimizations (eg: if the function gets optimized away/all call sites are dead and optimized out, etc) - it's /probably/ better they get attached to the llvm::Function declaration so it can be cleaned up as needed. That's why DISubprogram moved to be attached to an llvm::Function (previously all DISubprograms were listed on the CU and DISubprograms pointed to llvm::Functions) to aid in LTO (@aprantl and @dexonsmith who were involved in that migration).

But as it pertains to this bug - I /imagine/ the fix proposed right now is about right, but I'd like @vsk or similar to chime in to confirm (& then maybe start another thread on ^)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79967/new/

https://reviews.llvm.org/D79967