[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:
> > > > 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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138958