Author: r4nt Date: 2026-10-21T20:59:11+02:00 New Revision: 3743f56d187182e0dc5a6c4eb373dd9d3e7251a6
URL: https://github.com/llvm/llvm-project/commit/3743f56d187182e0dc5a6c4eb373dd9d3e7251a6 DIFF: https://github.com/llvm/llvm-project/commit/3743f56d187182e0dc5a6c4eb373dd9d3e7251a6.diff LOG: [clang-format] Fix macro expansion crashes and raw string indentation (#233758) Fix three issues with clang-format's macro expansion (FormatStyle::Macros): 1. Guard MatchingOpeningBlockLineIndex / MatchingClosingBlockLineIndex during macro expansion: When a macro expands to a multi-line block (such as DECL_STRUCT(Name)=struct Name {}), UnwrappedLineParser::addUnwrappedLine pushes the expanded lines into CurrentExpandedLines rather than CurrentLines. However, parseBlock() and parseEnum() still computed OpeningLineIndex from CurrentLines->size() - 1, recording an unrelated or out-of-bounds line index in MatchingOpeningBlockLineIndex and MatchingClosingBlockLineIndex and crashing DefinitionBlockSeparator / NamespaceEndCommentsFixer. Only record block line indices when CurrentExpandedLines is empty and the closing line is not part of a macro expansion. 2. Annotate and analyze each run immediately in TokenAnalyzer::finishRun(): FormatToken objects in AllTokens are shared across preprocessor branch runs in UnwrappedLineParser::parse(), and UnwrappedLineParser::reset() resets Token->MacroCtx at the start of each branch. Previously, TokenAnalyzer stored a nested vector of runs (SmallVector<SmallVector<UnwrappedLine, 16>, 2> UnwrappedLines), appending a new inner vector on each finishRun() while Parser.parse() parsed all #if / #else branches, and only iterated over UnwrappedLines to annotate and analyze after Parser.parse() returned. Because later #if / #else branches reset Token->MacroCtx and mutated shared FormatTokens during parsing, earlier runs had their token state clobbered before they were ever annotated or analyzed. Instead, keep only the current run's lines in a flat SmallVector<UnwrappedLine, 16> UnwrappedLines, invoke OnFinishRun() inside finishRun() to annotate and analyze the current run immediately before the parser moves to the next run/branch, and then clear UnwrappedLines. 3. Indent multi-line raw string literals in unexpanded macro arguments: When a multi-line raw string literal (e.g. R"pb(...)pb") is passed as a macro argument, ContinuationIndenter::handleEndOfLine previously reformatted the raw string literal during the expanded pass (MR_ExpandedArg) using the expanded statement's indentation, and UnwrappedLineFormatter did not update ParenState::NestedBlockIndent after formatting a MacroParent's children in the unexpanded pass. Skip reformatRawStringLiteral when Role == MR_ExpandedArg and update NestedBlockIndent after formatting MacroParent children. Fixes #78263 Fixes #233300 Added: Modified: clang/lib/Format/ContinuationIndenter.cpp clang/lib/Format/TokenAnalyzer.cpp clang/lib/Format/TokenAnalyzer.h clang/lib/Format/UnwrappedLineFormatter.cpp clang/lib/Format/UnwrappedLineParser.cpp clang/unittests/Format/FormatTestMacroExpansion.cpp Removed: ################################################################################ diff --git a/clang/lib/Format/ContinuationIndenter.cpp b/clang/lib/Format/ContinuationIndenter.cpp index b69eca38a8ef9..98232bb16cc04 100644 --- a/clang/lib/Format/ContinuationIndenter.cpp +++ b/clang/lib/Format/ContinuationIndenter.cpp @@ -2558,7 +2558,8 @@ unsigned ContinuationIndenter::handleEndOfLine(const FormatToken &Current, // Compute the raw string style to use in case this is a raw string literal // that can be reformatted. auto RawStringStyle = getRawStringStyle(Current, State); - if (RawStringStyle && !Current.Finalized) { + if (RawStringStyle && !Current.Finalized && + (!Current.MacroCtx || Current.MacroCtx->Role != MR_ExpandedArg)) { Penalty = reformatRawStringLiteral(Current, State, *RawStringStyle, DryRun, Newline); } else if (Current.IsMultiline && Current.isNot(TT_BlockComment)) { diff --git a/clang/lib/Format/TokenAnalyzer.cpp b/clang/lib/Format/TokenAnalyzer.cpp index 6ba3b05312607..73e567ce11538 100644 --- a/clang/lib/Format/TokenAnalyzer.cpp +++ b/clang/lib/Format/TokenAnalyzer.cpp @@ -83,7 +83,6 @@ Environment::Environment(StringRef Code, StringRef FileName, TokenAnalyzer::TokenAnalyzer(const Environment &Env, const FormatStyle &Style) : Style(Style), LangOpts(getFormattingLangOpts(Style)), Env(Env), AffectedRangeMgr(Env.getSourceManager(), Env.getCharRanges()), - UnwrappedLines(1), Encoding(encoding::detectEncoding( Env.getSourceManager().getBufferData(Env.getFileID()))) { LLVM_DEBUG( @@ -104,20 +103,19 @@ TokenAnalyzer::process(bool SkipAnnotation) { IdentTable); ArrayRef<FormatToken *> Toks(Lex.lex()); SmallVector<FormatToken *, 10> Tokens(Toks); - UnwrappedLineParser Parser(Env.getSourceManager(), Style, Lex.getKeywords(), - Env.getFirstStartColumn(), Tokens, *this, - Allocator, IdentTable); - Parser.parse(); - assert(UnwrappedLines.back().empty()); + unsigned Penalty = 0; - for (unsigned Run = 0, RunE = UnwrappedLines.size(); Run + 1 != RunE; ++Run) { - const auto &Lines = UnwrappedLines[Run]; + unsigned Run = 0; + bool HasError = false; + OnFinishRun = [&]() { + if (HasError) + return; LLVM_DEBUG(llvm::dbgs() << "Run " << Run << "...\n"); SmallVector<AnnotatedLine *, 16> AnnotatedLines; - AnnotatedLines.reserve(Lines.size()); + AnnotatedLines.reserve(UnwrappedLines.size()); TokenAnnotator Annotator(Style, Lex.getKeywords()); - for (const UnwrappedLine &Line : Lines) { + for (const UnwrappedLine &Line : UnwrappedLines) { AnnotatedLines.push_back(new AnnotatedLine(Line)); if (!SkipAnnotation) Annotator.annotate(*AnnotatedLines.back()); @@ -133,6 +131,7 @@ TokenAnalyzer::process(bool SkipAnnotation) { }); for (AnnotatedLine *Line : AnnotatedLines) delete Line; + ++Run; Penalty += RunResult.second; for (const auto &R : RunResult.first) { @@ -141,20 +140,27 @@ TokenAnalyzer::process(bool SkipAnnotation) { // Replacements to indicate failure. if (Err) { llvm::errs() << llvm::toString(std::move(Err)) << "\n"; - return {tooling::Replacements(), 0}; + HasError = true; + return; } } - } + }; + UnwrappedLineParser Parser(Env.getSourceManager(), Style, Lex.getKeywords(), + Env.getFirstStartColumn(), Tokens, *this, + Allocator, IdentTable); + Parser.parse(); + if (HasError) + return {tooling::Replacements(), 0}; return {Result, Penalty}; } void TokenAnalyzer::consumeUnwrappedLine(const UnwrappedLine &TheLine) { - assert(!UnwrappedLines.empty()); - UnwrappedLines.back().push_back(TheLine); + UnwrappedLines.push_back(TheLine); } void TokenAnalyzer::finishRun() { - UnwrappedLines.push_back(SmallVector<UnwrappedLine, 16>()); + OnFinishRun(); + UnwrappedLines.clear(); } } // end namespace format diff --git a/clang/lib/Format/TokenAnalyzer.h b/clang/lib/Format/TokenAnalyzer.h index ef559099d325d..40f95684e10dc 100644 --- a/clang/lib/Format/TokenAnalyzer.h +++ b/clang/lib/Format/TokenAnalyzer.h @@ -97,7 +97,8 @@ class TokenAnalyzer : public UnwrappedLineConsumer { const Environment &Env; // AffectedRangeMgr stores ranges to be fixed. AffectedRangeManager AffectedRangeMgr; - SmallVector<SmallVector<UnwrappedLine, 16>, 2> UnwrappedLines; + SmallVector<UnwrappedLine, 16> UnwrappedLines; + std::function<void()> OnFinishRun; encoding::Encoding Encoding; }; diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp b/clang/lib/Format/UnwrappedLineFormatter.cpp index 304d8649f2e81..2be02c8eea36a 100644 --- a/clang/lib/Format/UnwrappedLineFormatter.cpp +++ b/clang/lib/Format/UnwrappedLineFormatter.cpp @@ -1246,13 +1246,15 @@ class LineFormatter { } if (NewLine || Previous.MacroParent) { - const ParenState &P = State.Stack.back(); + ParenState &P = State.Stack.back(); int AdditionalIndent = P.Indent.Total - Previous.Children[0]->Level * Style.IndentWidth; Penalty += BlockFormatter->format(Previous.Children, DryRun, AdditionalIndent, /*FixBadIndentation=*/true); + if (Previous.MacroParent) + P.NestedBlockIndent = P.Indent.Total; return true; } diff --git a/clang/lib/Format/UnwrappedLineParser.cpp b/clang/lib/Format/UnwrappedLineParser.cpp index f0531d2c8a9df..1b00e9015ea6e 100644 --- a/clang/lib/Format/UnwrappedLineParser.cpp +++ b/clang/lib/Format/UnwrappedLineParser.cpp @@ -815,7 +815,7 @@ FormatToken *UnwrappedLineParser::parseBlock( !parsingPPDirective() ? PreprocessorDirectives.size() : 0; addUnwrappedLine(); size_t OpeningLineIndex = - CurrentLines->empty() + CurrentLines->empty() || !CurrentExpandedLines.empty() ? (UnwrappedLine::kInvalidIndex) : (CurrentLines->size() - 1 - NbPreprocessorDirectives); @@ -876,6 +876,8 @@ FormatToken *UnwrappedLineParser::parseBlock( if (Previous->is(tok::r_brace) && !Previous->Optional) return false; } + if (OpeningLineIndex == UnwrappedLine::kInvalidIndex) + return false; assert(!CurrentLines->empty()); auto &LastLine = CurrentLines->back(); if (LastLine.Level == InitialLevel + 1 && !mightFitOnOneLine(LastLine)) @@ -933,7 +935,8 @@ FormatToken *UnwrappedLineParser::parseBlock( if (MunchSemi && FormatTok->is(tok::semi)) nextToken(); - if (PPStartHash == PPEndHash) { + if (PPStartHash == PPEndHash && CurrentExpandedLines.empty() && + !containsExpansion(*Line)) { Line->MatchingOpeningBlockLineIndex = OpeningLineIndex; if (OpeningLineIndex != UnwrappedLine::kInvalidIndex) { // Update the opening line to add the forward reference as well @@ -4071,9 +4074,10 @@ bool UnwrappedLineParser::parseEnum() { if (!ManageWhitesmithsBraces) ++Line->Level; } - const auto OpeningLineIndex = CurrentLines->empty() - ? UnwrappedLine::kInvalidIndex - : CurrentLines->size() - 1; + const auto OpeningLineIndex = + CurrentLines->empty() || !CurrentExpandedLines.empty() + ? UnwrappedLine::kInvalidIndex + : CurrentLines->size() - 1; bool HasError = !parseBracedList(/*IsAngleBracket=*/false, /*IsEnum=*/true); if (!Style.AllowShortEnumsOnASingleLine && !ManageWhitesmithsBraces) --Line->Level; @@ -4083,8 +4087,10 @@ bool UnwrappedLineParser::parseEnum() { addUnwrappedLine(); } setPreviousRBraceType(TT_EnumRBrace); - if (ManageWhitesmithsBraces) + if (ManageWhitesmithsBraces && CurrentExpandedLines.empty() && + !containsExpansion(*Line)) { Line->MatchingOpeningBlockLineIndex = OpeningLineIndex; + } return true; // There is no addUnwrappedLine() here so that we fall through to parsing a diff --git a/clang/unittests/Format/FormatTestMacroExpansion.cpp b/clang/unittests/Format/FormatTestMacroExpansion.cpp index 7bc9cc98f4e24..31fce2dbd8311 100644 --- a/clang/unittests/Format/FormatTestMacroExpansion.cpp +++ b/clang/unittests/Format/FormatTestMacroExpansion.cpp @@ -494,6 +494,119 @@ TEST_F(FormatTestMacroExpansion, TokensAfterMacroCallAreNotPartOfCall) { Style); } +TEST_F(FormatTestMacroExpansion, + SeparateDefinitionBlocksWithMacroExpandedStruct) { + // Reproduces https://llvm.org/PR78263 + FormatStyle Style = getLLVMStyle(); + Style.BreakBeforeBraces = FormatStyle::BS_Custom; + Style.BraceWrapping.AfterStruct = true; + Style.SeparateDefinitionBlocks = FormatStyle::SDS_Always; + Style.Macros.push_back("PACK(x)=x"); + + verifyFormat("static bool foo;\n" + "\n" + "//! \\brief Blah\n" + "PACK(\n" + " struct Foo\n" + " {\n" + " uint16_t a;\n" + " });\n" + "\n" + "PACK(\n" + " struct Bar\n" + " {\n" + " uint32_t a;\n" + " });\n" + "\n" + "PACK(\n" + " struct Baz\n" + " {\n" + " uint16_t a;\n" + " });", + "static bool foo;\n" + "\n" + "//! \\brief Blah\n" + "PACK(struct Foo\n" + "{\n" + " uint16_t a;\n" + "});\n" + "\n" + "PACK(struct Bar\n" + "{\n" + " uint32_t a;\n" + "});\n" + "\n" + "PACK(struct Baz\n" + "{\n" + " uint16_t a;\n" + "});", + Style); +} + +TEST_F(FormatTestMacroExpansion, MacroExpansionWithIfElseBranches) { + // Reproduces https://llvm.org/PR233300 + FormatStyle Style = getLLVMStyle(); + Style.Macros.push_back("M(x)=x"); + + verifyFormat("#if A\n" + "auto l = []\n" + "#else\n" + "if\n" + "#endif\n" + " M((int a) {});", + Style); + + Style.Macros = {"M(a,b,c)=a b(c)"}; + verifyNoCrash("({\n" + "#if A\n" + "{[]\n" + "#else\n" + "if\n" + "#endif\n" + "M(, (int a){int b, ))", + Style); +} + +TEST_F(FormatTestMacroExpansion, RawStringLiteralInMacroArg) { + FormatStyle Style = getGoogleStyle(FormatStyle::LK_Cpp); + Style.Macros.push_back("ASSIGN_OR_RETURN(a, b)=a = (b)"); + + StringRef Code1 = + "void f() {\n" + " ASSIGN_OR_RETURN(FlowConfig flow_config,\n" + " ParseTextProto<FlowConfig>(Substitute(\n" + " R\"pb(\n" + " key: value\n" + " )pb\",\n" + " arg1, arg2, arg3, arg4)));\n" + "}"; + verifyFormat(Code1, Code1, Style); + + StringRef Code2 = + "void f() {\n" + " ASSIGN_OR_RETURN(\n" + " FlowConfig flow_config,\n" + " " + "proto2::contrib::parse_proto::ParseTextProto<FlowConfig>(absl::" + "Substitute(\n" + " R\"pb(\n" + " key: value\n" + " )pb\",\n" + " kPreprocessingFlowConfig, kAudioConfig, kPostConfig)));\n" + "}"; + verifyFormat(Code2, Code2, Style); + + StringRef Code3 = + "void f() {\n" + " ASSIGN_OR_RETURN(\n" + " // comment\n" + " TestCase test_case, ParseTextProto<TestCase>(R\"pb(\n" + " name: \"foo\"\n" + " )pb\"));\n" + "}"; + verifyFormat(Code3, Code3, Style); +} + } // namespace } // namespace test } // namespace format _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
