dblaikie added a comment. In D104777#2838211 <https://reviews.llvm.org/D104777#2838211>, @aaron.ballman wrote:
> In D104777#2837794 <https://reviews.llvm.org/D104777#2837794>, @dblaikie > wrote: > >> In D104777#2837347 <https://reviews.llvm.org/D104777#2837347>, >> @brunodefraine wrote: >> >>> In D104777#2836669 <https://reviews.llvm.org/D104777#2836669>, @dblaikie >>> wrote: >>> >>>> Yeah, all that sounds reasonable to me - @brunodefraine could you look >>>> into supporting nodebug in a similar way as @aaron.ballman has described >>>> here? >>> >>> Since the debuginfo for `use()` is slightly affected by the `nodebug` >>> version of `t1()` that follows it, I can see how this back propagation is >>> perhaps dangerous. Checking that `nodebug` is the same on all declarations >>> of a function is a way to prevent this. >>> >>> But when discussing the PR, @probinson wrote "I'm inclined to think we want >>> this to work" and I can see what he means from the use case where I >>> observed the bug. If you don't want debuginfo for the implementation of >>> `t1()`, it should be fine to annotate just the function definition in an >>> implementation file, not the declaration in a header, since the debuginfo >>> of the implementation is not of the caller's concern. But `nodebug` as it >>> exists **does** affect the debuginfo of callers as well, so I cannot really >>> express that I don't want debuginfo for the implementation of a function >>> and leave its callers unaffected? >> >> I can see the convenience there, to be sure, being able to put the attribute >> directly on the function you want to debug - but consistency in how >> attributes are handled (admitedly this isn't a strong consistency - some are >> handled this way, some aren't) & consistently seeing the same state for an >> attribute for a given function seems useful. >> >> @probinson - thoughts? > > To me, this is the key bit: > >> But `nodebug` as it exists **does** affect the debuginfo of callers as well, >> so I cannot really express that I don't want debuginfo for the >> implementation of a function and leave its callers unaffected? > > because it **does** affect the callers, the programmer introducing the API > should be aware of that. Making this case an error helps them to understand > that this attribute is actually a part of their API and not an implementation > detail, and silently applying the attribute may cause hard-to-debug problems > for them after deployment of their API. Yeah - the effect on callers is fairly minimal when it comes to DWARF. It changes whether DW_TAG_call_sites are emitted for the function - those call sites can help describe tail calls (which isn't nothing - in the absence of this info you can have missing stack frames, I think) but also mostly helps by describing "entry values" (if a caller stashes a value before calling a function, the call site can record where the extra copy was stored - then the callee might be able to describe the value of its parameters in terms of those entry values even if the callee's copy has been clobbered) - that these features would be dropped only in the translation unit that's compiling the implementation of the function, but not in every calling TU, is not a great loss - adding the attribute to the header would remove this extra information from everywhere (but at the expense of a longer build, having to rebuild all the dependencies). Hmm - yeah, nodebug is more the sort of thing you put on a function because it's not an abstraction you want to debug - so I /think/ it makes sense to put it on the declaration in the header, rather than only on the implementation (I was going to say I thought it'd be the thing someone might do more iteratively/interactively - but that's "optnone" - where you'd want to put it on a function during a test/debug loop and wouldn't want to have to rebuild the whole project because it's more one-off, but "nodebug" isn't intended for that same sort of one-off use case) Eh, I could see it going either way... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104777/new/ https://reviews.llvm.org/D104777 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits