[WIP] Verible standalone preprocessor

[karimtera]

Description:
This PR introduces takes another step to enhance the standalone preprocessor tool by implementing multiple-cu mode.
Please refer to issue #1358 for more information about the proposed design.

Goal:

• In case multiple files are passed to the tool, each one of them will be compiled in a separate compilation unit, thus declarations scopes will end by the end of each file (this is the default behavior).
• Compiler directives should be correctly interpreted (files inclusion, macros expansion and conditionals are a good start).

Flags description

Using the Abseil flags library allows an easy access to flags and their arguments.

Flags	Type	Default	Description	State
--strip_comments	bool	false	This replaces comments with equal number of whitespaces, to preserve the same byte offsets.	DONE
--generate_variants	bool	false	This generates all the possible variants for conditionals.	DONE
--multiple_cu	bool	true	In case of true multiple files are passed to the tool, each one of them will be compiled in a separate compilation unit, thus declarations scopes will end by the end of each file (this is the default behavior), otherwise all files will be compiled in the same compilation unit.	DONE

Positional arguments

Those mainly are the SV files to preprocess, defines and search-directory paths for includes.
Any of the following positional argument can be passed one or more times.

Argument	Description	State
<file.sv>	Specifies a SV file to be preprocessed.	DONE
+define+<foo>[=<text>]	Defines one or more macros same as `define foo text	DONE
+incdir+<dir>	Specifies the directory to search for files included with `include "file_name"	DONE

TODO:

• Fixing testcases reported in mglb's review.
• Better comment placing.
• const qualifier in cmd_positional_arguments.h.
• Fixing +define+macro+="a=b" issue.
• Removing the full usage message, relying on --helpfull from absl/flags.
• Better error messages.

[codecov-commenter]

Codecov Report

Merging #1360 (18e990c) into master (80b07f9) will decrease coverage by 0.99%.
The diff coverage is 19.06%.

@@            Coverage Diff             @@
##           master    #1360      +/-   ##
==========================================
- Coverage   92.90%   91.91%   -1.00%     
==========================================
  Files         340      344       +4     
  Lines       23832    24125     +293     
==========================================
+ Hits        22141    22174      +33     
- Misses       1691     1951     +260     

Impacted Files	Coverage Δ
verilog/analysis/flow_tree.cc	0.00% <0.00%> (ø)
verilog/analysis/flow_tree.h	0.00% <0.00%> (ø)
verilog/preprocessor/verilog_preprocess.h	100.00% <ø> (ø)
verilog/preprocessor/verilog_preprocess.cc	74.68% <17.58%> (-16.90%)	⬇️
verilog/tools/preprocessor/verilog_preprocessor.cc	28.07% <23.30%> (-65.04%)	⬇️
common/util/cmd_positional_arguments.cc	35.18% <35.18%> (ø)
common/util/cmd_positional_arguments.h	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[karimtera]

Hello @hzeller ,

Wish you had a good vacation!

Here is a quick overview about what's going on here:

1. The preprocessor tool was getting arguments through a class defined in common/util/subcommand.h, It defines a subcommands such that each one has its own flags.
   I changed that to the normal abseil flags to be similar to other Verible tools.
   As @tgorochowik's request to standardize the way of passing arguments, What do you think about my approach?
2. About the generate_variants feature, I added comments so the code is understandable, but let me give you a quick overview:
   Now, this feature doesn't know if a macro is defined or not, It just assumes all the possibilities and outputs the SV code for each one by constructing a control flow tree, which defines the edges between each token and the possible next tokens, then traveses with a depth first search to generate all possible vairants.

[karimtera]

verible::CmdPositionalArguments aims to parse the preprocessor tool's positional arguments which includes:

1. <file.sv>
2. +define+<name>[=value]
3. +incdir+<dir>

Note:

1. For each type multiple arguments are supported in a single tool run.
2. For +define: multiple macros are supported in the same argument.
3. For +incdir: multiple directories are supported in the same argument.

TODO:

• The arguments are accessible now from the tool, but only SV files are used, need to use the rest.
• Need to implement a HandleIncludes function in verilog/analysis/preprocessor/.
• Need to implement a AddDefineFromCmdLine interface function in 'verilog/analysis/preprocessor`.

[karimtera]

Include directives supported usage:

• "file_path" can be an absolute path, or a relative one.
• In case of relative one, search_paths_ of verilog::VerilogPreprocess are searched.
• search_paths_ are passed to the tool through +incdir+<dir>[+<dir>].

Limitations:

• Number of nested includes should be limited (min is 15 as per LRM).
• Come up with a more efficient way to extend the included file TokenInfos life (more info is documented verilog/preprocess/verilog_preprocess.cc).
• Share defined macros with the included files.
• Minimal testing was done, need to add more tests.

[karimtera]

