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

Moving set & getCSR from bThread header to implementation to prevent compiler errors #114

Open
wants to merge 1 commit into
base: software-cleanup
Choose a base branch
from

Conversation

sven-weber
Copy link

Description

For my master thesis, I integrated the register-based configuration into my Coyote FPGA design. I then proceeded to try to write/read register values using the getCSR and setCSR methods offered by bThread. However, doing so lead to a segmentation fault.

After some hours of debugging, I found the following problem:

The getCSR and setCSR methods rely on a memory-mapped region. This mapping is performed inside the bThread::mmapFpga() function where the ctrl_reg variable is set to point to the beginning of this memory region. Writing/Reading registers is then executed by reading from/writing to the address given by ctrl_reg. This writing/reading is implement in the header file of the bThread class. However, with certain compiler settings/versions/kinds this can lead to problems. I compiled Coyote with gcc version 9.4.0 and clang version 10.0.0 that are available in the HACC cluster.

Probably due to a compiler bug, the address of the ctrl_reg was NOT correct when this field is accessed via a method implemented inside the the bThread header (e.g. getCSR and setCSR) . To be precise, the address of ctrl_reg was shifted by 8 bytes (not the pointer ctrl_reg holds, but the address of the field itself!). The memory this field was then referencing was uninitialized in my case. Therefore, de-referencing the pointer ctrl_reg was supposed to hold, lead to a segmentation fault (due to ctrl_reg being NULL).

When using gcc, calling set/getCSR was still working fine inside the bThread header but broke as soon as the constructor finished (for example, when calling the same methods inside the cThread constructor or via cThread directly). Using clang, set/getCSR already broke inside the bThread constructor right after the bThread::mmapFpga() call finished.

This issue could be mitigated by:

  1. Adding a field right before the ctrl_reg
  2. Incrementing the address of the ctrl_reg field by 8 before performing the de-referencing inside getCSR and setCSR
  3. Moving the implementation from the bThread.hpp header to its implementation bThread.cpp

As it is best practice to have implementations inside the implementation and not the header, this PR provides the changes for the third mitigation to prevent similar problems in the future. With this mitigation, all addresses were correct again and everything worked as expected.

Type of change

  • Bug fix
  • New feature
  • Documentation update
  • A new research paper code implementation
  • Other

Tests & Results

I tested these changes on my Coyote project were they lead to registers now working properly!

Checklist

  • I have commented my code and made corresponding changes to the documentation.
  • I have added tests/results that prove my fix is effective or that my feature works.
  • My changes generate no new warnings or errors & all tests successfully pass.

I did not run any tests besides the one mentioned above. Unfortunately, I would not find instructions on how to run further tests (maybe I am blind). If you could give me guidance on which additional tests to run I am happy to run them as well!

@JonasDann JonasDann assigned JonasDann and bo3z and unassigned JonasDann Mar 25, 2025
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.

3 participants