[PATCH] D106030: [Clang] add support for error+warning fn attrs

Wed Aug 4 12:49:48 PDT 2021

aaron.ballman added inline comments.

================
Comment at: clang/include/clang/Basic/AttrDocs.td:6057
+    assertions that depend on optimizations, while providing diagnostics
+    pointing to precise locations of the call site in the source.
+  }];
----------------
nickdesaulniers wrote:
> @aaron.ballman anything I should add to the documentation blurb?
I think the prose is generally fine, but it might be useful to show some trivial example usages to set expectations. e.g.,
```
__attribute__((warning("oh no"))) void unused_func() {} // Not diagnosed because it's never called
__attribute__((warning("oh no"))) void used_func() {}
__attribute__((error("oh no"))) int unevaluated_func() { return 0; }
__attribute__((error("oh no"))) constexpr int constexpr_func() { return 12; }

void use() {
  used_func(); // Warning
  sizeof(unevaluated_func()); // No error, function never called
  int array[constexpr_func()]; // No error, function never called
}
```
Feel free to use better/different/more examples, btw. I'm not tied to mine.

================
Comment at: clang/include/clang/Basic/DiagnosticGroups.td:1205
 def BackendOptimizationFailure : DiagGroup<"pass-failed">;
+def BackendUserDiagnostic : DiagGroup<"user-diagnostic">;

----------------
nickdesaulniers wrote:
> aaron.ballman wrote:
> > nickdesaulniers wrote:
> > > aaron.ballman wrote:
> > > > nickdesaulniers wrote:
> > > > > aaron.ballman wrote:
> > > > > > GCC doesn't seem to support this spelling -- do they have a different one we should reuse?
> > > > > I think these diagnostics don't have corresponding flags in GCC, so they cannot be disabled.
> > > > > 
> > > > > Without adding this, clang/test/Misc/warning-flags.c would fail, because I was adding a new unnamed warning `warn_fe_backend_user_diagnostic`.  Perhaps I should not be adding this line here, and doing something else?
> > > > > 
> > > > > That test says we shouldn't be adding new warnings not controlled by flags, but that's very much my intention.
> > > > > 
> > > > > Though now `-Wno-user-diagnostic` doesn't produce a `-Wunknown-warning-option` diagnostic...
> > > > Ah! I think the warning attribute should be controllable via a diagnostic flag (we should always be able to disable warnings with some sort of flag) and I don't think the error attribute should be controllable (an error is an error and should stop translation, same as with `#error`).
> > > > 
> > > > Normally, I'd say "let's use the same flag that controls `#warning`" but...
> > > > ```
> > > > def PoundWarning : DiagGroup<"#warnings">;
> > > > ```
> > > > That's... not exactly an obvious flag for the warning attribute. So I would probably name it `warning-attributes` similar to how we have `deprecated-attributes` already.
> > > Done, now `-Wno-warning-attributes` doesn't produce `-Wunknown-warning-option`, but also doesn't disable the diagnostic.  Was there something else I needed to add?
> > huh, that sounds suspicious -- you don't need to do anything special for `-Wno-foo` handling, that should be automatically supported via tablegen. I'm not certain why you're not seeing `-Wno-warning-attributes` silencing the warnings.
> Ah! Because I was testing `__attribute__((error("")))` not `__attribute__((warning("")))`. `-Wno-warning-attributes` only affects the latter, not the former.  I should add a test for `-Wno-warning-attributes`! Is this something I also need to add documentation for?
Given that this behavior surprised you, I certainly wouldn't oppose mentioning it in the documentation, but I also don't think it's strictly required. That said, I do agree about adding some additional test coverage for when the warning is disabled.

Just to double-check: do you think this functionality needs an "escape hatch" for the error case? e.g., should the `error` attribute generate a warning diagnostic that defaults to being an error, so we have `-Wno-warning-attributes` to turn off `warning` attribute diagnostics and `-Wno-error-attributes` to turn off `error` attribute diagnostics?

