Re: forced casts question

Etienne Gagnon wrote:
> Chris Pickett wrote:
> 
>> -    _svmt_stack_frame *frame = (_svmt_stack_frame *)
>> +    _svmt_stack_frame *frame = (_svmt_stack_frame *) (void *)
>>        (((char *) env->stack.current_frame) + offset);
>>
>> I think this was to "eliminate tons of spurious warnings" (r3144), but 
>> why were such things causing warnings?  Isn't it good to let the 
>> compiler help you catch casting errors?
> 
> 
> The casting error was about possible mi-alignment of the pointer, after 
> casting.  In this case, as SableVM's stack frame computations are always 
> done such as to guarantee correct alignment, the warning was spurious. 
> Such spurious warnings, when issued in great quantity, can hide real 
> warnings.  This is why I got rid of them using the ugly (and only) trick 
> that allowed enabling GCC's cast warnings without being riddled with 
> tons of spurious warnings.  I did check every single cast before adding 
> "(void *)", to make sure I was not actually hiding a real problem.

Okay, thanks.

> Please note that for any new code, in general, it is *NOT* an accepted 
> practice to write type casts directly in SableVM.  One should instead 
> add an entry in cast.list and use the generated _svmf_cast_XXX() 
> function to do a cast.

I have not been using the .list files (casting, memory allocation) 
because it has typically been too much of a pain trying to make them and 
types.h work with conditional compilation.  In general, is it okay to 
introduce types and utility functions into the main VM even if they're 
only going to be used under certain builds (in my case if _SABLEVM_SPMT 
is defined)?  Or should I make new files for all such things?

> [I wonder if it would not be a good idea to even create special cast 
> functions for computing the frame, local and stack pointers, and this 
> centralize these dangerous hacks, while adding some additional type 
> safety.]

I have thought about it before -- it's always the same code, everywhere. 
  Maybe m4 macros instead of functions.  I think it might be a problem 
with *stack and *locals in _svmf_interpreter() since they're declared 
with the register keyword (OTOH, I'm not so sure this keyword actually 
does anything useful), because gcc doesn't like it if you pass registers 
as function arguments.

The reasons not to do this are that 1) the code is so short, and 2) it's 
really all over the place.  Being selfish, I'd rather see your energy / 
brainpower go towards fixing locking problems (still present).

Chris