Repository navigation
cycleclock: Fix type conversion to match function return type on riscv64 - #1802
Conversation
Fixes builds with clang
src/cycleclock.h:213:10: error: implicit conversion changes signedness: 'uint64_t' (aka 'unsigned long') to 'int64_t' (aka 'long') [-Werror,-Wsign-conversion]
213 | return cycles;
| ~~~~~~ ^~~~~~
1 error generated.
| uint64_t cycles; | ||
| asm volatile("rdtime %0" : "=r"(cycles)); | ||
| return cycles; | ||
| return static_cast<int64_t>(cycles); |
There was a problem hiding this comment.
maybe we should check the range before the cast?
There was a problem hiding this comment.
We don't do that anywhere else though. I'm not sure what we would even do in that situation?
There was a problem hiding this comment.
assert i guess? the C++ style guide talks about this a bit in discussing avoidance of unsigned types ("In particular, do not use unsigned types to say a number will never be negative. Instead, use assertions for this.")
but i agree we don't currently do it anywhere else. happy to punt this to another PR.
There was a problem hiding this comment.
https://riscv.org/wp-content/uploads/2016/06/riscv-spec-v2.1.pdf, page 32:
The RDTIME pseudo-instruction reads the low XLEN bits of the time CSR, which counts wall-clock
real time that has passed from an arbitrary start time in the past. RDTIMEH is an RV32I-only instruction that reads bits 63–32 of the same real-time counter. The underlying 64-bit counter should
never overflow in practice.
At 10GHz clock rate, it would take ~30 years of uptime to overflow.
|
thanks! |
Fixes builds with clang
src/cycleclock.h:213:10: error: implicit conversion changes signedness: 'uint64_t' (aka 'unsigned long') to 'int64_t' (aka 'long') [-Werror,-Wsign-conversion]
213 | return cycles;
| ~~~~~~ ^~~~~~
1 error generated.