Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

coverage support #65

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

coverage support #65

wants to merge 5 commits into from

Conversation

LeFl0w
Copy link

@LeFl0w LeFl0w commented Feb 13, 2023

hi,
it is more like an information for you than a real pull request.

Thanks to GHDL with GCC backend, we can compute coverage.
You can see in the diff that there is not many changes , the only 'difficulty' is to recompile GHDL with gcc backend.
Feel free to merge it (be careful as if your ghdl has not got covergage builtin, the simulation won't work) or to refuse it and get inspired to include coverage in your repository later on if wanted.

You can have a look at the lcov coverage output:
html.zip

cheers.

@stnolting
Copy link
Owner

This is really cool! 👍

I've never (really) worked with HDL code coverage analysis so far, but I think this might help a lot to improve code quality. It would be nice to have this code-coverage-enabled version of GHDL as default simulator version (and to deploy the results to GitHub pages automatically).

Are there any prebuild (and up-to-date) packages of GHDL with GCC as backend available?

@LeFl0w
Copy link
Author

LeFl0w commented Feb 14, 2023

I ran a little test and search. It seems that any gcc support version pack the coverage support .

I tried docker version fedora36-gcc-11.3.0 from GHDL docker hub and the simple adder coverage example worked.

I tried binaires ghdl-gha-ubuntu-22.04-gcc.tgz from ghdl but even of ubuntu 22.04 I got a library error libgcov profiling error:./ghdl-coverage-master/projects/adder/adder.gcda:Version mismatch - expected 11.3 (release) (B13*) got 11.2 (release) (B12*) .

so the best way to go would be the docker image or maybe the binary if you achieve the same gcc version

@LeFl0w
Copy link
Author

LeFl0w commented Feb 22, 2023

if you want to give a try, here is an extract of the github action we are using to apply the coverage on the vunit part of the neorv32 project (neorv32 was included as a submodule). It is not directly related to the riscof project but very similar anyway.

name: Full CI/CD
on:
  push:
    branches:
      - main
      - develop

jobs:
  coverage:
    name: Neorv32 coverage
    timeout-minutes: 60
    runs-on:  ubuntu-latest
    container: 
      image: ghdl/ghdl:fedora36-gcc-11.3.0
      volumes:
        - /$(pwd):/src
    steps:
      - name: Install Prerequisite
        run: |
          yum install -y python3-pip
          yum  -y install git
          yum install -y patch
          pip install gcovr 
          pip install vunit_hdl
          pip install beautifulsoup4
      - name: Git Checkout test Neorv32
        uses: actions/checkout@v3
        with:
          fetch-depth: 0
          submodules: 'true'

      - name: Create coverage report
        continue-on-error: true
        run: |
          patch -u ./neorv32/sim/run.py -i run.py.patch
          patch -u ./neorv32/rtl/core/neorv32_cpu.vhd -i neorv32_cpu.vhd.patch
          cd neorv32/sim
          VUNIT_SIMULATOR=ghdl ./run.py
          cd ../../
      
      - name: Archive code coverage results
        uses: actions/upload-artifact@v3
        with:
          name: neorv32-code-coverage-report
          path: ./coverage.xml

neorv32_cpu.vhd.patch
run.py.patch

you can notice we are using two patches :

  • one to enable coverage in th vunit part
  • the second to comment two assertion in the neorv32/rtl/core/neorv32_cpu.vhd file which trigger an error when the associated generic is 0 (which is posisble in the VHDL but not allowed in the assertion)

be careful as the pipeline could take 40 min to 1h to perform the coverage on the Vunit part of neorv32

@stnolting
Copy link
Owner

Thanks for the code (and sorry for the late reply). I will do some tests here.
I am still hoping to include this in the default CI configuration of this repository.

the second to comment two assertion in the neorv32/rtl/core/neorv32_cpu.vhd file which trigger an error when the associated generic is 0 (which is posisble in the VHDL but not allowed in the assertion)

Could you explain this a little bit more in detail? 🤔

@LeFl0w
Copy link
Author

LeFl0w commented Mar 1, 2023

No worries. We integrated that already in a custom ci-cd with sonarqube .We got a graphical feedback about the missing coverage related to the code produced within a new version( which is a more interesting way of dealing with coverage that a whole absolute number).
When everything is set up, we will release the link to the web instance.

For the second element , it is linked with
The assertions https://github.com/stnolting/neorv32/blob/49236537a223708b7366c55f40140f0d0b1e7817/rtl/core/neorv32_cpu.vhd#L214 and
https://github.com/stnolting/neorv32/blob/49236537a223708b7366c55f40140f0d0b1e7817/rtl/core/neorv32_cpu.vhd#L226

They trigger an assertion failure with example https://github.com/stnolting/neorv32/blob/main/rtl/processor_templates/neorv32_ProcessorTop_UP5KDemo.vhd in which PMP_NUM_REGIONS and HPM_NUM_CNTS are set to 0.
A failure is triggered with any Verific based VHDL frontend.
I did not investigate to see if the was legit , we simply commented them out , as it is only assertions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants