nickdesaulniers 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.
+ }];
----------------
@aaron.ballman anything I should add to the documentation blurb?
================
Comment at: clang/include/clang/Basic/DiagnosticGroups.td:1205
def BackendOptimizationFailure : DiagGroup<"pass-failed">;
+def BackendUserDiagnostic : DiagGroup<"user-diagnostic">;
----------------
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?
================
Comment at: clang/lib/CodeGen/CodeGenAction.cpp:783
+ else
+ Diags.Report(DiagID) << CalleeName << D.getMsgStr();
+}
----------------
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."
================
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();
----------------
To check this, I should probably add tests using `__attribute__((__error__))`
as opposed to just testing `__attribute__((error))`
================
Comment at: llvm/include/llvm/IR/DiagnosticInfo.h:1079
+public:
+ // TODO: make LocCookie optional
+ DiagnosticInfoDontCall(StringRef CalleeName, unsigned LocCookie)
----------------
TODO: play with llvm::Optional
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D106030/new/
https://reviews.llvm.org/D106030
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
