cheri_compartment: Warn if return type is void or return value is unused

Author: v01dxyz

clang/include/clang/AST/TypeLoc.h

@@ -883,6 +883,10 @@ class AttributedTypeLoc : public ConcreteTypeLoc<UnqualTypeLoc,
     return getInnerTypeLoc();
   }
 
+  TypeLoc getEquivalentTypeLoc() const {
+    return TypeLoc(getTypePtr()->getEquivalentType(), getNonLocalData());
+  }
+
   /// The type attribute.
   const Attr *getAttr() const {
     return getLocalData()->TypeAttr;

clang/include/clang/Basic/DiagnosticGroups.td

@@ -158,10 +158,11 @@ def CHERIPrototypesStrict: DiagGroup<"cheri-prototypes-strict">;
 // Remarks about setting/not setting subobject bounds
 def CheriSubobjectBoundsSuspicous : DiagGroup<"cheri-subobject-bounds-suspicious">;
 def CheriSubobjectBoundsRemarks : DiagGroup<"cheri-subobject-bounds">;
+def CHERICompartmentReturnVoid : DiagGroup<"cheri-compartment-return-void">;
 
 def CheriAll : DiagGroup<"cheri",
      [CHERICaps, CHERIBitwiseOps, CHERIMisaligned, CHERIImplicitConversion, CheriSubobjectBoundsSuspicous,
-      CHERIProvenance, CHERIImplicitConversionSign, CHERIPrototypes]>;
+      CHERIProvenance, CHERIImplicitConversionSign, CHERIPrototypes, CHERICompartmentReturnVoid]>;
 // CHERI warnings that are too noisy to turn on by default
 def CHERICapabilityToIntegerCast : DiagGroup<"capability-to-integer-cast">;
 def CheriPedantic : DiagGroup<"cheri-pedantic", [CHERICapabilityToIntegerCast, CHERIPrototypesStrict, CHERIProvenancePedantic]>;

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -2336,6 +2336,15 @@ 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 makes it impossible for callers to detect failure">,
+  InGroup<CHERICompartmentReturnVoid>,
+  DefaultIgnore;
+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">,
+  InGroup<CHERICompartmentReturnVoid>,
+  DefaultIgnore;
 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/AST/Decl.cpp

@@ -3745,8 +3745,29 @@ bool FunctionDecl::doesDeclarationForceExternallyVisibleDefinition() const {
 
 FunctionTypeLoc FunctionDecl::getFunctionTypeLoc() const {
   const TypeSourceInfo *TSI = getTypeSourceInfo();
-  return TSI ? TSI->getTypeLoc().IgnoreParens().getAs<FunctionTypeLoc>()
-             : FunctionTypeLoc();
+  // CHERIOT: The following code diverges from upstream. The previous
+  // code returned a null FunctionTypeLoc when the function was
+  // annotated (impacting getReturnTypeSourceRange too). It is
+  // necessary to provide hints to replace the return type of a
+  // compartment entry point which returns void instead of int.
+  //
+  // https://github.com/llvm/llvm-project/pull/118420
+  if (!TSI)
+    return FunctionTypeLoc();
+
+  TypeLoc TL = TSI->getTypeLoc();
+  FunctionTypeLoc FTL;
+
+  while (!(FTL = TL.getAs<FunctionTypeLoc>())) {
+    if (auto PTL = TL.getAs<ParenTypeLoc>())
+      TL = PTL.getInnerLoc();
+    else if (auto ATL = TL.getAs<AttributedTypeLoc>())
+      TL = ATL.getEquivalentTypeLoc();
+    else
+      break;
+  }
+
+  return FTL;
 }
 
 SourceRange FunctionDecl::getReturnTypeSourceRange() const {

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,28 @@ 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,11 +2388,57 @@ 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) {
+  // 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 using hasDeclarator
+  // (as other handlers do) as Sema::GetTypeForDeclarator only does
+  // the Function Type part.
+  //
+  // BUT because it is a Function Type attribute, when the attribute
+  // is initially attached to the DeclSpec, Sema::GetTypeForDeclarator
+  // distributes it to the first DeclChunk which is a function.  Thus,
+  // the attribute is always be present at the DeclChunk level (but
+  // not always at the DeclSpec level). In order to not possibly
+  // process it twice, we always skip the DeclSpec level.
+  //
+  // **Attention**: The attribute is not always initially attached to
+  // the DeclSpec especially when it follows a pointer:
+  //
+  // - cheri_compartment(...) void f(): attached to DeclSpec
+  // - void * cheri_compartment(...) f(): attached to DeclChunk
+  if (DAL == Sema::DAL_DeclSpec)
+    return;
+
   StringRef Str;
   SourceLocation LiteralLoc;
   if (!S.checkStringLiteralArgumentAttr(Attr, 0, Str, &LiteralLoc))
     return;
+
+  // 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);
+
+    if (SourceRange SR = FD->getReturnTypeSourceRange(); SR.isValid()) {
+      S.Diag(SR.getBegin(), diag::note_cheri_compartment_void_return_type)
+          << FixItHint::CreateReplacement(SR, "int");
+    }
+  } else {
+    if (!S.Diags.isIgnored(diag::warn_cheri_compartment_void_return_type,
+                           LiteralLoc)) {
+      D->addAttr(::new (S.Context) WarnUnusedResultAttr(
+          S.Context, Attr, "CHERI compartment call"));
+    }
+  }
+
   D->addAttr(::new (S.Context) CHERICompartmentNameAttr(S.Context, Attr, Str));
 }
 
