Skip to content

Conversation

@akoeplinger
Copy link
Member

@akoeplinger akoeplinger commented Sep 5, 2023

Companion to #91395. Removes some duplicate defines and allows CHECKED_BUILD to be turned on in Release config.


//#define ENABLE_CHECKED_CASTS
#ifdef ENABLE_CHECKED_CASTS
#ifdef ENABLE_CHECKED_BUILD_CASTS
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What exactly is this checked option doing ? The defines below seem to not do anything in case of overflow.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no actions hooked up to these checks at the moment, so we would need to implement valid actions in case of underflow/owerflow when we would start to use them. When I prepared for them a year ago I know we have some casts that will trigger the checks so we probably need to start logging or similar to get a better understanding where we do potential dangerous casts and where the underflow/overflow might be excepted.

mono_aligned_addr_hash (gconstpointer ptr)
{
/* Same hashing we use for objects */
return (GPOINTER_TO_UINT (ptr) >> 3) * 2654435761u;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overflowing here is expected

Copy link
Member Author

@akoeplinger akoeplinger Sep 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I suspect we'd need to audit every usage if/when we turn on validation, but right now this at least gives us all the places where conversions are happening.

@akoeplinger akoeplinger requested a review from BrzVlad September 6, 2023 15:35
@akoeplinger akoeplinger merged commit 34dd479 into dotnet:main Sep 7, 2023
@akoeplinger akoeplinger deleted the fix-enable-checked-casts branch September 7, 2023 13:21
@ghost ghost locked as resolved and limited conversation to collaborators Oct 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants