Skip to content

Fix ENABLE_CHECKED_CASTS build and transform it into ENABLE_CHECKED_BUILDS_ option #91630

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Sep 7, 2023
Merged

Fix ENABLE_CHECKED_CASTS build and transform it into ENABLE_CHECKED_BUILDS_ option #91630

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
138cd3f
Fix ENABLE_CHECKED_CASTS build and transform it into ENABLE_CHECKED_B…
akoeplinger Sep 5, 2023
89dab50
Fix setting ENABLE_CHECKED_BUILD_CASTS in config.h and cleanup duplic…
akoeplinger Sep 6, 2023
e80b894
Allow turning on checked build checks in Release config
akoeplinger Sep 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 3 additions & 15 deletions src/mono/cmake/config.h.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -885,21 +885,6 @@
/* Enable experiment 'Tiered Compilation' */
#cmakedefine ENABLE_EXPERIMENT_TIERED 1

/* Enable checked build */
#cmakedefine ENABLE_CHECKED_BUILD 1

/* Enable GC checked build */
#cmakedefine ENABLE_CHECKED_BUILD_GC 1

/* Enable metadata checked build */
#cmakedefine ENABLE_CHECKED_BUILD_METADATA 1

/* Enable thread checked build */
#cmakedefine ENABLE_CHECKED_BUILD_THREAD 1

/* Enable private types checked build */
#cmakedefine ENABLE_CHECKED_BUILD_PRIVATE_TYPES 1

/* Enable EventPipe library support */
#cmakedefine ENABLE_PERFTRACING 1

Expand Down Expand Up @@ -942,6 +927,9 @@
/* Enable runtime checks of mempool references between metadata images (must set env var MONO_CHECK_MODE=metadata) */
#cmakedefine ENABLE_CHECKED_BUILD_METADATA 1

/* Enable runtime checks of casts between types */
#cmakedefine ENABLE_CHECKED_BUILD_CASTS 1

/* Enable static linking of mono runtime components */
#cmakedefine STATIC_COMPONENTS