@@ -8932,7 +8978,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;
 
@@ -9441,7 +9488,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);
@@ -9755,12 +9802,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
@@ -10011,6 +10059,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));
@@ -10022,13 +10071,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/CodeGen/cheri/cheri-mcu-interrupts.c

@@ -23,10 +23,11 @@ int inherit(void)
 
 // The default for exported functions should be interrupts enabled
 //
-// CHECK: define dso_local chericcallcce void @_Z21default_enable_calleev() local_unnamed_addr addrspace(200) #[[DEFEN:[0-9]]]
+// CHECK: define dso_local chericcallcce i32 @_Z21default_enable_calleev() local_unnamed_addr addrspace(200) #[[DEFEN:[0-9]]]
 __attribute__((cheri_compartment("example")))
-void default_enable_callee(void)
+int default_enable_callee(void)
 {
+  return 0;
 }
 
 // CHECK: define dso_local chericcallcc void @default_enable_callback() local_unnamed_addr addrspace(200) #[[DEFEN]]
@@ -37,11 +38,12 @@ void default_enable_callback(void)
 
 // Explicitly setting interrupt status should override the default
 
-// CHECK: define dso_local chericcallcce void @_Z23explicit_disable_calleev() local_unnamed_addr addrspace(200) #[[EXPDIS:[0-9]]]
+// CHECK: define dso_local chericcallcce i32 @_Z23explicit_disable_calleev() local_unnamed_addr addrspace(200) #[[EXPDIS:[0-9]]]
 __attribute__((cheri_interrupt_state(disabled)))
 __attribute__((cheri_compartment("example")))
-void explicit_disable_callee(void)
+int explicit_disable_callee(void)
 {
+  return 0;
 }
 
 // CHECK: define dso_local chericcallcc void @explicit_disable_callback() local_unnamed_addr addrspace(200) #[[EXPDIS]]

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

@@ -0,0 +1,18 @@
+// 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 -Wcheri-compartment-return-void -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 -Wcheri-compartment-return-void -fdiagnostics-parseable-fixits -fsyntax-only 2>&1 | FileCheck %s
+
+__attribute__((cheri_compartment("example"))) void void_return_type_f(int a) // expected-warning{{void return on a cross-compartment call makes 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;
+}
+
+void unused_int_return_type_f() {
+  int_return_type_f(); // expected-warning{{ignoring return value of function declared with 'nodiscard' attribute: CHERI compartment call}}
+}

clang/unittests/AST/CMakeLists.txt

@@ -36,6 +36,8 @@ add_clang_unittest(ASTTests
   TemplateNameTest.cpp
   TypePrinterTest.cpp
   UnresolvedSetTest.cpp
+
+  cheri/AttrTest.cpp
   )
 
 clang_target_link_libraries(ASTTests