[Clang] Add support for the C defer TS
#162848
Conversation
Sirraide
added
clang:frontend
|
@llvm/pr-subscribers-clang-codegen Author: None (Sirraide) ChangesI was talking to @AaronBallman about this, and we decided it would make sense to open a PR for this at this point, even if we ultimately decide to defer merging it to a later point in time. Patch is 57.12 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/162848.diff 40 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
index 07bb08166a006..f1b4682c397ab 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
@@ -241,6 +241,12 @@ static bool isIdenticalStmt(const ASTContext &Ctx, const Stmt *Stmt1,
return false;
return true;
}
+ case Stmt::DeferStmtClass: {
+ const auto *DefStmt1 = cast<DeferStmt>(Stmt1);
+ const auto *DefStmt2 = cast<DeferStmt>(Stmt2);
+ return isIdenticalStmt(Ctx, DefStmt1->getBody(), DefStmt2->getBody(),
+ IgnoreSideEffects);
+ }
case Stmt::CompoundStmtClass: {
const auto *CompStmt1 = cast<CompoundStmt>(Stmt1);
const auto *CompStmt2 = cast<CompoundStmt>(Stmt2);
diff --git a/clang/include/clang/AST/RecursiveASTVisitor.h b/clang/include/clang/AST/RecursiveASTVisitor.h
index 1d1b7f183f75a..a7a89e8338af5 100644
--- a/clang/include/clang/AST/RecursiveASTVisitor.h
+++ b/clang/include/clang/AST/RecursiveASTVisitor.h
@@ -2560,6 +2560,7 @@ DEF_TRAVERSE_STMT(DefaultStmt, {})
DEF_TRAVERSE_STMT(DoStmt, {})
DEF_TRAVERSE_STMT(ForStmt, {})
DEF_TRAVERSE_STMT(GotoStmt, {})
+DEF_TRAVERSE_STMT(DeferStmt, {})
DEF_TRAVERSE_STMT(IfStmt, {})
DEF_TRAVERSE_STMT(IndirectGotoStmt, {})
DEF_TRAVERSE_STMT(LabelStmt, {})
diff --git a/clang/include/clang/AST/Stmt.h b/clang/include/clang/AST/Stmt.h
index 76942f1a84f9a..219a99bee8432 100644
--- a/clang/include/clang/AST/Stmt.h
+++ b/clang/include/clang/AST/Stmt.h
@@ -317,6 +317,16 @@ class alignas(void *) Stmt {
SourceLocation KeywordLoc;
};
+ class DeferStmtBitfields {
+ friend class DeferStmt;
+
+ LLVM_PREFERRED_TYPE(StmtBitfields)
+ unsigned : NumStmtBits;
+
+ /// The location of the "defer".
+ SourceLocation DeferLoc;
+ };
+
//===--- Expression bitfields classes ---===//
class ExprBitfields {
@@ -1318,6 +1328,7 @@ class alignas(void *) Stmt {
LoopControlStmtBitfields LoopControlStmtBits;
ReturnStmtBitfields ReturnStmtBits;
SwitchCaseBitfields SwitchCaseBits;
+ DeferStmtBitfields DeferStmtBits;
// Expressions
ExprBitfields ExprBits;
@@ -3232,6 +3243,46 @@ class ReturnStmt final
}
};
+/// DeferStmt - This represents a deferred statement.
+class DeferStmt : public Stmt {
+ friend class ASTStmtReader;
+
+ /// The deferred statement.
+ Stmt *Body;
+
+public:
+ DeferStmt(SourceLocation DeferLoc, Stmt *Body) : Stmt(DeferStmtClass) {
+ setDeferLoc(DeferLoc);
+ setBody(Body);
+ }
+
+ explicit DeferStmt(EmptyShell Empty) : Stmt(DeferStmtClass, Empty) {}
+
+ SourceLocation getDeferLoc() const { return DeferStmtBits.DeferLoc; }
+ void setDeferLoc(SourceLocation DeferLoc) {
+ DeferStmtBits.DeferLoc = DeferLoc;
+ }
+
+ Stmt *getBody() const { return Body; }
+ void setBody(Stmt *S) {
+ assert(S && "defer body must not be null");
+ Body = S;
+ }
+
+ SourceLocation getBeginLoc() const { return getDeferLoc(); }
+ SourceLocation getEndLoc() const { return Body->getEndLoc(); }
+
+ child_range children() { return child_range(&Body, &Body + 1); }
+
+ const_child_range children() const {
+ return const_child_range(&Body, &Body + 1);
+ }
+
+ static bool classof(const Stmt *S) {
+ return S->getStmtClass() == DeferStmtClass;
+ }
+};
+
/// AsmStmt is the base class for GCCAsmStmt and MSAsmStmt.
class AsmStmt : public Stmt {
protected:
diff --git a/clang/include/clang/Basic/DiagnosticParseKinds.td b/clang/include/clang/Basic/DiagnosticParseKinds.td
index 4d9e123eb4ef1..5fd7bdb08fe14 100644
--- a/clang/include/clang/Basic/DiagnosticParseKinds.td
+++ b/clang/include/clang/Basic/DiagnosticParseKinds.td
@@ -347,6 +347,9 @@ def err_address_of_label_outside_fn : Error<
"use of address-of-label extension outside of a function body">;
def err_asm_operand_wide_string_literal : Error<
"cannot use %select{unicode|wide}0 string literal in 'asm'">;
+def err_defer_ts_labeled_stmt
+ : Error<"body of 'defer' statement cannot start with a label">;
+def err_defer_unsupported : Error<"'defer' statements are only supported in C">;
def err_asm_expected_string : Error<
"expected string literal %select{or parenthesized constant expression |}0in 'asm'">;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index bd896524321d1..6b7bd117b990c 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -6793,6 +6793,7 @@ def note_protected_by_objc_weak_init : Note<
"jump bypasses initialization of __weak variable">;
def note_protected_by_non_trivial_c_struct_init : Note<
"jump bypasses initialization of variable of non-trivial C struct type">;
+def note_protected_by_defer_stmt : Note<"jump bypasses defer statement">;
def note_enters_block_captures_cxx_obj : Note<
"jump enters lifetime of block which captures a destructible C++ object">;
def note_enters_block_captures_strong : Note<
@@ -6806,6 +6807,7 @@ def note_enters_compound_literal_scope : Note<
"jump enters lifetime of a compound literal that is non-trivial to destruct">;
def note_enters_statement_expression : Note<
"jump enters a statement expression">;
+def note_enters_defer_stmt : Note<"jump enters a defer statement">;
def note_exits_cleanup : Note<
"jump exits scope of variable with __attribute__((cleanup))">;
@@ -6851,6 +6853,15 @@ def note_exits_block_captures_non_trivial_c_struct : Note<
"to destroy">;
def note_exits_compound_literal_scope : Note<
"jump exits lifetime of a compound literal that is non-trivial to destruct">;
+def note_exits_defer_stmt : Note<"jump exits a defer statement">;
+def err_jump_out_of_defer_stmt
+ : Error<"cannot %enum_select<DeferJumpKind>{"
+ "%Break{break out of a}|"
+ "%Continue{continue loop outside of enclosing}|"
+ "%Return{return from a}|"
+ "%SEHLeave{__leave a}"
+ "}0 defer statement">;
+def err_defer_invalid_sjlj : Error<"cannot use %0 inside a defer statement">;
def err_func_returning_qualified_void : ExtWarn<
"function cannot return qualified void type %0">,
@@ -10926,6 +10937,8 @@ def err_switch_explicit_conversion : Error<
def err_switch_incomplete_class_type : Error<
"switch condition has incomplete class type %0">;
+// TODO: It ought to be possible to refactor these to be a single warning that
+// uses %enum_select.
def warn_empty_if_body : Warning<
"if statement has empty body">, InGroup<EmptyBody>;
def warn_empty_for_body : Warning<
@@ -10936,6 +10949,8 @@ def warn_empty_while_body : Warning<
"while loop has empty body">, InGroup<EmptyBody>;
def warn_empty_switch_body : Warning<
"switch statement has empty body">, InGroup<EmptyBody>;
+def warn_empty_defer_body : Warning<"defer statement has empty body">,
+ InGroup<EmptyBody>;
def note_empty_body_on_separate_line : Note<
"put the semicolon on a separate line to silence this warning">;
diff --git a/clang/include/clang/Basic/LangOptions.def b/clang/include/clang/Basic/LangOptions.def
index 84f5ab3443a59..3ba281a68fcb3 100644
--- a/clang/include/clang/Basic/LangOptions.def
+++ b/clang/include/clang/Basic/LangOptions.def
@@ -194,6 +194,7 @@ LANGOPT(NoSignedZero , 1, 0, Benign, "Permit Floating Point optimization wi
LANGOPT(AllowRecip , 1, 0, Benign, "Permit Floating Point reciprocal")
LANGOPT(ApproxFunc , 1, 0, Benign, "Permit Floating Point approximation")
LANGOPT(NamedLoops , 1, 0, Benign, "Permit named break/continue")
+LANGOPT(DeferTS , 1, 0, Benign, "C 'defer' Technical Specification")
ENUM_LANGOPT(ComplexRange, ComplexRangeKind, 3, CX_None, NotCompatible, "Enable use of range reduction for complex arithmetics.")
diff --git a/clang/include/clang/Basic/StmtNodes.td b/clang/include/clang/Basic/StmtNodes.td
index dd1a24405fae7..04f8f2ee1687e 100644
--- a/clang/include/clang/Basic/StmtNodes.td
+++ b/clang/include/clang/Basic/StmtNodes.td
@@ -17,6 +17,7 @@ def ForStmt : StmtNode<Stmt>;
def GotoStmt : StmtNode<Stmt>;
def IndirectGotoStmt : StmtNode<Stmt>;
def ReturnStmt : StmtNode<Stmt>;
+def DeferStmt : StmtNode<Stmt>;
def DeclStmt : StmtNode<Stmt>;
def SwitchCase : StmtNode<Stmt, 1>;
def CaseStmt : StmtNode<SwitchCase>;
diff --git a/clang/include/clang/Basic/TokenKinds.def b/clang/include/clang/Basic/TokenKinds.def
index 9d1a23d1af218..436b1a3756dc1 100644
--- a/clang/include/clang/Basic/TokenKinds.def
+++ b/clang/include/clang/Basic/TokenKinds.def
@@ -293,6 +293,7 @@ PUNCTUATOR(greatergreatergreater, ">>>")
// CHAR8SUPPORT - This is a keyword if 'char8_t' is a built-in type
// KEYFIXEDPOINT - This is a keyword according to the N1169 fixed point
// extension.
+// KEYDEFERTS - This is a keyword if the C 'defer' TS is enabled
// KEYZOS - This is a keyword in C/C++ on z/OS
//
KEYWORD(auto , KEYALL)
@@ -441,6 +442,9 @@ KEYWORD(_Float16 , KEYALL)
C23_KEYWORD(typeof , KEYGNU)
C23_KEYWORD(typeof_unqual , 0)
+// 'defer' TS
+KEYWORD(defer , KEYDEFERTS)
+
// ISO/IEC JTC1 SC22 WG14 N1169 Extension
KEYWORD(_Accum , KEYFIXEDPOINT)
KEYWORD(_Fract , KEYFIXEDPOINT)
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 16e1c396fedbe..28429c3aff83e 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -1653,6 +1653,14 @@ defm named_loops
PosFlag<SetTrue, [], [CC1Option], "Enable support for named loops">,
NegFlag<SetFalse>>;
+// C 'defer' TS
+defm defer_ts
+ : BoolFOption<
+ "defer-ts", LangOpts<"DeferTS">, DefaultFalse,
+ PosFlag<SetTrue, [], [ClangOption, CC1Option],
+ "Enable support for the C 'defer' Technical Specification">,
+ NegFlag<SetFalse>>;
+
// C++ Coroutines
defm coroutines : BoolFOption<"coroutines",
LangOpts<"Coroutines">, Default<cpp20.KeyPath>,
diff --git a/clang/include/clang/Parse/Parser.h b/clang/include/clang/Parse/Parser.h
index 30edd303e1824..52d8a0238cb2a 100644
--- a/clang/include/clang/Parse/Parser.h
+++ b/clang/include/clang/Parse/Parser.h
@@ -7501,6 +7501,16 @@ class Parser : public CodeCompletionHandler {
StmtResult ParseBreakOrContinueStatement(bool IsContinue);
+ /// ParseDeferStatement
+ /// \verbatim
+ /// defer-statement:
+ /// 'defer' deferred-block
+ ///
+ /// deferred-block:
+ /// unlabeled-statement
+ /// \endverbatim
+ StmtResult ParseDeferStatement(SourceLocation *TrailingElseLoc);
+
StmtResult ParsePragmaLoopHint(StmtVector &Stmts, ParsedStmtContext StmtCtx,
SourceLocation *TrailingElseLoc,
ParsedAttributes &Attrs,
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index d017d1f829015..1634ccf97f603 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -10915,6 +10915,10 @@ class Sema final : public SemaBase {
/// Stack of active SEH __finally scopes. Can be empty.
SmallVector<Scope *, 2> CurrentSEHFinally;
+ /// Stack of 'defer' statements that are currently being parsed, as well
+ /// as the locations of their 'defer' keywords. Can be empty.
+ SmallVector<std::pair<Scope *, SourceLocation>, 2> CurrentDefer;
+
StmtResult ActOnExprStmt(ExprResult Arg, bool DiscardedValue = true);
StmtResult ActOnExprStmtError();
@@ -11061,6 +11065,10 @@ class Sema final : public SemaBase {
StmtResult ActOnBreakStmt(SourceLocation BreakLoc, Scope *CurScope,
LabelDecl *Label, SourceLocation LabelLoc);
+ void ActOnStartOfDeferStmt(SourceLocation DeferLoc, Scope *CurScope);
+ void ActOnDeferStmtError(Scope *CurScope);
+ StmtResult ActOnEndOfDeferStmt(Stmt *Body, Scope *CurScope);
+
struct NamedReturnInfo {
const VarDecl *Candidate;
diff --git a/clang/include/clang/Serialization/ASTBitCodes.h b/clang/include/clang/Serialization/ASTBitCodes.h
index 441047d64f48c..b287539681ded 100644
--- a/clang/include/clang/Serialization/ASTBitCodes.h
+++ b/clang/include/clang/Serialization/ASTBitCodes.h
@@ -2060,6 +2060,7 @@ enum StmtCode {
// HLSL Constructs
EXPR_HLSL_OUT_ARG,
+ STMT_DEFER,
};
/// The kinds of designators that can occur in a
diff --git a/clang/lib/AST/StmtPrinter.cpp b/clang/lib/AST/StmtPrinter.cpp
index 2c9c3581a2962..843110bc93f59 100644
--- a/clang/lib/AST/StmtPrinter.cpp
+++ b/clang/lib/AST/StmtPrinter.cpp
@@ -491,6 +491,11 @@ void StmtPrinter::VisitBreakStmt(BreakStmt *Node) {
if (Policy.IncludeNewlines) OS << NL;
}
+void StmtPrinter::VisitDeferStmt(DeferStmt *Node) {
+ Indent() << "defer";
+ PrintControlledStmt(Node->getBody());
+}
+
void StmtPrinter::VisitReturnStmt(ReturnStmt *Node) {
Indent() << "return";
if (Node->getRetValue()) {
diff --git a/clang/lib/AST/StmtProfile.cpp b/clang/lib/AST/StmtProfile.cpp
index 37c4d43ec0b2f..10794d9dcba26 100644
--- a/clang/lib/AST/StmtProfile.cpp
+++ b/clang/lib/AST/StmtProfile.cpp
@@ -323,6 +323,8 @@ void StmtProfiler::VisitReturnStmt(const ReturnStmt *S) {
VisitStmt(S);
}
+void StmtProfiler::VisitDeferStmt(const DeferStmt *S) { VisitStmt(S); }
+
void StmtProfiler::VisitGCCAsmStmt(const GCCAsmStmt *S) {
VisitStmt(S);
ID.AddBoolean(S->isVolatile());
diff --git a/clang/lib/Basic/IdentifierTable.cpp b/clang/lib/Basic/IdentifierTable.cpp
index 4a2b77cd16bfc..79f9eb696d9e8 100644
--- a/clang/lib/Basic/IdentifierTable.cpp
+++ b/clang/lib/Basic/IdentifierTable.cpp
@@ -110,7 +110,8 @@ enum TokenKey : unsigned {
KEYNOZOS = 0x4000000,
KEYHLSL = 0x8000000,
KEYFIXEDPOINT = 0x10000000,
- KEYMAX = KEYFIXEDPOINT, // The maximum key
+ KEYDEFERTS = 0x20000000,
+ KEYMAX = KEYDEFERTS, // The maximum key
KEYALLCXX = KEYCXX | KEYCXX11 | KEYCXX20,
KEYALL = (KEYMAX | (KEYMAX - 1)) & ~KEYNOMS18 & ~KEYNOOPENCL &
~KEYNOZOS // KEYNOMS18, KEYNOOPENCL, KEYNOZOS are excluded.
@@ -215,6 +216,8 @@ static KeywordStatus getKeywordStatusHelper(const LangOptions &LangOpts,
return KS_Unknown;
case KEYFIXEDPOINT:
return LangOpts.FixedPoint ? KS_Enabled : KS_Disabled;
+ case KEYDEFERTS:
+ return LangOpts.DeferTS ? KS_Enabled : KS_Disabled;
default:
llvm_unreachable("Unknown KeywordStatus flag");
}
diff --git a/clang/lib/CodeGen/CGStmt.cpp b/clang/lib/CodeGen/CGStmt.cpp
index aeff73d525c10..1e909b6dfafc4 100644
--- a/clang/lib/CodeGen/CGStmt.cpp
+++ b/clang/lib/CodeGen/CGStmt.cpp
@@ -114,6 +114,7 @@ void CodeGenFunction::EmitStmt(const Stmt *S, ArrayRef<const Attr *> Attrs) {
case Stmt::ContinueStmtClass:
case Stmt::DefaultStmtClass:
case Stmt::CaseStmtClass:
+ case Stmt::DeferStmtClass:
case Stmt::SEHLeaveStmtClass:
case Stmt::SYCLKernelCallStmtClass:
llvm_unreachable("should have emitted these statements as simple");
@@ -536,6 +537,9 @@ bool CodeGenFunction::EmitSimpleStmt(const Stmt *S,
case Stmt::CaseStmtClass:
EmitCaseStmt(cast<CaseStmt>(*S), Attrs);
break;
+ case Stmt::DeferStmtClass:
+ EmitDeferStmt(cast<DeferStmt>(*S));
+ break;
case Stmt::SEHLeaveStmtClass:
EmitSEHLeaveStmt(cast<SEHLeaveStmt>(*S));
break;
@@ -1997,6 +2001,21 @@ void CodeGenFunction::EmitDefaultStmt(const DefaultStmt &S,
EmitStmt(S.getSubStmt());
}
+namespace {
+struct EmitDeferredStatement final : EHScopeStack::Cleanup {
+ const DeferStmt &Stmt;
+ EmitDeferredStatement(const DeferStmt *Stmt) : Stmt(*Stmt) {}
+
+ void Emit(CodeGenFunction &CGF, Flags flags) override {
+ CGF.EmitStmt(Stmt.getBody());
+ }
+};
+} // namespace
+
+void CodeGenFunction::EmitDeferStmt(const DeferStmt &S) {
+ EHStack.pushCleanup<EmitDeferredStatement>(NormalAndEHCleanup, &S);
+}
+
/// CollectStatementsForCase - Given the body of a 'switch' statement and a
/// constant value that is being switched on, see if we can dead code eliminate
/// the body of the switch to a simple series of statements to emit. Basically,
diff --git a/clang/lib/CodeGen/CodeGenFunction.h b/clang/lib/CodeGen/CodeGenFunction.h
index 727487b46054f..5e032eae37f11 100644
--- a/clang/lib/CodeGen/CodeGenFunction.h
+++ b/clang/lib/CodeGen/CodeGenFunction.h
@@ -3606,6 +3606,7 @@ class CodeGenFunction : public CodeGenTypeCache {
void EmitDefaultStmt(const DefaultStmt &S, ArrayRef<const Attr *> Attrs);
void EmitCaseStmt(const CaseStmt &S, ArrayRef<const Attr *> Attrs);
void EmitCaseStmtRange(const CaseStmt &S, ArrayRef<const Attr *> Attrs);
+ void EmitDeferStmt(const DeferStmt &S);
void EmitAsmStmt(const AsmStmt &S);
const BreakContinue *GetDestForLoopControlStmt(const LoopControlStmt &S);
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index f67454ee517bd..169127a71e76e 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -7066,6 +7066,9 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,
types::isCXX(InputType))
CmdArgs.push_back("-fcoro-aligned-allocation");
+ if (Args.hasFlag(options::OPT_fdefer_ts, options::OPT_fno_defer_ts, false))
+ CmdArgs.push_back("-fdefer-ts");
+
Args.AddLastArg(CmdArgs, options::OPT_fdouble_square_bracket_attributes,
options::OPT_fno_double_square_bracket_attributes);
diff --git a/clang/lib/Frontend/InitPreprocessor.cpp b/clang/lib/Frontend/InitPreprocessor.cpp
index edf0a091e087c..3c4b2fbfb8760 100644
--- a/clang/lib/Frontend/InitPreprocessor.cpp
+++ b/clang/lib/Frontend/InitPreprocessor.cpp
@@ -529,6 +529,9 @@ static void InitializeStandardPredefinedMacros(const TargetInfo &TI,
Builder.defineMacro("__STDC_EMBED_EMPTY__",
llvm::itostr(static_cast<int>(EmbedResult::Empty)));
+ if (LangOpts.DeferTS)
+ Builder.defineMacro("__STDC_DEFER_TS25755__", "1");
+
if (LangOpts.ObjC)
Builder.defineMacro("__OBJC__");
diff --git a/clang/lib/Parse/ParseStmt.cpp b/clang/lib/Parse/ParseStmt.cpp
index 2e7af1219547e..c18c3067b217b 100644
--- a/clang/lib/Parse/ParseStmt.cpp
+++ b/clang/lib/Parse/ParseStmt.cpp
@@ -28,6 +28,7 @@
#include "clang/Sema/SemaOpenMP.h"
#include "clang/Sema/TypoCorrection.h"
#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/ScopeExit.h"
#include <optional>
using namespace clang;
@@ -312,6 +313,10 @@ StmtResult Parser::ParseStatementOrDeclarationAfterAttributes(
Res = ParseReturnStatement();
SemiError = "co_return";
break;
+ case tok::kw_defer: // C defer TS: defer-statement
+ ProhibitAttributes(GNUAttrs);
+ ProhibitAttributes(CXX11Attrs);
+ return ParseDeferStatement(TrailingElseLoc);
case tok::kw_asm: {
for (const ParsedAttr &AL : CXX11Attrs)
@@ -2376,6 +2381,33 @@ StmtResult Parser::ParseReturnStatement() {
return Actions.ActOnReturnStmt(ReturnLoc, R.get(), getCurScope());
}
+StmtResult Parser::ParseDeferStatement(SourceLocation *TrailingElseLoc) {
+ assert(Tok.is(tok::kw_defer));
+ SourceLocation DeferLoc = ConsumeToken();
+ Actions.ActOnStartOfDeferStmt(DeferLoc, getCurScope());
+ auto OnError = llvm::make_scope_exit(
+ [&] { Actions.ActOnDeferStmtError(getCurScope()); });
+
+ StmtResult Res = ParseStatement(TrailingElseLoc);
+ if (!Res.isUsable())
+ return StmtError();
+
+ // Diagnose this *after* parsing the body for better synchronisation.
+ if (getLangOpts().CPlusPlus) {
+ Diag(DeferLoc, diag::err_defer_unsupported);
+ return StmtError();
+ }
+
+ // The grammar specifically calls for an unlabeled-statement here.
+ if (auto *L = dyn_cast<LabelStmt>(Res.get())) {
+ Diag(L->getIdentLoc(), diag::err_defer_ts_labeled_stmt);
+ return StmtError();
+ }
+
+ OnError.release();
+ return Actions.ActOnEndOfDeferStmt(Res.get(), getCurScope());
+}
+
StmtResult Parser::ParsePragmaLoopHint(StmtVector &Stmts,
ParsedStmtContext StmtCtx,
SourceLocation *TrailingElseLoc,
diff --git a/clang/lib/Sema/JumpDiagnostics.cpp b/clang/lib/Sema/JumpDiagnostics.cpp
index 36704c3826dfd..36c9d9afb37f1 100644
--- a/clang/lib/Sema/JumpDiagnostics.cpp
+++ b/clang/lib/Sema/JumpDiagnostics.cpp
@@ -590,6 +590,27 @@ void Jum...
[truncated]
|
|
its gonna be fun |
|
The current wording of the defer TS draft allows a defer statement to appear directly as the body of a selection or iteration statement, and the implementation in this PR behaves accordingly. That is, code like the following is permitted: Since the body of a selection or iteration statement has its own scope as a secondary block (in C99 and later), the effect is that the deferred statement is executed as soon as execution reaches the (Example 4 in the TS drafts shows this behavior.) Personally, I think allowing First, it may be desirable to give a warning when Second, the TS is specified only as an extension to C23, but the code in this PR also allows |
I agree that it is not particularly useful to use Most generally speaking, disallowing patterns like these means more work on the implementation side (since we’d have to implement, test, and maintain these artificial restrictions), and I’m personally somewhat doubtful that such warnings would really be all that helpful.
That is something you’d have to raise with the TS author.
I candidly don’t think we care to support C90’s weird scoping rules here, but @AaronBallman should have a more informed opinion on this matter. In my opinion, if someone wants to use |
A loop might do all sorts of things that you would want to defer cleanup for, and someone who didn't know about this block scope behavior of defer might expect something like this to work: while (cond) {
file_array[i++] = fopen(...);
defer fclose(file_array(i-1));
}
do_stuff_with(file_array);
return; |
Sure, but that is also a legitimate use case for while (true) {
int socket = accept(...);
defer close(socket);
// do something w/ socket
} |
|
As I understand the TS, your example is not disallowed and will work fine. The |
Yes, that’s correct. |
There was a problem hiding this comment.
Thank you for working on this @Sirraide!
I took an initial pass over the PR and it's heading in the direction I'd expect it to go. Some things I think are missing:
- Release note
- Documentation that we support the TS and in which language modes (we may want to consider C89 support in particular due to the difference in scoping rules).
- Update https://clang.llvm.org/c_status.html to list the TS once its published and our status with it
- A PCH test to ensure serialization works as expected
- Do we want to do anything special for the reserved identifier warning for
defer?
| DeferStmt(SourceLocation DeferLoc, Stmt *Body) : Stmt(DeferStmtClass) { | ||
| setDeferLoc(DeferLoc); | ||
| setBody(Body); | ||
| } | ||
|
|
||
| explicit DeferStmt(EmptyShell Empty) : Stmt(DeferStmtClass, Empty) {} |
There was a problem hiding this comment.
Should we go with the Create pattern like we have for other AST nodes?
There was a problem hiding this comment.
Should we go with the
Createpattern like we have for other AST nodes?
I think that’s mainly there for AST nodes that have trailing data (because for those you have to allocate more storage after the end of the object), but DeferStmt has no trailing data. We also don’t have a Create() function for some of the other AST nodes iirc.
There was a problem hiding this comment.
Yeah, as usual we're consistently inconsistent. :-) I don't insist on a change, but I think we usually go with the Create method by default, the others seem more accidental.
| @@ -0,0 +1,5 @@ | |||
| // RUN: %clang_cc1 -fsyntax-only -verify %s | |||
| // RUN: %clang_cc1 -fsyntax-only -verify -fdefer-ts %s | |||
There was a problem hiding this comment.
Should we warn when trying to use the defer TS outside of a C TU?
There was a problem hiding this comment.
Currently, this is using ShouldParseIf as per Corentin’s suggestion, which just ignores the option entirely; do you happen to know if there is a way to specify in Options.td that it should error/warn instead? We can also do that manually I suppose though.
There was a problem hiding this comment.
I was thinking something manual; I don't know of a way to automatically handle that in tablegen. CC @jansvoboda11 @MaskRay in case I'm missing something
There was a problem hiding this comment.
The Options.td machinery doesn't allow generating complex diagnostics like the one you're after. We'd typically diagnose flag incompatibility in the driver, but if you have a reason to do so in the frontend, we typically add these into FixupInvocation in CompilerInvocation.cpp.
| defer b: {} // expected-error {{body of 'defer' statement cannot start with a label}} | ||
| defer { c: g(); } | ||
|
|
||
| if (g()) defer g(); |
There was a problem hiding this comment.
Do we think we should have a warning for this kind of code where the defer isn't actually deferred at all because the scope is immediately exited? I can see folks getting into problems with code like:
void *ptr = malloc(12);
if (ptr) defer free(ptr);
when they really meant to write:
void *ptr = malloc(12);
defer free(ptr);
or
void *ptr = malloc(12);
defer if(ptr) free(ptr);
instead. This might be out of scope for the initial implementation, and it might make more sense as a static analysis check because it'll likely require control flow analysis.
There was a problem hiding this comment.
Someone else also suggested that; one thing we could do is check if the body of an if statement is a defer statement, but that wouldn’t handle cases such as if (foo) { defer ... };, so yeah, that should probably be handled in the static analyser instead.
|
|
||
| // Not exposed for now because 'defer' is currently just a TS. | ||
| case Stmt::DeferStmtClass: | ||
| K = CXCursor_UnexposedStmt; |
There was a problem hiding this comment.
We don't have to do it for the initial patch, but this should still return a valid cursor at some point even though it's just a TS. But that can be done later IMO.
|
Adding some more reviewers for a look at codegen behavior |
|
@AaronBallman Is there any sort of stable link for the TS that I can put in the release notes in case it gets updated later (something like wg21.link)? |
|
For structs returned by-value, clang currently would apply RVO, which in effect reorders evaluation of the return expression to be after defer statements. #include <stdio.h>
typedef struct {
int i;
} S;
int test_int(void) {
S s = {.i = 22};
defer { s.i += 7; }
return s.i;
}
S test_struct(void) {
S s = {.i = 22};
defer { s.i += 7; }
return s;
}
int main() {
printf("%d\n", test_int()); // got 22
printf("%d\n", test_struct().i); // expected 22, got 29
}The expected result can be obtained by passing |
|
This one is probably miscompilation: should print #include <stdio.h>
void loop_in_defer_block(void) {
for (int i = 0;; i++) {
defer {
for (int j = 0; j < 1; j++) {
printf("A");
}
}
if (i == 1) {
printf("B");
break;
}
printf("C");
}
}
int main(void) {
loop_in_defer_block();
puts("");
} |
I’m not surprised we’re running into the same issue here considering that the codegen for defer is basically the same as for the cleanup attribute; at a glance, the fix for this would be to disable RVO in C if any cleanups are present, but this should be a separate patch. |
At a glance the IR Clang generates for this is fine but I’ll look into it a bit more |
So this one looks like it’s Clang doing some weird stuff with cleanups when optimisations are enabled because we’re dropping one of the branches in the cleanup |
Somewhat reduced: int putchar(int c);
void loop_in_defer_block() {
for (int i = 0;; i++) {
defer {
for (int j = 0; j != 1; j++) {
putchar('A');
}
}
if (i == 1) break;
}
}The codegen for this (w/ define dso_local void @loop_in_defer_block() {
entry:
%i = alloca i32, align 4
%cleanup.dest.slot = alloca i32, align 4
%j = alloca i32, align 4
call void @llvm.lifetime.start.p0(ptr %i)
store i32 0, ptr %i, align 4
br label %for.cond
for.cond: ; preds = %for.inc4, %entry
%0 = load i32, ptr %i, align 4
%cmp = icmp eq i32 %0, 1
br i1 %cmp, label %if.then, label %if.end
if.then: ; preds = %for.cond
store i32 2, ptr %cleanup.dest.slot, align 4
br label %cleanup
if.end: ; preds = %for.cond
store i32 0, ptr %cleanup.dest.slot, align 4
br label %cleanup
cleanup: ; preds = %if.end, %if.then
call void @llvm.lifetime.start.p0(ptr %j)
store i32 0, ptr %j, align 4
br label %for.cond1
for.cond1: ; preds = %for.inc, %cleanup
%1 = load i32, ptr %j, align 4
%cmp2 = icmp ne i32 %1, 1
br i1 %cmp2, label %for.body, label %for.cond.cleanup
for.cond.cleanup: ; preds = %for.cond1
store i32 5, ptr %cleanup.dest.slot, align 4
call void @llvm.lifetime.end.p0(ptr %j)
br label %for.end
for.body: ; preds = %for.cond1
%call = call i32 @putchar(i32 noundef 65)
br label %for.inc
for.inc: ; preds = %for.body
%2 = load i32, ptr %j, align 4
%inc = add nsw i32 %2, 1
store i32 %inc, ptr %j, align 4
br label %for.cond1
for.end: ; preds = %for.cond.cleanup
%cleanup.dest = load i32, ptr %cleanup.dest.slot, align 4
switch i32 %cleanup.dest, label %cleanup6 [
i32 0, label %cleanup.cont
]
cleanup.cont: ; preds = %for.end
br label %for.inc4
for.inc4: ; preds = %cleanup.cont
%3 = load i32, ptr %i, align 4
%inc5 = add nsw i32 %3, 1
store i32 %inc5, ptr %i, align 4
br label %for.cond
cleanup6: ; preds = %for.end
call void @llvm.lifetime.end.p0(ptr %i)
br label %for.end7
for.end7: ; preds = %cleanup6
ret void
}
declare void @llvm.lifetime.start.p0(ptr captures(none))
declare i32 @putchar(i32 noundef)
declare void @llvm.lifetime.end.p0(ptr captures(none))which doesn’t seem right since this does indeed only call |
|
Interestingly, passing |
CC @usx95 |
Normally we'd have a link to the closest draft to the published TS on the WG14 website, but since this isn't yet published, we don't have anything better than the latest N document to point to. |
Specifically, the cleanup that is pushed in order to emit the call to |
CC @erichkeane @rjmccall @efriedma-quic Maybe you have an idea as to what’s going on here, because I’m personally not too familiar w/ the cleanup code... |
|
We talked about this PR a bit in my office hours yesterday and one thing which came up for feedback to WG14 is that this should be spelled |
11 participants
I was talking to @AaronBallman about this, and we decided it would make sense to open a PR for this at this point, even if we ultimately decide to defer merging it to a later point in time.