Expand Down
3 changes: 2 additions & 1 deletion src/mono/cmake/options.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ option (ENABLE_CHECKED_BUILD_PRIVATE_TYPES "Enable compile time checking that ge
option (ENABLE_CHECKED_BUILD_GC "Enable runtime GC Safe / Unsafe mode assertion checks (must set env var MONO_CHECK_MODE=gc)")
option (ENABLE_CHECKED_BUILD_THREAD "Enable runtime history of per-thread coop state transitions (must set env var MONO_CHECK_MODE=thread)")
option (ENABLE_CHECKED_BUILD_METADATA "Enable runtime checks of mempool references between metadata images (must set env var MONO_CHECK_MODE=metadata)")
option (ENABLE_CHECKED_BUILD_CASTS "Enable runtime checks of casts between types")
option (ENABLE_MSCORDBI "Generate mscordbi to support icordbg interface")
option (ENABLE_SMALL_CONFIG "Reduce runtime requirements (and capabilities)")
option (ENABLE_DTRACE "Enable DTrace probes")
Expand All @@ -60,7 +61,7 @@ option (ENABLE_WEBCIL "Enable the WebCIL loader")

set (MONO_GC "sgen" CACHE STRING "Garbage collector implementation (sgen or boehm). Default: sgen")
set (GC_SUSPEND "default" CACHE STRING "GC suspend method (default, preemptive, coop, hybrid)")
set (CHECKED_BUILD "" CACHE STRING "Set ENABLE_CHECKED_BUILD_ options at once. Comma-separated list of lowercase ENABLE_CHECKED_BUILD_ options ie. 'gc,threads,private_types' etc.")
set (CHECKED_BUILD "" CACHE STRING "Set ENABLE_CHECKED_BUILD_ options at once. Comma-separated list of lowercase ENABLE_CHECKED_BUILD_ options ie. 'private_types,gc,thread,metadata,casts' etc.")
set (ENABLE_MINIMAL "" CACHE STRING "Set many DISABLE_ options at once. Comma-separated list of lowercase DISABLE_ options ie. 'jit,simd' etc.")
set (AOT_TARGET_TRIPLE "" CACHE STRING "Target triple for AOT cross compiler")
set (AOT_OFFSETS_FILE "" CACHE STRING "Offsets file for AOT cross compiler")
Expand Down
2 changes: 1 addition & 1 deletion src/mono/mono/eglib/ghashtable.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -648,7 +648,7 @@ g_direct_equal (gconstpointer v1, gconstpointer v2)
guint
g_direct_hash (gconstpointer v1)
{
return GPOINTER_TO_UINT (v1);
return GCONSTPOINTER_TO_UINT (v1);
}

gboolean
Expand Down
12 changes: 9 additions & 3 deletions src/mono/mono/eglib/glib.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1314,8 +1314,7 @@ gint
g_clock_nanosleep (clockid_t clockid, gint flags, const struct timespec *request, struct timespec *remain);
#endif

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

@BrzVlad BrzVlad Sep 6, 2023

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

@lateralusX lateralusX Sep 6, 2023

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.


#define __CAST_PTRTYPE_TO_STYPE(src,dest,min_v,max_v) \
static inline dest \
Expand Down Expand Up @@ -1400,6 +1399,9 @@ __CAST_PTRTYPE_TO_UTYPE(gpointer, gulong, ULONG_MAX)
__CAST_PTRTYPE_TO_STYPE(gpointer, gint, INT_MIN, INT_MAX)
__CAST_PTRTYPE_TO_UTYPE(gpointer, guint, UINT_MAX)

__CAST_PTRTYPE_TO_STYPE(gconstpointer, gint, INT_MIN, INT_MAX)
__CAST_PTRTYPE_TO_UTYPE(gconstpointer, guint, UINT_MAX)

__CAST_PTRTYPE_TO_STYPE(gintptr, gint32, INT32_MIN, INT32_MAX)
__CAST_PTRTYPE_TO_UTYPE(gintptr, guint32, UINT32_MAX)
__CAST_PTRTYPE_TO_STYPE(gintptr, gint16, INT16_MIN, INT16_MAX)
Expand Down Expand Up @@ -1450,6 +1452,7 @@ __CAST_STYPE_TO_UTYPE(gssize, gsize, SIZE_MAX)
__CAST_UTYPE_TO_STYPE(gsize, gssize, PTRDIFF_MIN, PTRDIFF_MAX)

__CAST_STYPE_TO_UTYPE(glong, gulong, ULONG_MAX)
__CAST_STYPE_TO_UTYPE(glong, guint32, UINT32_MAX)
__CAST_UTYPE_TO_STYPE(gulong, glong, LONG_MIN, LONG_MAX)

__CAST_STYPE_TO_STYPE(gdouble, gint64, INT64_MIN, INT64_MAX)
Expand Down Expand Up @@ -1558,7 +1561,7 @@ __CAST_UTYPE_TO_STYPE(gunichar, gchar, CHAR_MIN, CHAR_MAX)

#endif

#if !defined(ENABLE_CHECKED_CASTS)
#if !defined(ENABLE_CHECKED_BUILD_CASTS)

#define G_CAST_PTRTYPE_TO_STYPE(src,dest,v) ((dest)(gssize)(v))
#define G_CAST_PTRTYPE_TO_UTYPE(src,dest,v) ((dest)(gsize)(v))
Expand Down Expand Up @@ -1588,6 +1591,9 @@ __CAST_UTYPE_TO_STYPE(gunichar, gchar, CHAR_MIN, CHAR_MAX)
#define GPOINTER_TO_INT(v) G_CAST_PTRTYPE_TO_STYPE(gpointer, gint, v)
#define GPOINTER_TO_UINT(v) G_CAST_PTRTYPE_TO_UTYPE(gpointer, guint, v)

#define GCONSTPOINTER_TO_INT(v) G_CAST_PTRTYPE_TO_STYPE(gconstpointer, gint, v)
#define GCONSTPOINTER_TO_UINT(v) G_CAST_PTRTYPE_TO_UTYPE(gconstpointer, guint, v)

#define GINTPTR_TO_INT32(v) G_CAST_PTRTYPE_TO_STYPE(gintptr, gint32, v)
#define GINTPTR_TO_UINT32(v) G_CAST_PTRTYPE_TO_UTYPE(gintptr, guint32, v)

Expand Down
2 changes: 1 addition & 1 deletion src/mono/mono/eventpipe/ep-rt-mono-runtime-provider.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3979,7 +3979,7 @@ fire_gc_event_bulk_root_edge (
break;
case MONO_ROOT_SOURCE_GC_HANDLE :
root_kind = GC_ROOT_KIND_HANDLE;
root_flags = GPOINTER_TO_INT (gc_root->key) != 0 ? GC_ROOT_FLAGS_PINNING : GC_ROOT_FLAGS_NONE;
root_flags = GCONSTPOINTER_TO_INT (gc_root->key) != 0 ? GC_ROOT_FLAGS_PINNING : GC_ROOT_FLAGS_NONE;
root_id = address;
break;
case MONO_ROOT_SOURCE_HANDLE :
Expand Down
2 changes: 1 addition & 1 deletion src/mono/mono/metadata/metadata.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7805,7 +7805,7 @@ guint
mono_aligned_addr_hash (gconstpointer ptr)
{
/* Same hashing we use for objects */
return (GPOINTER_TO_UINT (ptr) >> 3) * 2654435761u;
Copy link
Member

@BrzVlad BrzVlad Sep 6, 2023

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 •
edited
Loading

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.

return (GCONSTPOINTER_TO_UINT (ptr) >> 3) * 2654435761u;
}

/*
Expand Down
4 changes: 2 additions & 2 deletions src/mono/mono/mini/aot-runtime.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6032,15 +6032,15 @@ mono_aot_get_unbox_arbitrary_trampoline (gpointer addr)
static int
i32_idx_comparer (const void *key, const void *member)
{
gint32 idx1 = GPOINTER_TO_INT (key);
gint32 idx1 = GCONSTPOINTER_TO_INT (key);
gint32 idx2 = *(gint32*)member;
return idx1 - idx2;
}

static int
ui16_idx_comparer (const void *key, const void *member)
{
int idx1 = GPOINTER_TO_INT (key);
int idx1 = GCONSTPOINTER_TO_INT (key);
int idx2 = *(guint16*)member;
return idx1 - idx2;
}
Expand Down
2 changes: 1 addition & 1 deletion src/mono/mono/mini/mini-runtime.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1239,7 +1239,7 @@ mono_patch_info_hash (gconstpointer data)
case MONO_PATCH_INFO_AOT_JIT_INFO:
case MONO_PATCH_INFO_METHOD_PINVOKE_ADDR_CACHE:
case MONO_PATCH_INFO_GSHARED_METHOD_INFO:
return hash | GPOINTER_TO_UINT (ji->data.target);
return hash | GCONSTPOINTER_TO_UINT (ji->data.target);
case MONO_PATCH_INFO_GSHAREDVT_CALL:
return hash | GPOINTER_TO_UINT (ji->data.gsharedvt->method);
case MONO_PATCH_INFO_RGCTX_FETCH:
Expand Down
12 changes: 6 additions & 6 deletions src/mono/mono/mini/unwind.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -725,8 +725,8 @@ static guint
cached_info_hash(gconstpointer key)
{
guint i, a;
const guint8 *info = cached_info [GPOINTER_TO_UINT (key)].info;
const guint len = cached_info [GPOINTER_TO_UINT (key)].len;
const guint8 *info = cached_info [GCONSTPOINTER_TO_UINT (key)].info;
const guint len = cached_info [GCONSTPOINTER_TO_UINT (key)].len;

for (i = a = 0; i != len; ++i)
a ^= (((guint)info [i]) << (i & 0xf));
Expand All @@ -737,11 +737,11 @@ cached_info_hash(gconstpointer key)
static gboolean
cached_info_eq(gconstpointer a, gconstpointer b)
{
const guint32 lena = cached_info [GPOINTER_TO_UINT (a)].len;
const guint32 lenb = cached_info [GPOINTER_TO_UINT (b)].len;
const guint32 lena = cached_info [GCONSTPOINTER_TO_UINT (a)].len;
const guint32 lenb = cached_info [GCONSTPOINTER_TO_UINT (b)].len;
if (lena == lenb) {
const guint8 *infoa = cached_info [GPOINTER_TO_UINT (a)].info;
const guint8 *infob = cached_info [GPOINTER_TO_UINT (b)].info;
const guint8 *infoa = cached_info [GCONSTPOINTER_TO_UINT (a)].info;
const guint8 *infob = cached_info [GCONSTPOINTER_TO_UINT (b)].info;
if (memcmp (infoa, infob, lena) == 0)
return TRUE;
}
Expand Down
2 changes: 1 addition & 1 deletion src/mono/mono/sgen/sgen-gc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ extern unsigned int sgen_global_stop_count;
static inline guint
sgen_aligned_addr_hash (gconstpointer ptr)
{
return GPOINTER_TO_UINT (ptr) >> 3;
return GCONSTPOINTER_TO_UINT (ptr) >> 3;
}

#define SGEN_PTR_IN_NURSERY(p,bits,start,end) (((mword)(p) & ~(((mword)1 << (bits)) - 1)) == (mword)(start))
Expand Down
4 changes: 2 additions & 2 deletions src/mono/mono/utils/mono-machine.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ typedef gssize host_mgreg_t;
typedef gsize host_umgreg_t;
#endif

#ifdef ENABLE_CHECKED_CASTS
#ifdef ENABLE_CHECKED_BUILD_CASTS
__CAST_STYPE_TO_STYPE(host_mgreg_t, gint32, INT32_MIN, INT32_MAX)
__CAST_STYPE_TO_UTYPE(host_mgreg_t, guint32, UINT32_MAX)
__CAST_STYPE_TO_STYPE(host_mgreg_t, gint16, INT16_MIN, INT16_MAX)
Expand Down Expand Up @@ -69,7 +69,7 @@ typedef gint32 target_mgreg_t;
typedef gint64 target_mgreg_t;
#endif

#ifdef ENABLE_CHECKED_CASTS
#ifdef ENABLE_CHECKED_BUILD_CASTS
__CAST_STYPE_TO_STYPE(target_mgreg_t, gint32, INT32_MIN, INT32_MAX)
__CAST_STYPE_TO_UTYPE(target_mgreg_t, guint32, UINT32_MAX)
__CAST_STYPE_TO_STYPE(target_mgreg_t, gint16, INT16_MIN, INT16_MAX)
Expand Down