From d6572d95e3048137321996fe122327c9e923b256 Mon Sep 17 00:00:00 2001 From: v01dxyz Date: Sat, 30 Nov 2024 04:24:46 +0100 Subject: [PATCH] Add FixItHints, Improve wording of Diagnosis messages --- .../clang/Basic/DiagnosticSemaKinds.td | 5 ++ clang/include/clang/Sema/Sema.h | 8 +++ clang/lib/Sema/AnalysisBasedWarnings.cpp | 26 +++++++-- clang/lib/Sema/SemaDeclAttr.cpp | 56 +++++++++++++++---- clang/lib/Sema/SemaStmt.cpp | 6 ++ ...ompartment-warn-if-return-void-or-unused.c | 15 ++++- 6 files changed, 99 insertions(+), 17 deletions(-) diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 89070c050bb2..0a49706c9934 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/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">; diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h index 7240a8f3f04f..178f00eb4489 100644 --- a/clang/include/clang/Sema/Sema.h +++ b/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, diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp index 43b13e0ec4d2..88576595c6a6 100644 --- a/clang/lib/Sema/AnalysisBasedWarnings.cpp +++ b/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(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(); } else if (const auto *MD = dyn_cast(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) { diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp index 4a8c67e33b9e..8edbb33865d3 100644 --- a/clang/lib/Sema/SemaDeclAttr.cpp +++ b/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(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()) + TL = ATL.getModifiedLoc(); + + if (auto FTL = TL.getAs()) { + 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); diff --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp index 3589abb004a7..3b0391caafe6 100644 --- a/clang/lib/Sema/SemaStmt.cpp +++ b/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()) { + 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, diff --git a/clang/test/Sema/cheri/cheri-compartment-warn-if-return-void-or-unused.c b/clang/test/Sema/cheri/cheri-compartment-warn-if-return-void-or-unused.c index dd4ea5b2443d..696fdbe0c39b 100644 --- a/clang/test/Sema/cheri/cheri-compartment-warn-if-return-void-or-unused.c +++ b/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;