[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:4138
+                             "functions and statements">;
+  let SimpleHandler = 1;
+}
----------------
lebedev.ri wrote:
> aaron.ballman wrote:
> > lebedev.ri wrote:
> > > aaron.ballman wrote:
> > > > lebedev.ri wrote:
> > > > > aaron.ballman wrote:
> > > > > > I would have guessed we'd want to help the user out in a case like: `[[clang::nounwind]] void func() noexcept(false);`, given that this stuff can creep in via macros?
> > > > > Could you please clarify, what do you mean by "help the user" in this case?
> > > > > Given
> > > > > ```
> > > > > [[clang::nounwind]] void func() noexcept(false);
> > > > > 
> > > > > void qqq();
> > > > > 
> > > > > void foo() {
> > > > >   try {
> > > > >     func();
> > > > >   } catch (...) {
> > > > >     qqq();
> > > > >   }
> > > > > }
> > > > > ```
> > > > > we already end up with
> > > > > ```
> > > > > ; Function Attrs: mustprogress noinline nounwind optnone uwtable
> > > > > define dso_local void @_Z3foov() #0 {
> > > > > entry:
> > > > >   call void @_Z4funcv() #2
> > > > >   ret void
> > > > > }
> > > > > 
> > > > > ; Function Attrs: nounwind
> > > > > declare void @_Z4funcv() #1
> > > > > 
> > > > > attributes #0 = { mustprogress noinline nounwind optnone uwtable "frame-pointer"="all" "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
> > > > > attributes #1 = { nounwind "frame-pointer"="all" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
> > > > > attributes #2 = { nounwind }
> > > > > ```
> > > > > and if i drop `[[clang::nounwind]]`, `landingpad` is back.
> > > > > Could you please clarify, what do you mean by "help the user" in this case?
> > > > 
> > > > The user has said "this function throws exceptions" (`noexcept(false)`) and it's also said "this function never unwinds to its caller" (the attribute) and these statements are in conflict with one another and likely signify user confusion. I would have expected a warning diagnostic here.
> > > Ah. This is effectively intentional. This attribute, effectively,
> > > specifies the behavior in case the exception does get thrown, as being UB.
> > > If we diagnose that case, should we also disallow the stmt case?
> > > ```
> > > void i_will_throw() noexcept(false);
> > > 
> > > void foo() {
> > >   [[clang::nounwind]] i_will_throw(); // Should this be diagnosed?
> > > }
> > > ```
> > > Presumably not, because that immediately limits usefulness of the attribute.
> > > 
> > > What we *DO* want, is diagnostics (and more importantly, a sanitizer!)
> > > in the case the exception *does* reach this UB boundary.
> > Sorry if this is a dumb question, but is the goal of this attribute basically to insert UB where there is well-defined behavior per spec? Throwing an exception that escapes from a function marked `noexcept(false)` is guaranteed to call `std::terminate()`: http://eel.is/c++draft/except#spec-5
> > Sorry if this is a dumb question, but is the goal of this attribute basically to insert UB where there is well-defined behavior per spec? 
> 
> Precisely! I was under impression i did call that out in the RFC/description, but guess not explicitly enough?
> This is consistent with the existing (accidental) behavior of `const`/`pure` attirbutes.
@lebedev.ri and I (and a few others) discussed this attribute on IRC and we're thinking of trying a different approach. Instead of exposing this attribute (which is pretty expert-only and seems likely to cause unintentional UB as a result), it might be worth exploring adding a `-f` flag to treat non-throwing exception specifications as implying `nounwind`. This is a language dialect and we usually avoid those, but we already have a significantly greater dialect in this same space with the `-fno-exceptions` functionality. We think the perf gains for the flag are likely to be worth supporting the dialect.

Next steps here are to rip out the nounwind attribute bits so we can move on the const and pure changes, and to start an RFC about the potential new flag.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138958