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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138958