Proposal: enhance handling of conditional preprocessor directives

[pmaupin]

Thanks for making this available! It looks immensely useful.

I am faced with a lot of verilog that verible cannot process due to syntax errors, similar to issue #228.

For example:

module two_modules_in_one(
    input a,
    output b
`ifdef EXTENDED_FUNCTIONALITY
    , output c
`endif
);

endmodule

I think you'll find a lot of this sort of thing in real-world codebases.

I haven't read through the codebase or anything (and my C++ is quite rusty anyway), but taking into account the description that the lexer and parser are decoupled, and after reading through the discussion on #228 and thinking about it a bit, I have a proposal that should be much easier to implement than a full preprocessor.

This would probably have to be implemented as a user-configurable option; for reasons that will become clear, badly-constructed modules could result in an exponential blow-up of processing time and the output.

Whenever an `ifxxx or `else is encountered in a context where the parser cannot process it, rewrite the token stream to push the preprocessor directive up and outside the context where it is invalid, add an `else if necessary, and duplicate the non-conditional tokens. For example, the above module could be represented by something like this:

Lexed and filtered tokens: 
(#"`ifdef" @53-59: "`ifdef")
(#PP_Identifier @60-82: "EXTENDED_FUNCTIONALITY")
(#"module" @0-6: "module")
(#SymbolIdentifier @7-25: "two_modules_in_one")
(#'(' @25-26: "(")
(#"input" @31-36: "input")
(#SymbolIdentifier @37-38: "a")
(#',' @38-39: ",")
(#"output" @44-50: "output")
(#SymbolIdentifier @51-52: "b")
(#',' @87-88: ",")
(#"output" @89-95: "output")
(#SymbolIdentifier @96-97: "c")
(#')' @105-106: ")")
(#';' @106-107: ";")
(#"endmodule" @109-118: "endmodule")
(#"`else" @????: "`else")
(#"module" @0-6: "module")
(#SymbolIdentifier @7-25: "two_modules_in_one")
(#'(' @25-26: "(")
(#"input" @31-36: "input")
(#SymbolIdentifier @37-38: "a")
(#',' @38-39: ",")
(#"output" @44-50: "output")
(#SymbolIdentifier @51-52: "b")
(#')' @105-106: ")")
(#';' @106-107: ";")
(#"endmodule" @109-118: "endmodule")
(#"`endif" @98-104: "`endif")
(#$end @119-119: "")

and, of course, the code in issue #128 would simply be represented as two always blocks on different branches of the `ifdef/`else/`endif

[pmaupin]

Actually, I realize now your parser is smart enough to handle conditionals in the header, just not right before a comma, so that's a separate issue. I think the proposal would still be useful, though, for cases like the always block.

Thanks,
Pat

[fangism]

Yes, I've considered the pseudo-preprocessing approaches you described of presenting various preprocessing filtered views, and forwarding those to subsequent parsing and processing. Some early thoughts here:
https://github.com/google/verible/blob/a9d1b6412820264dc3c279d4636e98c1a761545c/verilog/preprocessor/verilog_preprocess.h#L15

Possible strategies include:

• take first branch, take else branch
• assume encountered predicates are defined/undefined
• take multiple/all branches (basically strip preprocessing directives)

It's currently a low priority and a help-wanted area. But I agree it would multiply the potential outreach of the Verible toolset.,