NoDebug attr does not totally eliminate debug info about a function when
inlining is enabled. This is inconsistent with when inlining is disabled.
This patch fixes that.
yaxunl on May 14 2020, 2:41 PM.Authored by
I'm not quite following the test case - could you explain how it's testing the situation?
It might be better/easier to have a more intentional test - also, could you check the commit history for this declaration-emitting functionality (CGDebugInfo::EmitFunctionDecl when used for a non-member function) & pull them into this review too?
This patch is a follow up of https://reviews.llvm.org/D79866, where we marked a compiler generated stub function with NoDebug attribute to eliminate debug info for it. Then we found there is a bug causing debug info not fully eliminated for a function with NoDebug attribute when the function is inlined, therefore I created this patch.
The bug can be demonstrated by
There is debug info with function foo, which should not. If you change -O3 to -O0, the debug info on function foo disappears, which is the expected behavior.
The following code is supposed to eliminate all debug info for functions with NoDebugAttr, however it is not sufficient when the function gets inlined:
I can add a lit test for a function with NoDebugAttr.
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:
However, it seems he is not in Phabricator.
Based on documentation of nodebug attribute:
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.
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 ^)
This seems reasonable, so this change looks good to me!
@dblaikie Thanks for pointing out to the potential problems of the usage of the func decl !dbg in the purpose of call sites debug info. It is currently being stored into CU's retainedTypes field.
Is it only one patch or a set of patches? I'll try to find these changes in the log history.
I am open for the discussion about redesigning it (please wait for a comment from @vsk as well). :)