MikePopoloski / slang

SystemVerilog compiler and language services
MIT License
555 stars 120 forks source link

Inlining 1-line files via include directives fails when no CR/LF is present #914

Closed ihathbeer closed 2 months ago

ihathbeer commented 3 months ago

Describe the bug Inlining 1-line header files with no CR/LF (carriage return / line feed) into source files compiled as a single compilation unit can result in the macro contents being prepended with the `include directive, which later causes the macro definitions to not be found.

I have noticed some editors (VScode) do not add a carriage return or line feed, so you can end up with something like this:

(base) drw@pirate slang % ./build/bin/slang ../inlining-repro/foo.sv ../inlining-repro/main.sv -I ../inlining-repro --single-unit --macros-only

SOMETHING 2`include "defs.svh"
SV_COV_ASSERTION 20
...
(base) drw@pirate x % hexdump inlining-repro/foo_header.svh
0000000 6460 6665 6e69 2065 4f53 454d 4854 4e49
0000010 2047 0032
0000013
(base) drw@pirate x % hexdump inlining-repro/defs.svh
0000000 6460 6665 6e69 2065 5542 5f53 4957 5444
0000010 2048 0032
0000013
(base) drw@pirate x %

To Reproduce Use the command I've posted above on the files attached or run this command:

./build/bin/slang ../inlining-repro/foo.sv ../inlining-repro/main.sv -I ../inlining-repro --single-unit

You'll get the following:

Top level design units:
    main_module

../inlining-repro/main.sv:3:31: error: unknown macro or compiler directive '`BUS_WIDTH'
module main_module#(bus_width=`BUS_WIDTH)(input logic clk, input logic rst);
                              ^

Build failed: 1 error, 0 warnings

inlining-repro.zip

ihathbeer commented 3 months ago

A potential fix is to push a newline here if no LF/CR is present in the buffer about to be pushed to the stack: https://github.com/MikePopoloski/slang/blob/cad562f821a575728e65f5414375c7cb94d2d124/source/parsing/Preprocessor.cpp#L520-L521

Happy to put a PR up if you agree.

yanggeorge commented 3 months ago

Agree. Two lines of code are concatenated into one, as in "define SOMETHING 2include \"defs.svh\"", which results in an error.

MikePopoloski commented 3 months ago

I think the right place is actually here: https://github.com/MikePopoloski/slang/blob/cad562f821a575728e65f5414375c7cb94d2d124/source/parsing/Preprocessor.cpp#L439

If there's no EndOfLine at the end of the trivia you can add one. I'd be happy to take a PR.