search

MikePopoloski / slang

SystemVerilog compiler and language services
MIT License
592 stars 132 forks source link

Ensure EoL at the end of trivia to prevent inlining issues #916

Closed ihathbeer closed 6 months ago

ihathbeer commented 6 months ago

This is a fix for the issue described in #914.

Functional evidence:

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

BUS_WIDTH 2
SOMETHING 2
SV_COV_ASSERTION 20
SV_COV_CHECK 3
SV_COV_ERROR -1
SV_COV_FSM_STATE 21
SV_COV_HIER 11
SV_COV_MODULE 10
SV_COV_NOCOV 0
SV_COV_OK 1
SV_COV_OVERFLOW -2
SV_COV_PARTIAL 2
SV_COV_RESET 2
SV_COV_START 0
SV_COV_STATEMENT 22
SV_COV_STOP 1
SV_COV_TOGGLE 23
__slang__ 1
__slang_major__ 5
__slang_minor__ 0
(base) drw@pirate slang % git log -1
commit 594e3aaab7bd76458da9997c72749227c5ef2e80 (HEAD -> fix/ensure-eof-trivia-to-prevent-inlining-issues, origin/fix/ensure-eof-trivia-to-prevent-inlining-issues)
Author: drw <nedea.a@northeastern.edu>
Date:   Thu Mar 14 13:44:15 2024 -0700

    Ensure EoF at the end of trivia to prevent inlining issues

Before:

(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
SV_COV_CHECK 3
SV_COV_ERROR -1
SV_COV_FSM_STATE 21
SV_COV_HIER 11
SV_COV_MODULE 10
SV_COV_NOCOV 0
SV_COV_OK 1
SV_COV_OVERFLOW -2
SV_COV_PARTIAL 2
SV_COV_RESET 2
SV_COV_START 0
SV_COV_STATEMENT 22
SV_COV_STOP 1
SV_COV_TOGGLE 23
__slang__ 1
__slang_major__ 5
__slang_minor__ 0
(base) drw@pirate slang % git log -1
commit cad562f821a575728e65f5414375c7cb94d2d124 (HEAD -> master, origin/master, origin/HEAD)
Author: MikePopoloski <mike@popoloski.com>
Date:   Wed Mar 13 20:09:09 2024 -0400

    Add explicit test for function result chaining
codecov[bot] commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 93.84%. Comparing base (cad562f) to head (71e28fb). Report is 22 commits behind head on master.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/MikePopoloski/slang/pull/916/graphs/tree.svg?width=650&height=150&src=pr&token=sS5JjK9091&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Michael+Popoloski)](https://app.codecov.io/gh/MikePopoloski/slang/pull/916?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Michael+Popoloski) ```diff @@ Coverage Diff @@ ## master #916 +/- ## ========================================== + Coverage 93.75% 93.84% +0.08% ========================================== Files 190 190 Lines 47784 47925 +141 ========================================== + Hits 44799 44973 +174 + Misses 2985 2952 -33 ``` | [Files](https://app.codecov.io/gh/MikePopoloski/slang/pull/916?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Michael+Popoloski) | Coverage Δ | | |---|---|---| | [source/parsing/Preprocessor.cpp](https://app.codecov.io/gh/MikePopoloski/slang/pull/916?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Michael+Popoloski#diff-c291cmNlL3BhcnNpbmcvUHJlcHJvY2Vzc29yLmNwcA==) | `99.86% <100.00%> (+<0.01%)` | :arrow_up: | ... and [55 files with indirect coverage changes](https://app.codecov.io/gh/MikePopoloski/slang/pull/916/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Michael+Popoloski) ------ [Continue to review full report in Codecov by Sentry](https://app.codecov.io/gh/MikePopoloski/slang/pull/916?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Michael+Popoloski). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Michael+Popoloski) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://app.codecov.io/gh/MikePopoloski/slang/pull/916?dropdown=coverage&src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Michael+Popoloski). Last update [cad562f...71e28fb](https://app.codecov.io/gh/MikePopoloski/slang/pull/916?dropdown=coverage&src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Michael+Popoloski). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Michael+Popoloski).
ihathbeer commented 6 months ago

With patch commented out the unit test fails:

Test project /home/morty/slang/build
    Start 1: unittests
1/6 Test #1: unittests ........................***Failed    0.69 sec
data/file_uses_define_in_file_with_no_cr.sv:2:5: error: member not allowed at compilation unit scope
    initial begin
    ^~~~~~~~~~~~~
data/file_uses_define_in_file_with_no_cr.sv:3:32: error: expected ','
        $display("Something: %d", `SOMETHING);
                                  ^
data/file_with_no_cr.sv:1:23: note: expanded from macro 'SOMETHING'
`define SOMETHING 1337
                      ^
data/file_uses_define_in_file_with_no_cr.sv:3:44: error: expected ')'
        $display("Something: %d", `SOMETHING);
                                              ^
data/file_uses_define_in_file_with_no_cr.sv:3:14: note: to match this '('
        $display("Something: %d", `SOMETHING);
                ^
data/file_uses_define_in_file_with_no_cr.sv:5:1: error: expected member
endmodule
^~~~~~~~~
Randomness seeded to: 420428605

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
unittests is a Catch2 v3.5.3 host application.
Run with -? for options

-------------------------------------------------------------------------------
Driver single-unit parsing files with no EOL
-------------------------------------------------------------------------------
/home/morty/slang/tests/unittests/DriverTests.cpp:244
...............................................................................

/home/morty/slang/tests/unittests/DriverTests.cpp:253: FAILED:
  CHECK( driver.reportParseDiags() )
with expansion:
  false

===============================================================================
test cases:  1590 |  1589 passed | 1 failed
assertions: 18619 | 18618 passed | 1 failed