To solve the white-spaces problem I have added VerilogPreprocess::GenerateBypassWhiteSpaces function which skips white-spaces tokens, and returns an iterator to a non-whitespace token.

TokenStreamView::const_iterator VerilogPreprocess::GenerateBypassWhiteSpaces(
    const StreamIteratorGenerator& generator) {
  auto iterator =
      generator();  // iterator should be pointing to a non-whitespace token;
  while (verilog::VerilogLexer::KeepSyntaxTreeTokens(**iterator) == 0) {
    iterator = generator();
  }
  return iterator;
}

Then replaced each generator() into GenerateBypassWhiteSpaces(generator)) in VerilogPreprocess methods.

Example:
verible-verilog-preprocessor m5.sv

Example input file:

`include "file.sv"
module m();
  wire x = 1;
`ifdef KARIM
  real is_defined = 1;
`else
  real is_defined = 0;
`endif
// pre-blank line

// post-blank line
endmodule

Example output:

module included_module();
  wire x=1;
endmodule

module m();
  wire x = 1;

  real is_defined = 0;

// pre-blank line

// post-blank line
endmodule

Note: if added +define+KARIM then the other branch will be selected.

TODO:

• Add tests for all the implemented features, didn't want to do it earlier as I will have to revisit them once I fix the white-spaces problem.
  • File inclusion.
  • Multiple compilation unit mode.
  • Generate variants mode.
• Open issues on the known limitations.
• Maybe add a flag to disable blank lines.

[mglb]

Hi! I've played a bit with the tool and checked part of the code - more comments to come.
So far:

Maybe add a flag to disable blank lines.

Suggestion: this could include a comment following `endif/`else in the same line.

Question mostly out of curiosity: do SystemVerilog allows any tokens other than comments in the same line as `else and `endif? Because this works:

module m();
  wire x = `ifdef KARIM
      1`else/* no KARIM */0`endif; // endif KARIM
endmodule

Failing use cases:

input:

`ifndef KARIM
    // not defined: define
    `define KARIM
`endif

module m();
`ifdef KARIM
    // defined
`else
    // NOT defined
`endif
endmodule

output:

    // not defined: define



module m();

    // NOT defined

endmodule

Expected // defined.

---

file: file.sv:

module included_module();
  wire x=1;
endmodule

file: test.m01.inc.sv:

// begin: test.m01.inc.sv
`include "file.sv"
// end: test.m01.inc.sv

file: test.m01.sv (input):

// begin: test.m01.sv
`include "test.m01.inc.sv"
module m();
  wire x = 1;
endmodule
// end: test.m01.sv

output:

// begin: test.m01.sv
// begin: test.m01.inc.sv

module m();
  wire x = 1;
endmodule
// end: test.m01.sv

Expected inclusion of nested `include or error.

Cosmetic "issues"

More informative error reporting would be nice. At this moment it looks like:

$ ./bazel-bin/verilog/tools/preprocessor/verible-verilog-preprocessor - <<< $'// before\n`include "no-such-file.sv"\n// after\n'

// before
no such file found.

It is even more confusing when there's no comment before the include.

Example error from verible-verilog-syntax:

$ ./bazel-bin/verilog/tools/syntax/verible-verilog-syntax --printtokens - <<< "module 4"

-:1:8: syntax error at token "4"

---

I would rename --strip_comments to --erase_comments or something. I think "strip" suggests that they will be removed (replaced with empty strings).

---

Instead of Variant number 1: it would be nice to print something like //////// variant: 1 //////// for easier parsing (also for human eyes).

[mglb]

To solve the white-spaces problem I have added VerilogPreprocess::GenerateBypassWhiteSpaces function which skips white-spaces tokens, and returns an iterator to a non-whitespace token.

It looks good generally, and seems to work here.

[mglb]

const std::vector<...>& GetX() const; and auto& in assignments in verilog_preprocessor.cc

[karimtera]

The first const is giving me a warning with clang-tidy:
Clang-Tidy: Return type 'const TYPE' (aka 'const TYPE') is 'const'-qualified at the top level, which may reduce code readability without improving const correctness

I think it means that it leads the reader to think the caller can't change the returned value, which is not true in general.

[mglb]

I guess you forgot & after const std::vector<...>. This warnings appears only when returning const values/objects (which makes sense, because such const does nothing), not references to const value (which is used a lot to avoid forced copy).

[mglb]

Put comments above code to avoid excessive wrapping. Also in other places.

[mglb]

AFAIK `include <file> is not yet supported in Verible at all.

[karimtera]

That should be enabled from the lexer's side to begin with, right?

[mglb]

Yes, that's right.

[hzeller]

@kbieganski : here are some bits and pieces for the preprocessor, that have been addressed in various sub-pull request.

Can you have a look through these to make sure everything actually made it to the final preprocessor or if there are pieces missing that we should reformulate in a new PR ?