================
Comment at: clang/lib/CodeGen/CodeGenAction.cpp:783
+  else
+    Diags.Report(DiagID) << CalleeName << D.getMsgStr();
+}
----------------
nickdesaulniers wrote:
> aaron.ballman wrote:
> > nickdesaulniers wrote:
> > > aaron.ballman wrote:
> > > > nickdesaulniers wrote:
> > > > > nickdesaulniers wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > nickdesaulniers wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > I think the usability in this situation is a concern -- only having a function name but no source location information is pretty tricky. Do you have a sense for how often we would hit this case?
> > > > > > > > I don't think we ever encounter it when building the kernel sources. Perhaps I should make this an assertion on `LocCookie.isValid()` instead?
> > > > > > > > 
> > > > > > > > While clang should always attach a `srcloc` MetaData node, other frontend might not, so it's optional.  But this TU is strictly Clang.
> > > > > > > I thought backend optimization passes would drop source location information with some regularity, so I'm a bit hesitant to go with an assertion unless it turns out I'm wrong there. However, if the backend shouldn't ever lose *this* source location information, then I think it's good to make it an assertion.
> > > > > > Perhaps debug info, but this SourceLocation is squirreled away in a Metadata node attached to the call site.
> > > > > > 
> > > > > > Ah, but via further testing of @arsenm 's point ("what about indirect calls?"), this case is tricky:
> > > > > > ```
> > > > > > // clang -O2 foo.c
> > > > > > __attribute__((error("oh no foo")))
> > > > > > void foo(void);
> > > > > > 
> > > > > > void (*bar)(void);
> > > > > > 
> > > > > > void baz(void) {
> > > > > >   bar = foo;
> > > > > >   bar();
> > > > > > }
> > > > > > ```
> > > > > > since we did not emit the Metadata node on the call, but then later during optimizations we replaced the call to `bar` with a call to `foo`. At that point, the front end did not emit a Metadata node with source location information, so we can't reconstruct the call site properly for the diagnostic. Hmm...I'll need to think more about this case.
> > > > > One thing I was thinking of to support @arsenm's case:
> > > > > 
> > > > > We probably generally could support diagnosing indirect calls if `-g` was used to emit `DILocation` MD nodes.  I'd need to change this up from a custom `!srcloc` node with one `unsigned` that is a `SourceLocation` to use the the `DILocation` tuple of `line`, `column`, and `scope` then change the frontend's `BackendDiagnosticConsumer` to reinstantiate a `SourceLocation` from those (if possible).
> > > > > 
> > > > > The only thing is: is it bad if we can only diagnose with `-g` for indirect calls?
> > > > > 
> > > > > Otherwise I don't see how we can support diagnosing indirect calls. GCC can.  We'd perhaps need to emit `!srcloc` or `DILocation` nodes for EVERY indirect call from the frontend, and add logic to merge these or something during optimizations when we turn indirect calls into direct calls.  And I'm sure that would cause lots of churn in the LLVM source tree / in the tests. I would create a parent revision to precommit that.  But I think that's maybe too overkill?
> > > > > 
> > > > > The current behavior is that we don't diagnose indirect calls.  I'm happy to add documentation, or create child or parent revisions trying to tackle that, but I think if we can't provide accurate debug info, it's perhaps better not to diagnose at all.
> > > > > 
> > > > > I could change that so that we still emit the diagnostic, but don't show _where_ the callsite was in the sources as part of the diagnostic.  But I think it's perhaps better to simply not warn in that case when we can't provide line info.
> > > > > 
> > > > > But this can go either way, so I appreciate others' guidance and thoughts here.
> > > > > Perhaps debug info, but this SourceLocation is squirreled away in a Metadata node attached to the call site.
> > > > 
> > > > I thought optimization passes frequently drop metadata nodes?
> > > > I thought optimization passes frequently drop metadata nodes?
> > > 
> > > Perhaps, but I've yet to see Metadata attached to a static `call` get dropped.  This patch emits such Metadata attached to `call` IR Instructions when the frontend sees a static call of callee whose Decl has these GNU C fn attrs.
> > > 
> > > Perhaps when we de-indirect an indirect call to a direct call we don't carry over the Metadata (unsure), but it's moot because the frontend didn't know to attach the `dontcall` IR Fn attr because the frontend _cant_ de-indirect the call.
> > > 
> > > So it's not a case that de-indirection in the backend drops the Metadata node (though perhaps that's also an issue), moreso that the frontend didn't know to attach the metadata in the first place because it only saw an indirect call.
> > > 
> > > We could emit such Metadata for _all_ indirect calls from the frontend (overkill), diagnose only when Debug Info is requested (not great), diagnose but not provide line no of indirect call site (not great), simply not diagnose (not great, but also unlikely IMO for there to be indirect calls of such attributed functions), or maybe something else I'm not thinking of.  The current implementation chooses to not diagnose.  Is that the best choice? Definitely a clothespin vote.  Is there something better?  I was hoping you all would tell me. :)
> > > The current implementation chooses to not diagnose. Is that the best choice?
> > 
> > I think it's a reasonable choice for the initial implementation. We can improve it later when we have a concrete use case. My primary concern was addressed though -- I'd rather have a false negative than a diagnostic with no source location information. The false negative is unfortunate, but IMO better than a frustrating diagnostic without source location information. The upside to the frustrating diagnostic is that we'd get a bug report about that (it's easier to miss the false negative and file a bug report about that). I suppose one way to address that is to use an assertion before the early return with a comment that the assertion is speculative rather than assertive. So in asserts builds we get a loud report and in other builds, we get the false negatives. WDYT?
> > I'd rather have a false negative than a diagnostic with no source location information. The false negative is unfortunate, but IMO better than a frustrating diagnostic without source location information.
> 
> Agreed.
> 
> > WDYT?
> 
> One part is that I added a test case to clang/test/Frontend/backend-attribute-error-warning-optimize.c with a TODO saying "I know this doesn't work yet" that would trigger such an assertion.  I think it would be unacceptable to add a new test that is red only for assertion enabled builds, which would be the case if I added the assertion.
> 
> So if I add the assertion, then I should just remove the unit test? I guess it doesn't add much value to have a TODO in the sources AND in a unit test both effectively saying "yes, this doesn't work yet."
> So if I add the assertion, then I should just remove the unit test? I guess it doesn't add much value to have a TODO in the sources AND in a unit test both effectively saying "yes, this doesn't work yet."

Agreed that duplicating the test coverage doesn't buy us much. I don't have a strong opinion on which way to go, so whatever seems sensible to you is likely fine by me. (I'd be fine if we left the assertion out and just went with the unit test coverage.)

================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:957
+    // TODO: something better than getNormalizedFullName to tell which spelling
+    // the new attribute we're trying to add is using?
+    std::string NewAttr = AL.getNormalizedFullName();
----------------
nickdesaulniers wrote:
> To check this, I should probably add tests using `__attribute__((__error__))` as opposed to just testing `__attribute__((error))`
I think a better approach is to use `AL.getAttributeSpellingListIndex()` instead of doing a string comparison directly. `ErrorAttr` will have a public `Spelling` enum that's auto-generated for it with all the different spellings. See the Attrs.inc file that's generated for the full list, but usage would look something like: https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaDeclAttr.cpp#L3074

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030