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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri marked 2 inline comments as done.
lebedev.ri added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:4138
+                             "functions and statements">;
+  let SimpleHandler = 1;
+}
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138958