aaron.ballman added inline comments.
================ Comment at: clang/test/Frontend/backend-attribute-error-warning-optimize.c:12 foo(); // expected-error {{call to 'foo' declared with 'error' attribute: oh no foo}} + // expected-note@* {{In function 'baz'}} if (x()) ---------------- nickdesaulniers wrote: > cjdb wrote: > > aaron.ballman wrote: > > > nickdesaulniers wrote: > > > > cjdb wrote: > > > > > aaron.ballman wrote: > > > > > > nickdesaulniers wrote: > > > > > > > nickdesaulniers wrote: > > > > > > > > aaron.ballman wrote: > > > > > > > > > Instead of allowing this note to appear anywhere in the file, > > > > > > > > > I think it's better to use "bookmark" comments. e.g., > > > > > > > > > ``` > > > > > > > > > void baz() { // #baz_defn > > > > > > > > > } > > > > > > > > > > > > > > > > > > void foo() { > > > > > > > > > baz(); // expected-note@#baz_defn {{whatever note text}} > > > > > > > > > } > > > > > > > > > ``` > > > > > > > > > so that we're testing where the diagnostic is emitted. (This > > > > > > > > > is especially important given that the changes are about > > > > > > > > > location tracking.) > > > > > > > > oh, that's new (to me)! will do > > > > > > > It looks like the "bookmarks" don't work because the notes do not > > > > > > > line+col info. They follow the warning/error diagnostic which > > > > > > > does, on the bottom most call site. > > > > > > > > > > > > > > The warning is supposed to be emitted on the callsite, not the > > > > > > > definition. > > > > > > I still don't think this is reasonable for test coverage because > > > > > > this means we'll accept the note *anywhere* in the file. This makes > > > > > > it much too easy to regress the behavior accidentally (e.g., > > > > > > accidentally emit all the notes on line 1 of the file). I think we > > > > > > need *some* way to nail down where these notes are emitted in the > > > > > > test. However, I might be asking you to solve "please track > > > > > > location information through the backend" for that, so perhaps this > > > > > > isn't a reasonable request? > > > > > > > > > > > > Also, CC @cjdb for awareness of how this kind of diagnostic is > > > > > > produced (Chris is working on an idea for how we emit diagnostics > > > > > > so we get better structured information from them, so knowing about > > > > > > this novel approach might impact that idea). > > > > > Very interesting, thanks for the heads up! Cross-phase diagnostics > > > > > are definitely something I hadn't considered, and it **does** impact > > > > > the design (but not in a derailing way). > > > > I think we're back at "please track location information through the > > > > backend". > > > > > > > > That is the tradeoff with this approach; not measurably regressing > > > > performance when these attributes aren't used in source in exchange for > > > > Note diagnostics that lack precise line+col info, but in practice > > > > provide enough info for the user to understand what's going wrong where. > > > > > > > > Your call if the tradeoff is worth it. > > > Here's my thinking, please correct any misunderstandings I have: > > > > > > * This functionality is primarily for use with `_FORTIFY_SOURCE` (but not > > > solely for that purpose), and that's an important security feature used > > > by glibc and the Linux kernel. > > > * Security of C and C++ source code is Very Important, especially now > > > that various gov't agencies [1][2] are suggesting to not use C or C++ for > > > new projects specifically because the security posture of the languages > > > and their tools. > > > * Therefore, the ergonomics of this functionality are quite important for > > > the intended use cases and the ecosystem as a whole. > > > * Emitting only the function name but without location information does > > > not give a good user experience, especially in circumstances where the > > > function has multiple overloads, because it pushes the burden onto the > > > programmer to figure out which functions are actually involved in the > > > call chain. This issue is also present in C because of support for > > > `__attribute__((overloadable))` and is not just a C++ concern. > > > * The compile-time performance costs of tracking this location > > > information are roughly 1%, and there are no runtime or binary size > > > overhead costs unless other functionality is enabled (such as emitting > > > debug info). > > > * We can determine whether we need to enable this location tracking while > > > building the AST when we see one of these two attributes being used, so > > > we could enable this functionality selectively without burdening the user > > > with enabling it manually via a flag. (We could still consider giving the > > > user a flag to never track this even in the presence of the attributes if > > > a user had a compelling use case for it.) > > > > > > If this is all reasonably accurate, I think it's worth seriously > > > considering accepting the costs. I don't want to increase our compile > > > time overhead, but at the same time, if ~1% is a "make or break" amount > > > of compile-time performance to lose, we should be addressing *that* issue > > > rather than hamstringing a user-facing diagnostic feature related to > > > security. It's hard to argue "that CVE was totally worth it because we > > > could then compile 1% faster". That said, perhaps others have a strong > > > contrary opinion they'd like to argue in favor of? > > > > > > [1] > > > https://media.defense.gov/2022/Nov/10/2003112742/-1/-1/0/CSI_SOFTWARE_MEMORY_SAFETY.PDF > > > [2] https://doi.org/10.6028/NIST.IR.8397 > > > > > My attitude is that we should prioritise ergonomics and usability over > > speed. Yes, having to wait a little longer for stuff to build sucks, but > > there's a clear ergonomics issue, which is motivation enough for me in this > > case (adding the CVE argument is the cherry on top). Folks either aren't > > going to use a feature if it's difficult to get right, or they're going to > > use it with large amounts of stress (and I don't think this is fair). > > > > Moreover, I think there may be some opportunities we can look into to > > cancel out this pessimisation. > > The compile-time performance costs of tracking this location information > > are roughly 1%, and there are no runtime or binary size overhead costs > > unless other functionality is enabled (such as emitting debug info). > > (Just a note for passers-by: https://reviews.llvm.org/D141451 is not a 1% > performance regression; @aaron.ballman is referring to > https://discourse.llvm.org/t/rfc-improving-clangs-middle-and-back-end-diagnostics/69261?u=nickdesaulniers > Solution 2 where would could track all location info unconditionally) > > That 1% measurement was for Linux kernel builds, as reported > https://discourse.llvm.org/t/rfc-improving-clangs-middle-and-back-end-diagnostics/69261?u=nickdesaulniers, > and it gets worse for other projects. The ones tracked on llvm compile time > tracker were much worse `geomean slowdown of 4.77% of wall time to build > various open source projects (ranging from 1.95% to 6.9%). ` > http://llvm-compile-time-tracker.com/compare.php?from=c1125ae5b05f5e23c7e61e85afb359e095444d18&to=059eb5565a580f7ea37c2d62dcffdb7401e12e55&stat=wall-time > > I did try tracking //only// source locations of //calls//. > https://discourse.llvm.org/t/rfc-improving-clangs-middle-and-back-end-diagnostics/69261/11?u=nickdesaulniers > That was faster, but I only measured Linux kernel build times, and not llvm > compile time tracker times. I could do a similar experiment (test on llvm > compile time tracker) if that helps? > > > We can determine whether we need to enable this location tracking while > > building the AST when we see one of these two attributes being used > > I had looked into that; I wasn't able to get something like that working. > IIRC, it's hard to modify codegen during parsing, or the decision on what > kind of DICompileUnit to emit had already been made. Or maybe I was trying > to do it from CodeGen after we already started producing IR, I don't remember > now. Are we guaranteed to never start CodeGen of IR until after Sema is > complete? I could look into this again/try harder though. > > > We could still consider giving the user a flag to never track this even in > > the presence of the attributes if a user had a compelling use case for it. > > Yeah, I'm concerned with the interaction between <whatever ships> and `-g0`. > If <whatever ships> == location tracking via debug info, then the presence of > `-g0` may change our diagnostics! > > > That said, perhaps others have a strong contrary opinion they'd like to > > argue in favor of? > > @efriedma had some such feedback on the RFC: > https://discourse.llvm.org/t/rfc-improving-clangs-middle-and-back-end-diagnostics/69261/2?u=nickdesaulniers > That 1% measurement was for Linux kernel builds, as reported > https://discourse.llvm.org/t/rfc-improving-clangs-middle-and-back-end-diagnostics/69261?u=nickdesaulniers, > and it gets worse for other projects. The ones tracked on llvm compile time > tracker were much worse geomean slowdown of 4.77% of wall time to build > various open source projects (ranging from 1.95% to 6.9%). > http://llvm-compile-time-tracker.com/compare.php?from=c1125ae5b05f5e23c7e61e85afb359e095444d18&to=059eb5565a580f7ea37c2d62dcffdb7401e12e55&stat=wall-time Oh, that makes me sad. A 5% slowdown is quite the hit for this. > I did try tracking only source locations of calls. https://discourse.llvm.org/t/rfc-improving-clangs-middle-and-back-end-diagnostics/69261/11?u=nickdesaulniers That was faster, but I only measured Linux kernel build times, and not llvm compile time tracker times. I could do a similar experiment (test on llvm compile time tracker) if that helps? I don't want to send you on a wild goose chase, but if we can find a solution that gets us closer to 1% (or less!), that would make it easier to decide what to do here. I think 5% is... probably too much, but then I wonder "how do we make Fortify work?". > I had looked into that; I wasn't able to get something like that working. > IIRC, it's hard to modify codegen during parsing, Ah, that I'd believe (I forgot, language options are basically const by the time we start parsing). However, could we squirrel this away on `ASTContext` instead? >> That said, perhaps others have a strong contrary opinion they'd like to >> argue in favor of? > @efriedma had some such feedback on the RFC: > https://discourse.llvm.org/t/rfc-improving-clangs-middle-and-back-end-diagnostics/69261/2?u=nickdesaulniers Ah, thank you for reminding me of that! However, I disagree with this: > (Most code doesn’t normally contain either inline asm or warning/error > attributes, and I’m not really convinced by other potential diagnostics.) because this attribute is also used by glibc: https://codebrowser.dev/glibc/glibc/misc/sys/cdefs.h.html#203 and that macro does get used: https://codebrowser.dev/glibc/glibc/stdlib/bits/stdlib.h.html#33 or https://codebrowser.dev/glibc/glibc/posix/bits/unistd.h.html#32 (etc), so I think this could potentially be significantly wider than just the Linux kernel. Does that change your opinion at all @efriedma? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141451/new/ https://reviews.llvm.org/D141451 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits