Add FixItHints, Improve wording of Diagnosis messages

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -2336,6 +2336,11 @@ def note_in_reference_temporary_list_initializer : Note<
   "list-initialize this reference">;
 def note_var_fixit_add_initialization : Note<
   "initialize the variable %0 to silence this warning">;
+def warn_cheri_compartment_void_return_type : Warning <
+  "void return on a cross-compartment call make it impossible for callers to detect failure">;
+def note_cheri_compartment_void_return_type : Note<"replace void return type with int">;
+def warn_cheri_compartment_return_void_or_falloff : Warning <
+  "Cross-compartement calls that always succeed should return 0 instead">;
 def note_uninit_fixit_remove_cond : Note<
   "remove the %select{'%1' if its condition|condition if it}0 "
   "is always %select{false|true}2">;

clang/include/clang/Sema/Sema.h

@@ -4717,8 +4717,16 @@ class Sema final {
     bool IgnoreTypeAttributes;
   };
 
+  enum DeclAttributeLocation {
+    DAL_Unspecified,
+    DAL_DeclSpec,
+    DAL_DeclChunk,
+    DAL_Decl,
+  };
+
   void ProcessDeclAttributeList(Scope *S, Decl *D,
                                 const ParsedAttributesView &AttrList,
+                                DeclAttributeLocation DAL = DAL_Unspecified,
                                 const ProcessDeclAttributeOptions &Options =
                                     ProcessDeclAttributeOptions());
   bool ProcessAccessDeclAttributeList(AccessSpecDecl *ASDecl,

clang/lib/Sema/AnalysisBasedWarnings.cpp

@@ -621,7 +621,7 @@ struct CheckFallThroughDiagnostics {
   }
 
   bool checkDiagnostics(DiagnosticsEngine &D, bool ReturnsVoid,
-                        bool HasNoReturn) const {
+                        bool HasNoReturn, bool HasCHERICompartmentName) const {
     if (funMode == Function) {
       return (ReturnsVoid ||
               D.isIgnored(diag::warn_maybe_falloff_nonvoid_function,
@@ -630,7 +630,8 @@ struct CheckFallThroughDiagnostics {
               D.isIgnored(diag::warn_noreturn_function_has_return_expr,
                           FuncLoc)) &&
              (!ReturnsVoid ||
-              D.isIgnored(diag::warn_suggest_noreturn_block, FuncLoc));
+              D.isIgnored(diag::warn_suggest_noreturn_block, FuncLoc)) &&
+             !HasCHERICompartmentName;
     }
     if (funMode == Coroutine) {
       return (ReturnsVoid ||
@@ -658,6 +659,7 @@ static void CheckFallThroughForBody(Sema &S, const Decl *D, const Stmt *Body,
 
   bool ReturnsVoid = false;
   bool HasNoReturn = false;
+  bool HasCHERICompartmentName = false;
   bool IsCoroutine = FSI->isCoroutine();
 
   if (const auto *FD = dyn_cast<FunctionDecl>(D)) {
@@ -666,6 +668,7 @@ static void CheckFallThroughForBody(Sema &S, const Decl *D, const Stmt *Body,
     else
       ReturnsVoid = FD->getReturnType()->isVoidType();
     HasNoReturn = FD->isNoReturn();
+    HasCHERICompartmentName = FD->hasAttr<CHERICompartmentNameAttr>();
   }
   else if (const auto *MD = dyn_cast<ObjCMethodDecl>(D)) {
     ReturnsVoid = MD->getReturnType()->isVoidType();
@@ -684,8 +687,9 @@ static void CheckFallThroughForBody(Sema &S, const Decl *D, const Stmt *Body,
   DiagnosticsEngine &Diags = S.getDiagnostics();
 
   // Short circuit for compilation speed.
-  if (CD.checkDiagnostics(Diags, ReturnsVoid, HasNoReturn))
-      return;
+  if (CD.checkDiagnostics(Diags, ReturnsVoid, HasNoReturn,
+                          HasCHERICompartmentName))
+    return;
   SourceLocation LBrace = Body->getBeginLoc(), RBrace = Body->getEndLoc();
   auto EmitDiag = [&](SourceLocation Loc, unsigned DiagID) {
     if (IsCoroutine)
@@ -708,12 +712,26 @@ static void CheckFallThroughForBody(Sema &S, const Decl *D, const Stmt *Body,
         EmitDiag(RBrace, CD.diag_MaybeFallThrough_HasNoReturn);
       else if (!ReturnsVoid)
         EmitDiag(RBrace, CD.diag_MaybeFallThrough_ReturnsNonVoid);
+
+      if (HasCHERICompartmentName)
+        if (!ReturnsVoid)
+          S.Diag(RBrace, diag::warn_cheri_compartment_return_void_or_falloff);
+        else
+          S.Diag(RBrace, diag::warn_cheri_compartment_return_void_or_falloff)
+              << FixItHint::CreateInsertion(RBrace, "return 0;");
       break;
     case AlwaysFallThrough:
       if (HasNoReturn)
         EmitDiag(RBrace, CD.diag_AlwaysFallThrough_HasNoReturn);
       else if (!ReturnsVoid)
         EmitDiag(RBrace, CD.diag_AlwaysFallThrough_ReturnsNonVoid);
+
+      if (HasCHERICompartmentName)
+        if (!ReturnsVoid)
+          S.Diag(RBrace, diag::warn_cheri_compartment_return_void_or_falloff);
+        else
+          S.Diag(RBrace, diag::warn_cheri_compartment_return_void_or_falloff)
+              << FixItHint::CreateInsertion(RBrace, "return 0;");
       break;
     case NeverFallThroughOrReturn:
       if (ReturnsVoid && !HasNoReturn && CD.diag_NeverFallThroughOrReturn) {

clang/lib/Sema/SemaDeclAttr.cpp

@@ -2388,17 +2388,49 @@ static void handleCHERIMethodSuffix(Sema &S, Decl *D, const ParsedAttr &Attr) {
   D->addAttr(::new (S.Context) CHERIMethodSuffixAttr(S.Context, Attr, Str));
 }
 
-static void handleCHERICompartmentName(Sema &S, Decl *D, const ParsedAttr &Attr) {
+static void handleCHERICompartmentName(Sema &S, Decl *D, const ParsedAttr &Attr,
+                                       Sema::DeclAttributeLocation DAL) {
+  if (DAL != Sema::DAL_DeclSpec && DAL != Sema::DAL_Unspecified)
+    return;
+
+  // cheri_compartment is both:
+  //
+  // * a Declaration attribute: marks the function as a compartment entry point
+  // * a Function Type attribute: affects the calling convention
+  //
+  // That's the reason why we don't short-circuit with hasDeclarator
+  // (as other handlers do).
+
   StringRef Str;
   SourceLocation LiteralLoc;
   if (!S.checkStringLiteralArgumentAttr(Attr, 0, Str, &LiteralLoc))
     return;
 
-  if (D->getFunctionType() &&
-      D->getFunctionType()->getReturnType()->isVoidType())
-    S.Diag(Attr.getLoc(), diag::warn_attribute_void_function_method)
-        << Attr << 0;
-  else
+  // cheri_compartment is considered as function type attribute
+
+  const auto *FD = dyn_cast<FunctionDecl>(D);
+
+  if (FD && FD->getReturnType()->isVoidType()) {
+    S.Diag(Attr.getLoc(), diag::warn_cheri_compartment_void_return_type);
+
+    const TypeSourceInfo *TSI = FD->getTypeSourceInfo();
+    TypeLoc TL = TSI->getTypeLoc().IgnoreParens();
+
+    // ignore function type attributes
+    while (auto ATL = TL.getAs<AttributedTypeLoc>())
+      TL = ATL.getModifiedLoc();
+
+    if (auto FTL = TL.getAs<FunctionTypeLoc>()) {
+      SourceRange SR = FTL.getReturnLoc().getSourceRange();
+      S.Diag(SR.getBegin(), diag::note_cheri_compartment_void_return_type)
+          << FixItHint::CreateReplacement(SR, "int");
+    }
+    //    FunctionTypeLoc FTL = FD->getFunctionTypeLoc();
+
+    //    TypeLoc RL = FTL.getReturnLoc();
+    //    SourceRange SR = FD->getReturnTypeSourceRange();
+
+  } else
     D->addAttr(::new (S.Context) WarnUnusedResultAttr(
         S.Context, Attr, "CHERI compartment call"));
 
@@ -8941,7 +8973,8 @@ static bool MustDelayAttributeArguments(const ParsedAttr &AL) {
 /// silently ignore it if a GNU attribute.
 static void
 ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D, const ParsedAttr &AL,
-                     const Sema::ProcessDeclAttributeOptions &Options) {
+                     const Sema::ProcessDeclAttributeOptions &Options,
+                     Sema::DeclAttributeLocation DAL = Sema::DAL_Unspecified) {
   if (AL.isInvalid() || AL.getKind() == ParsedAttr::IgnoredAttribute)
     return;
 
@@ -9450,7 +9483,7 @@ ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D, const ParsedAttr &AL,
     handleCHERIMethodSuffix(S, D, AL);
     break;
   case ParsedAttr::AT_CHERICompartmentName:
-    handleCHERICompartmentName(S, D, AL);
+    handleCHERICompartmentName(S, D, AL, DAL);
     break;
   case ParsedAttr::AT_InterruptState:
     handleInterruptState(S, D, AL);
@@ -9764,12 +9797,13 @@ ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D, const ParsedAttr &AL,
 /// attribute list to the specified decl, ignoring any type attributes.
 void Sema::ProcessDeclAttributeList(
     Scope *S, Decl *D, const ParsedAttributesView &AttrList,
+    Sema::DeclAttributeLocation DAL,
     const ProcessDeclAttributeOptions &Options) {
   if (AttrList.empty())
     return;
 
   for (const ParsedAttr &AL : AttrList)
-    ProcessDeclAttribute(*this, S, D, AL, Options);
+    ProcessDeclAttribute(*this, S, D, AL, Options, DAL);
 
   // FIXME: We should be able to handle these cases in TableGen.
   // GCC accepts
@@ -10020,6 +10054,7 @@ void Sema::ProcessDeclAttributes(Scope *S, Decl *D, const Declarator &PD) {
   // Apply decl attributes from the DeclSpec if present.
   if (!PD.getDeclSpec().getAttributes().empty()) {
     ProcessDeclAttributeList(S, D, PD.getDeclSpec().getAttributes(),
+                             DAL_DeclSpec,
                              ProcessDeclAttributeOptions()
                                  .WithIncludeCXX11Attributes(false)
                                  .WithIgnoreTypeAttributes(true));
@@ -10031,13 +10066,14 @@ void Sema::ProcessDeclAttributes(Scope *S, Decl *D, const Declarator &PD) {
   // when X is a decl attribute.
   for (unsigned i = 0, e = PD.getNumTypeObjects(); i != e; ++i) {
     ProcessDeclAttributeList(S, D, PD.getTypeObject(i).getAttrs(),
+                             DAL_DeclChunk,
                              ProcessDeclAttributeOptions()
                                  .WithIncludeCXX11Attributes(false)
                                  .WithIgnoreTypeAttributes(true));
   }
 
   // Finally, apply any attributes on the decl itself.
-  ProcessDeclAttributeList(S, D, PD.getAttributes());
+  ProcessDeclAttributeList(S, D, PD.getAttributes(), DAL_Decl);
 
   // Apply additional attributes specified by '#pragma clang attribute'.
   AddPragmaAttributes(S, D);

clang/lib/Sema/SemaStmt.cpp

@@ -4120,6 +4120,12 @@ StmtResult Sema::BuildReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp,
           return StmtError();
         RetValExp = ER.get();
       }
+    } else if (getCurFunctionOrMethodDecl()
+                   ->hasAttr<CHERICompartmentNameAttr>()) {
+      SourceLocation AfterReturnLoc = getLocForEndOfToken(ReturnLoc);
+      /* Compartment call */
+      Diag(ReturnLoc, diag::warn_cheri_compartment_return_void_or_falloff)
+          << FixItHint::CreateInsertion(AfterReturnLoc, " 0");
     }
 
     Result = ReturnStmt::Create(Context, ReturnLoc, RetValExp,

clang/test/Sema/cheri/cheri-compartment-warn-if-return-void-or-unused.c

@@ -1,8 +1,17 @@
-// RUN: %clang_cc1 %s -o - -triple riscv32-unknown-unknown -emit-llvm -mframe-pointer=none -mcmodel=small -target-cpu cheriot -target-feature +xcheri -target-feature -64bit -target-feature -relax -target-feature -xcheri-rvc -target-feature -save-restore -target-abi cheriot -Oz -cheri-compartment=example -fsyntax-only -verify
+// RUN: %clang_cc1 %s -o - -triple riscv32-unknown-unknown -emit-llvm -mframe-pointer=none -mcmodel=small -target-cpu cheriot -target-feature +xcheri -target-feature -64bit -target-feature -relax -target-feature -xcheri-rvc -target-feature -save-restore -target-abi cheriot -Oz -cheri-compartment=example -verify -fsyntax-only
+// RUN: %clang_cc1 %s -o - -triple riscv32-unknown-unknown -emit-llvm -mframe-pointer=none -mcmodel=small -target-cpu cheriot -target-feature +xcheri -target-feature -64bit -target-feature -relax -target-feature -xcheri-rvc -target-feature -save-restore -target-abi cheriot -Oz -cheri-compartment=example -fdiagnostics-parseable-fixits -fsyntax-only 2>&1 | FileCheck %s
 
-__attribute__((cheri_compartment("example"))) void void_return_type_f() // expected-warning{{attribute 'cheri_compartment' cannot be applied to functions without return value}} expected-warning{{attribute 'cheri_compartment' cannot be applied to functions without return value}}
+//  -fdiagnostics-parseable-fixits -fixit
+//
+
+//  -fdiagnostics-fixit-info
+__attribute__((cheri_compartment("example"))) void void_return_type_f(int a) // expected-warning{{void return on a cross-compartment call make it impossible for callers to detect failure}} expected-note{{replace void return type with int}}
 {
-}
+  if (a) {
+    /// CHECK: fix-it:"{{.*}}":{[[@LINE+1]]:[[COL:[0-9]+]]-[[@LINE+1]]:[[COL]]}:" 0"
+    return; // expected-warning{{Cross-compartement calls that always succeed should return 0 instead}}
+  }
+} // expected-warning{{Cross-compartement calls that always succeed should return 0 instead}}
 
 __attribute__((cheri_compartment("example"))) int int_return_type_f() {
   return 0;