-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Make the tests pass on a CHERI system. #2932
Conversation
Thanks for the PR. Could you elaborate why this test is failing on CHERI and in particular what |
I think that on CHERI there is extra data associated with a pointer, so the address is only a part of the value of a pointer. That is, sizeof(void*) is larger than the size needed to just store the address. On a CHERI system how do you find the number of bytes of an address? Using size_t seems weird to me but is that how the people writing compilers etc. for CHERI expect things to be done? I suppose sizeof(intptr_t) is also the wrong number of bytes (that is, not the same as the number of bytes in an address) on a CHERI system? |
On Morello, they are 16 and 8, respectively. On CHERI systems with 32-bit address spaces, they are 8 and 4.
Correct, a pointer stores an address, distance to the top and bottom and permissions.
There is a
The root of the problem is that C conflates the size of a type and the range of a type. I'd like to see something like |
test/format-test.cc
Outdated
EXPECT_EQ("0x" + std::string(sizeof(size_t) * CHAR_BIT / 4, 'f'), | ||
fmt::format("{0}", reinterpret_cast<void*>(~uintptr_t()))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest removing conditional compilation by switching from void*
to size_t
unconditionally as a more portable version (i.e. keeping just this block). uintptr_t
should probably be changed to size_t
as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And please add a short comment clarifying why we use size_t
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't use size_t
instead of uintptr_t
for the cast, they're different sizes on CHERI, if you want to cast a pointer to a size_t
it needs to be static_cast<size_t>(reinterpret_cast<uintptr_t>(ptr))
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't use size_t instead of uintptr_t for the cast
Right, please ignore this part of my comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Thank you! |
Opening this for discussion because I don't particularly like this as the 'fix':
On a CHERI system (and, indeed, any system where pointers are not simply addresses), the size of
void*
does not define the number of bytes of an address. The C++ standard conflates pointer (a thing that authorises access to an object) and an address (a location in some memory space that contains the object) in various ways.With this change, the test suite all passes on Arm's Morello system but I would prefer a cleaner solution.