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

Need help understanding the parsing logic. #157

Closed
amokfa opened this issue Dec 30, 2024 · 1 comment
Closed

Need help understanding the parsing logic. #157

amokfa opened this issue Dec 30, 2024 · 1 comment

Comments

@amokfa
Copy link
Contributor

amokfa commented Dec 30, 2024

Hi. Thanks for the wonderful library. I was going through SignalImpl.cpp and couldn't understand the signed integer parsing logic.

_mask = (1ull << (_bit_size - 1ull) << 1ull) - 1;

Could this expression be simplified to:

// Create a mask with _bit_size bits set
_mask = (1ull << _bit_size) - 1;

or there is some bit magic or edge cases in the DBC spec that I'm unaware of. I can raise a PR with simplified code or with documentation.

@xR3b0rn
Copy link
Owner

xR3b0rn commented Jan 2, 2025

You need to handle it this way due to how the x64 shift instruction behaves:

int main() {
    volatile std::size_t bit_size = 64ull;
    std::cout << (1ull << bit_size) - 1 << std::endl;
}

This small example program will output 0.

The reason for this is that, on x64 architectures, the CPU instruction for shifting only considers the first six bits of the second shift operand. Effectively, the CPU performs the following operation:

(1ull << 0) - 1

That means your rearranged formula would produce an incorrect result in the special case of a bit size of 64.

Interestingly, replacing the volatile keyword by constexpr will make g++ produce a program that behaves as expected.

However, to ensure the CPU behaves as intended in all cases, you need to write the code as demonstrated in SignalImpl.cpp.

@amokfa amokfa closed this as completed Jan 6, 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

No branches or pull requests

2 participants