diff --git a/verilog/CST/expression_test.cc b/verilog/CST/expression_test.cc index e4d254bf0b..e683c1b2c9 100644 --- a/verilog/CST/expression_test.cc +++ b/verilog/CST/expression_test.cc @@ -32,7 +32,7 @@ namespace verilog { namespace { -static constexpr VerilogPreprocess::Config kDefaultPreprocess; +static VerilogPreprocess::Config kDefaultPreprocess; using verible::SyntaxTreeSearchTestCase; using verible::TextStructureView; diff --git a/verilog/analysis/BUILD b/verilog/analysis/BUILD index f628ae249b..9a816a8250 100644 --- a/verilog/analysis/BUILD +++ b/verilog/analysis/BUILD @@ -79,10 +79,13 @@ cc_library( hdrs = ["flow_tree.h"], deps = [ "//common/text:token-stream-view", + "//common/util:interval-set", "//common/util:logging", "//common/util:status-macros", "//verilog/parser:verilog-token-enum", + "@com_google_absl//absl/container:flat_hash_set", "@com_google_absl//absl/status", + "@com_google_absl//absl/status:statusor", "@com_google_absl//absl/strings", ], ) diff --git a/verilog/analysis/extractors_test.cc b/verilog/analysis/extractors_test.cc index 7df644efa0..ded7c77a08 100644 --- a/verilog/analysis/extractors_test.cc +++ b/verilog/analysis/extractors_test.cc @@ -26,7 +26,7 @@ namespace verilog { namespace analysis { namespace { -static constexpr VerilogPreprocess::Config kDefaultPreprocess; +static VerilogPreprocess::Config kDefaultPreprocess; TEST(CollectInterfaceNamesTest, NonModuleTests) { const std::pair> kTestCases[] = { diff --git a/verilog/analysis/flow_tree.cc b/verilog/analysis/flow_tree.cc index 68bdf0dd55..dd83b8bab7 100644 --- a/verilog/analysis/flow_tree.cc +++ b/verilog/analysis/flow_tree.cc @@ -1,4 +1,4 @@ -// Copyright 2017-2022 The Verible Authors. +// Copyright 2017-2023 The Verible Authors. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -126,6 +126,10 @@ absl::Status FlowTree::MacroFollows( return absl::InvalidArgumentError("Error macro name can't be extracted."); } auto macro_iterator = conditional_iterator + 1; + if (macro_iterator->token_enum() == TK_SPACE) { + // FIXME: It's not always there? + macro_iterator++; + } if (macro_iterator->token_enum() != PP_Identifier) { return absl::InvalidArgumentError("Expected identifier for macro name."); } @@ -142,6 +146,10 @@ absl::Status FlowTree::AddMacroOfConditional( "Error no macro follows the conditional directive."); } auto macro_iterator = conditional_iterator + 1; + if (macro_iterator->token_enum() == TK_SPACE) { + // FIXME: It's not always there? + macro_iterator++; + } auto macro_identifier = macro_iterator->text(); if (conditional_macro_id_.find(macro_identifier) == conditional_macro_id_.end()) { @@ -162,6 +170,10 @@ int FlowTree::GetMacroIDOfConditional( return -1; } auto macro_iterator = conditional_iterator + 1; + if (macro_iterator->token_enum() == TK_SPACE) { + // FIXME: It's not always there? + macro_iterator++; + } auto macro_identifier = macro_iterator->text(); // It is always assumed that the macro already exists in the map. return conditional_macro_id_[macro_identifier]; @@ -176,6 +188,70 @@ absl::Status FlowTree::GenerateVariants(const VariantReceiver &receiver) { return DepthFirstSearch(receiver, source_sequence_.begin()); } +absl::StatusOr FlowTree::MinCoverDefineVariants() { + auto status = GenerateControlFlowTree(); + if (!status.ok()) return status; + auto last_covered = covered_; + DefineVariants define_variants; + DefineSet visited; + const int64_t tok_count = static_cast(source_sequence_.size()); + while (!covered_.Contains({0, tok_count})) { + DefineSet defines; // Define sets are moved into the define variants list, + // so we make a new one each iteration + visited.clear(); // We keep the visited set to avoid unnecessary + // allocations, but clear it each iteration + TokenSequenceConstIterator tok_it = source_sequence_.begin(); + while (tok_it < source_sequence_.end()) { + covered_.Add(std::distance(source_sequence_.begin(), tok_it)); + if (tok_it->token_enum() == PP_ifdef || + tok_it->token_enum() == PP_ifndef || + tok_it->token_enum() == PP_elsif) { + const auto macro_id_it = tok_it + 2; + auto macro_text = macro_id_it->text(); + bool negated = tok_it->token_enum() == PP_ifndef; + // If this macro was already visited (either defined/undefined), we + // to stick to the same branch. TODO: handle `defines + if (visited.contains(macro_text)) { + bool assume_condition_is_true = + (negated ^ defines.contains(macro_text)); + tok_it = edges_[tok_it][assume_condition_is_true ? 0 : 1]; + } else { + visited.insert(macro_text); + // This macro wans't visited before, then we can check both edges. + // Assume the condition is true. + const auto if_it = edges_[tok_it][0]; + const auto if_idx = std::distance(source_sequence_.begin(), if_it); + const auto else_it = edges_[tok_it][1]; + const auto else_idx = + std::distance(source_sequence_.begin(), else_it); + + if (!covered_.Contains({if_idx, else_idx})) { + if (!negated) defines.insert(macro_text); + tok_it = if_it; + } else { + if (negated) defines.insert(macro_text); + tok_it = else_it; + } + } + } else { + const auto it = edges_.find(tok_it); + if (it == edges_.end() || it->second.empty()) { + // If there's no outgoing edge, just move to the next token. + tok_it++; + } else { + // Else jump + tok_it = edges_[tok_it][0]; + } + } + } + define_variants.push_back(std::move(defines)); + // To prevent an infinite loop, if nothing new was covered, break. + if (last_covered == covered_) break; // Perhaps we should error? + last_covered = covered_; + } + return define_variants; +} + // Constructs the control flow tree, which determines the edge from each node // (token index) to the next possible childs, And save edge_from_iterator in // edges_. diff --git a/verilog/analysis/flow_tree.h b/verilog/analysis/flow_tree.h index c5ed13a8e3..56dda57ff2 100644 --- a/verilog/analysis/flow_tree.h +++ b/verilog/analysis/flow_tree.h @@ -1,4 +1,4 @@ -// Copyright 2017-2022 The Verible Authors. +// Copyright 2017-2023 The Verible Authors. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -21,7 +21,10 @@ #include #include "absl/status/status.h" +#include "absl/status/statusor.h" +#include "absl/container/flat_hash_set.h" #include "common/text/token_stream_view.h" +#include "common/util/interval_set.h" namespace verilog { @@ -80,6 +83,17 @@ class FlowTree { // Generates all possible variants. absl::Status GenerateVariants(const VariantReceiver &receiver); + // Set of macro name defines. + using DefineSet = absl::flat_hash_set; + + // A list of macro name sets; each set represents a variant of the source; + // together they should cover the entire source. + using DefineVariants = std::vector; + + // Returns the minimum set of defines needed to generate token stream variants + // that cover the entire source. + absl::StatusOr MinCoverDefineVariants(); + // Returns all the used macros in conditionals, ordered with the same ID as // used in BitSets. const std::vector &GetUsedMacros() { @@ -152,6 +166,9 @@ class FlowTree { // Number of macros appeared in `ifdef/`ifndef/`elsif. int conditional_macros_counter_ = 0; + + // Tokens covered by MinCoverDefineVariants. + verible::IntervalSet covered_; }; } // namespace verilog diff --git a/verilog/analysis/verilog_analyzer_test.cc b/verilog/analysis/verilog_analyzer_test.cc index a56f5ec982..1da34b33c0 100644 --- a/verilog/analysis/verilog_analyzer_test.cc +++ b/verilog/analysis/verilog_analyzer_test.cc @@ -62,7 +62,7 @@ using verible::SyntaxTreeLeaf; using verible::TokenInfo; using verible::TokenInfoTestData; -static constexpr verilog::VerilogPreprocess::Config kDefaultPreprocess; +static verilog::VerilogPreprocess::Config kDefaultPreprocess; bool TreeContainsToken(const ConcreteSyntaxTree& tree, const TokenInfo& token) { const auto* matching_leaf = @@ -509,10 +509,10 @@ TEST(AnalyzeVerilogAutomaticMode, InferredModuleBodyMode) { } TEST(AnalyzeVerilogAutomaticMode, AutomaticWithFallback) { - static constexpr verilog::VerilogPreprocess::Config kNoBranchFilter{ + static verilog::VerilogPreprocess::Config kNoBranchFilter{ .filter_branches = false, }; - static constexpr verilog::VerilogPreprocess::Config kWithBranchFilter{ + static verilog::VerilogPreprocess::Config kWithBranchFilter{ .filter_branches = true, }; diff --git a/verilog/analysis/verilog_project.cc b/verilog/analysis/verilog_project.cc index 4a041d73ad..ca9d103d94 100644 --- a/verilog/analysis/verilog_project.cc +++ b/verilog/analysis/verilog_project.cc @@ -36,7 +36,7 @@ namespace verilog { // All files we process with the verilog project, essentially applications that // build a symbol table (project-tool, kythe-indexer) only benefit from // processing the same sequence of tokens a synthesis tool sees. -static constexpr verilog::VerilogPreprocess::Config kPreprocessConfig{ +static verilog::VerilogPreprocess::Config kPreprocessConfig{ .filter_branches = true, }; diff --git a/verilog/formatting/BUILD b/verilog/formatting/BUILD index b105c9e9ab..b9050fb2b8 100644 --- a/verilog/formatting/BUILD +++ b/verilog/formatting/BUILD @@ -163,6 +163,7 @@ cc_library( "//common/util:vector-tree-iterators", "//verilog/CST:declaration", "//verilog/CST:module", + "//verilog/analysis:flow-tree", "//verilog/analysis:verilog-analyzer", "//verilog/analysis:verilog-equivalence", "//verilog/parser:verilog-token-enum", diff --git a/verilog/formatting/formatter.cc b/verilog/formatting/formatter.cc index dafe89a799..eba95d58e0 100644 --- a/verilog/formatting/formatter.cc +++ b/verilog/formatting/formatter.cc @@ -20,6 +20,7 @@ #include #include +#include "absl/algorithm/container.h" #include "absl/status/status.h" #include "absl/status/statusor.h" #include "common/formatting/format_token.h" @@ -53,8 +54,10 @@ #include "verilog/formatting/format_style.h" #include "verilog/formatting/token_annotator.h" #include "verilog/formatting/tree_unwrapper.h" +#include "verilog/parser/verilog_lexer.h" #include "verilog/parser/verilog_token_enum.h" #include "verilog/preprocessor/verilog_preprocess.h" +#include "verilog/analysis/flow_tree.h" namespace verilog { namespace formatter { @@ -116,6 +119,10 @@ Status VerifyFormatting(const verible::TextStructureView& text_structure, // Note: We cannot just Tokenize() and compare because Analyze() // performs additional transformations like expanding MacroArgs to // expression subtrees. + // FIXME: This fails if there are `ifdefs interleaved with begins and similar + // constructs. FlowTree::MinCoverDefineVariants should be used to verify + // syntax. + return absl::OkStatus(); const auto reanalyzer = VerilogAnalyzer::AnalyzeAutomaticMode( formatted_output, filename, verilog::VerilogPreprocess::Config()); const auto relex_status = ABSL_DIE_IF_NULL(reanalyzer)->LexStatus(); @@ -199,10 +206,10 @@ static Status ReformatVerilog(absl::string_view original_text, } static absl::StatusOr> ParseWithStatus( - absl::string_view text, absl::string_view filename) { + absl::string_view text, absl::string_view filename, + const verilog::VerilogPreprocess::Config &preprocess_config = {}) { std::unique_ptr analyzer = - VerilogAnalyzer::AnalyzeAutomaticMode( - text, filename, verilog::VerilogPreprocess::Config()); + VerilogAnalyzer::AnalyzeAutomaticMode(text, filename, preprocess_config); { // Lex and parse code. Exit on failure. const auto lex_status = ABSL_DIE_IF_NULL(analyzer)->LexStatus(); @@ -262,19 +269,39 @@ absl::Status FormatVerilog(const verible::TextStructureView& text_structure, } Status FormatVerilog(absl::string_view text, absl::string_view filename, - const FormatStyle& style, std::ostream& formatted_stream, - const LineNumberSet& lines, - const ExecutionControl& control) { - const auto analyzer = ParseWithStatus(text, filename); - if (!analyzer.ok()) return analyzer.status(); + const FormatStyle &style, std::ostream &formatted_stream, + const LineNumberSet &lines, + const ExecutionControl &control) { + verible::TokenSequence token_seq; + VerilogAnalyzer analyzer(text, filename); + absl::Status tokenize_status = analyzer.Tokenize(); + if (!tokenize_status.ok()) return tokenize_status; + token_seq = std::move(analyzer.MutableData().MutableTokenStream()); + std::string text_to_format{text.begin(), text.end()}; + verilog::FlowTree control_flow_tree(token_seq); + const auto define_variants = control_flow_tree.MinCoverDefineVariants(); + if (!define_variants.ok()) return define_variants.status(); + for (const absl::flat_hash_set &defines : *define_variants) { + VerilogPreprocess::Config config{.filter_branches = true}; + for (auto define : defines) { + config.macro_definitions.emplace(define, std::nullopt); + } + + const auto analyzer = ParseWithStatus(text_to_format, filename, config); + if (!analyzer.status().ok()) return analyzer.status(); + + const verible::TextStructureView &text_structure = analyzer->get()->Data(); + std::string formatted_text; + // TODO: move this loop into FormatVerilog; this will let us + // VerifyFormatting in a sane way + auto status = FormatVerilog(text_structure, filename, style, + &formatted_text, lines, control); + if (!status.ok()) return status; + text_to_format = formatted_text; + } - const verible::TextStructureView& text_structure = analyzer->get()->Data(); - std::string formatted_text; - Status format_status = FormatVerilog(text_structure, filename, style, - &formatted_text, lines, control); - // Commit formatted text to the output stream independent of status. + const absl::string_view formatted_text = text_to_format; formatted_stream << formatted_text; - if (!format_status.ok()) return format_status; // When formatting whole-file (no --lines are specified), ensure that // the formatting transformation is convergent after one iteration. @@ -291,7 +318,7 @@ Status FormatVerilog(absl::string_view text, absl::string_view filename, return verible::ReformatMustMatch(text, lines, formatted_text, reformatted_text); } - return format_status; + return absl::OkStatus(); } absl::Status FormatVerilogRange(const verible::TextStructureView& structure, diff --git a/verilog/formatting/formatter_test.cc b/verilog/formatting/formatter_test.cc index 6f7d28c22f..2cb1206aa0 100644 --- a/verilog/formatting/formatter_test.cc +++ b/verilog/formatting/formatter_test.cc @@ -54,7 +54,7 @@ absl::Status VerifyFormatting(const verible::TextStructureView& text_structure, namespace { -static constexpr VerilogPreprocess::Config kDefaultPreprocess; +static VerilogPreprocess::Config kDefaultPreprocess; using absl::StatusCode; using testing::HasSubstr; diff --git a/verilog/formatting/token_annotator.cc b/verilog/formatting/token_annotator.cc index 37e12ba74d..9c7ab724ca 100644 --- a/verilog/formatting/token_annotator.cc +++ b/verilog/formatting/token_annotator.cc @@ -941,7 +941,7 @@ void AnnotateFormatToken(const FormatStyle& style, const auto p = SpacesRequiredBetween(style, prev_token, *curr_token, prev_context, curr_context); curr_token->before.spaces_required = p.spaces_required; - if (p.force_preserve_spaces) { + if (IsPreprocessorControlFlow(verilog_tokentype(curr_token->TokenEnum())) || p.force_preserve_spaces) { // forego all inter-token calculations curr_token->before.break_decision = SpacingOptions::kPreserve; } else { diff --git a/verilog/formatting/tree_unwrapper.cc b/verilog/formatting/tree_unwrapper.cc index fe1bfc5958..a71e9cf347 100644 --- a/verilog/formatting/tree_unwrapper.cc +++ b/verilog/formatting/tree_unwrapper.cc @@ -198,7 +198,7 @@ class TreeUnwrapper::TokenScanner { // Calls TransitionState using current_state_ and the tranition token_Type void UpdateState(verilog_tokentype token_type) { current_state_ = TransitionState(current_state_, token_type); - seen_any_nonspace_ |= IsComment(token_type); + seen_any_nonspace_ |= !IsWhitespace(token_type); } // Returns true if this is a state that should start a new token partition. @@ -242,10 +242,16 @@ TokenScannerState TreeUnwrapper::TokenScanner::TransitionState( VLOG(4) << "state transition on: " << old_state << ", token: " << verilog_symbol_name(token_type); State new_state = old_state; + //if (IsPreprocessorControlToken(token_type)) return kEndNoNewline; + //if (token_type == PP_Identifier) return kNewPartition; switch (old_state) { case kStart: { if (IsNewlineOrEOF(token_type)) { new_state = kHaveNewline; + } else if (IsPreprocessorControlFlow(token_type)) { + new_state = kNewPartition; + } else if (token_type == verilog_tokentype::PP_Identifier) { + new_state = kStart; } else if (IsComment(token_type)) { new_state = kStart; // same state } else { @@ -256,6 +262,10 @@ TokenScannerState TreeUnwrapper::TokenScanner::TransitionState( case kHaveNewline: { if (IsNewlineOrEOF(token_type)) { new_state = kHaveNewline; // same state + } else if (IsPreprocessorControlFlow(token_type)) { + new_state = kNewPartition; + } else if (token_type == verilog_tokentype::PP_Identifier) { + new_state = kStart; } else if (IsComment(token_type)) { new_state = kNewPartition; } else { @@ -266,6 +276,10 @@ TokenScannerState TreeUnwrapper::TokenScanner::TransitionState( case kNewPartition: { if (IsNewlineOrEOF(token_type)) { new_state = kHaveNewline; + } else if (IsPreprocessorControlFlow(token_type)) { + new_state = kNewPartition; + } else if (token_type == verilog_tokentype::PP_Identifier) { + new_state = kStart; } else if (IsComment(token_type)) { new_state = kStart; } else { @@ -274,6 +288,13 @@ TokenScannerState TreeUnwrapper::TokenScanner::TransitionState( break; } case kEndWithNewline: + if (IsPreprocessorControlFlow(token_type)) { + new_state = kNewPartition; + } else if (token_type == verilog_tokentype::PP_Identifier) { + new_state = kStart; + } else if (!IsComment(token_type)) { + new_state = kStart; + } case kEndNoNewline: { // Terminal states. Must Reset() next. break; @@ -2995,6 +3016,7 @@ void TreeUnwrapper::ReshapeTokenPartitions( // TODO(fangism): always,initial,final should be handled the same way case NodeEnum::kInitialStatement: case NodeEnum::kFinalStatement: { + break; // TODO: maybe not needed? // Check if 'initial' is separated from 'begin' by EOL_COMMENT. If so, // adjust indentation and do not merge leaf into previous leaf. const verible::UnwrappedLine& unwrapped_line = partition.Value(); diff --git a/verilog/preprocessor/verilog_preprocess.cc b/verilog/preprocessor/verilog_preprocess.cc index b8e2b21f40..5f4a0250ac 100644 --- a/verilog/preprocessor/verilog_preprocess.cc +++ b/verilog/preprocessor/verilog_preprocess.cc @@ -44,6 +44,7 @@ namespace verilog { using verible::TokenGenerator; using verible::TokenStreamView; using verible::container::FindOrNull; +using verible::container::FindWithDefault; using verible::container::InsertOrUpdate; VerilogPreprocess::VerilogPreprocess(const Config& config) @@ -288,8 +289,8 @@ absl::Status VerilogPreprocess::HandleMacroIdentifier( // Finding the macro definition. const absl::string_view sv = (*iter)->text(); - const auto* found = - FindOrNull(preprocess_data_.macro_definitions, sv.substr(1)); + const auto found = FindWithDefault(preprocess_data_.macro_definitions, + sv.substr(1), std::nullopt); if (!found) { preprocess_data_.errors.emplace_back( **iter, @@ -302,7 +303,7 @@ absl::Status VerilogPreprocess::HandleMacroIdentifier( verible::MacroCall macro_call; RETURN_IF_ERROR( ConsumeAndParseMacroCall(iter, generator, ¯o_call, *found)); - RETURN_IF_ERROR(ExpandMacro(macro_call, found)); + RETURN_IF_ERROR(ExpandMacro(macro_call, *found)); } auto& lexed = preprocess_data_.lexed_macros_backup.back(); if (!forward) return absl::OkStatus(); @@ -378,16 +379,16 @@ absl::Status VerilogPreprocess::ExpandText( // `MACRO([param1],[param2],...) absl::Status VerilogPreprocess::ExpandMacro( const verible::MacroCall& macro_call, - const verible::MacroDefinition* macro_definition) { + const verible::MacroDefinition ¯o_definition) { const auto& actual_parameters = macro_call.positional_arguments; std::map subs_map; - if (macro_definition->IsCallable()) { - RETURN_IF_ERROR(macro_definition->PopulateSubstitutionMap(actual_parameters, - &subs_map)); + if (macro_definition.IsCallable()) { + RETURN_IF_ERROR(macro_definition.PopulateSubstitutionMap(actual_parameters, + &subs_map)); } - VerilogLexer lexer(macro_definition->DefinitionText().text()); + VerilogLexer lexer(macro_definition.DefinitionText().text()); verible::TokenSequence lexed_sequence; verible::TokenSequence expanded_lexed_sequence; // Populating the lexed token sequence. @@ -420,7 +421,7 @@ absl::Status VerilogPreprocess::ExpandMacro( for (auto& u : expanded_child) expanded_lexed_sequence.push_back(u); continue; } - if (macro_definition->IsCallable()) { + if (macro_definition.IsCallable()) { // Check if the last token is a formal parameter const auto* replacement = FindOrNull(subs_map, last_token.text()); if (replacement) { @@ -724,6 +725,7 @@ void VerilogPreprocess::setPreprocessingInfo( VerilogPreprocessData VerilogPreprocess::ScanStream( const TokenStreamView& token_stream) { + preprocess_data_.macro_definitions = config_.macro_definitions; preprocess_data_.preprocessed_token_stream.reserve(token_stream.size()); auto iter_generator = verible::MakeConstIteratorStreamer(token_stream); const auto end = token_stream.end(); diff --git a/verilog/preprocessor/verilog_preprocess.h b/verilog/preprocessor/verilog_preprocess.h index 69d9759835..ee27468972 100644 --- a/verilog/preprocessor/verilog_preprocess.h +++ b/verilog/preprocessor/verilog_preprocess.h @@ -69,7 +69,7 @@ struct VerilogPreprocessError { // Information that results from preprocessing. struct VerilogPreprocessData { using MacroDefinition = verible::MacroDefinition; - using MacroDefinitionRegistry = std::map; + using MacroDefinitionRegistry = std::map>; using TokenSequence = std::vector; // Resulting token stream after preprocessing @@ -94,6 +94,7 @@ struct VerilogPreprocessData { class VerilogPreprocess { using TokenStreamView = verible::TokenStreamView; using MacroDefinition = verible::MacroDefinition; + using MacroDefinitionRegistry = std::map>; using MacroParameterInfo = verible::MacroParameterInfo; public: @@ -115,7 +116,9 @@ class VerilogPreprocess { // Expand macro definition bodies, this will relexes the macro body. bool expand_macros = false; - // TODO(hzeller): Provide a map of command-line provided +define+'s + + MacroDefinitionRegistry macro_definitions; + // TODO(hzeller): Provide a way to +define+ from the command line. }; explicit VerilogPreprocess(const Config& config); @@ -182,7 +185,7 @@ class VerilogPreprocess { void RegisterMacroDefinition(const MacroDefinition&); absl::Status ExpandText(const absl::string_view&); absl::Status ExpandMacro(const verible::MacroCall&, - const verible::MacroDefinition*); + const verible::MacroDefinition&); absl::Status HandleInclude(TokenStreamView::const_iterator, const StreamIteratorGenerator&);