[Mono-devel-list] (AMD64) Compiler Warnings (2)

[Willibald Krenn]

Paolo Molaro schrieb:

>>+		return (guint) GPOINTER_TO_UINT(value);
> The extra cast to guint is not needed.

Of course. :-(
Somehow I had the impression that the macro just casts to some 64 bit 
uint.. Will recheck everything.

>> struct MonoDebugLineNumberEntry {
>> 	guint32 offset;
>>-	guint32 address;
>>+	uintptr_t address;
> 
> 
> Unfortunately the names here are confusing: this is not a pointer-sized
> address, it is actually the offset in the native code from the start of
> the method (at least after a quick look at the code). So that change is not
> needed.
> A separate patch here could be done that changes the names of both fields to
> il_offset and native_offset.
> In other parts of the debug code it's used as a real address, though: this
> really requires one of the debug guys to fix the mess. Just changing
> the types to uintptr_t is not going to help.

Ok, so I'll revert that to guint32 and add a *big* FIXME.
But this can break the code on 64 bit architectures as long as the debug 
code uses this field as real address?!


>>+#include <intps.h>
> 
> mono-debug.h is an installed header and you can't add headers that require config.h 
> to it.

That complicates matters. IIRC I need uintptr_t in that header for some 
function arguments that take some address..
I guess I'll use GLIB_SIZEOF_VOID_P instead of SIZEOF_VOID_P in intps.h 
to test for 64 bit and additionally I'll map the [u]intptr types to glib 
types, so that should remove the config.h dependance..


>>-	g_error ("Invalid IL code at IL%04x in %s: %s\n", ip - header->code, 
>>+	g_error ("Invalid IL code at IL%04"x_INT_PTR_T" in %s: %s\n", ip - header->code,
> 
> 
> Here and in other places like this, the correct thing to do is to change the code
> to cast the pointer difference to an int:
> 	g_error ("Invalid IL code at IL%04x in %s: %s\n", (int)(ip - header->code),
> because we're not going to have IL methods bigger than 2 gigabytes.

Ok, I'll change that in all cases where it's obvious (to me).. Most of 
these changes, however, print arguments of type g[s]size - and this is 
64 bits on AMD64. Shall I truncate these types to 32 bit too?


>> 	/* The address of the generated code, used for fixups */
>>-	int native_offset;
>>-	int max_offset;
>>+	intptr_t native_offset;
>>+	intptr_t max_offset;
> 
> 
> They are offsets, the ints are fine.

ok.


> 
>>Index: mono/mini/mini-exceptions.c
>>===================================================================
>> 					/* Switch back to normal stack */
>> 					if (stack_overflow)
>> 						/* Free up some stack space */
>>-						MONO_CONTEXT_SET_SP (&initial_ctx, (guint32)(MONO_CONTEXT_GET_SP (&initial_ctx)) + (64 * 1024));
>>-					MONO_CONTEXT_SET_IP (&initial_ctx, (unsigned int)jit_tls->abort_func);
>>+						MONO_CONTEXT_SET_SP (&initial_ctx, GPOINTER_TO_UINT(MONO_CONTEXT_GET_SP (&initial_ctx)) + (64 * 1024));
>>+					MONO_CONTEXT_SET_IP (&initial_ctx, GPOINTER_TO_UINT(jit_tls->abort_func));
> 
> 
> The code here is broken and the change just masks the bug. abort_func 
> should not be cast to a uint: it is a pointer.


A cast to uintptr_t should work, I guess..


>>	#if SIZEOF_VOID_P == 8
>>		/*only applies if sizeof(void*)==8*/
>>		#define d_INT_PTR_T "ld"
>>		#define d_INT_64    "ld"
>>		#define u_INT_PTR_T "lu"
>>		#define u_INT_64    "lu"
>>		#define x_INT_PTR_T "lx"
>>		#define x_INT_64    "lx"
>>	#else
>>		/*else switch back to normal*/
>>		#define d_INT_PTR_T "d"
>>		#define d_INT_64    "lld"
>>		#define u_INT_PTR_T "u"
>>		#define u_INT_64    "llu"
>>		#define x_INT_PTR_T "x"
>>		#define x_INT_64    "llx"
>>	#endif
> 
> 
> I don't like the macro names and they don't follow the convention
> in mono: they pullute the namespace. Maybe MONO_FMT_PTR/MONO_FMT_64
> and leave the d/u/x format in the string.

You mean something like


#if GLIB_SIZEOF_VOID_P == 8
	/*only applies if sizeof(void*)==8*/
	#define MONO_FMT_PTR "l"
	#define MONO_FMT_64  "l"
#else
	/*else switch back to normal*/
	#define MONO_FMT_PTR ""
	#define MONO_FMT_64  "ll"
#endif

printf ("some %"MONO_FMT_64"x",some guint64);


Thanks a lot for your feedback, will redo my patch,
  Willi