Re: [Valgrind-developers] coregrind: semaphore remapping rewritten

[Jeremy F. <je...@go...>]

On Mon, 2004-03-15 at 02:09, Aleksander Salwa wrote:
> Here is a patch that implements semaphore remapping in constant time.
> Function sem_destroy really works now. Applications that create and
> destroy a lot of semaphores will work (you may still need to increase
> VG_N_SEMAPHORES if you use a lot of semaphores at a time).
> 
> Performance is better because of:
> - simple pointer look-ups in se_new, se_remap, se_unmap - no linear
>   search
> - locking is no longer needed in se_remap
> 
> Storing pointers to V's internal data in user's sem_t seems to be an
> advantage, not disadvantage, because:
> - now Valgrind can detect overwrites of user's sem_t (additional error
>   checking - SEM_CHECK_MAGIC) - previously it would go undetected
> - if invalid pointer is passed to one of sem_xxx functions, Valgrind
>   will report invalid memory access in se_remap or se_new - previously
>   se_remap would silently allocate new semaphore
> 
> Patch is attached. Could someone with write access commit it into CVS ?

It looks pretty good, but I have a few comments:

Why still keep the big array?  Can't this be made dynamic so that
there's no need for VG_N_SEMAPHORES?

Also, barf() seems to be mostly used for reporting vg_libpthread
internal errors, where as the places you use it seem to be all client
bugs; maybe just using VG_INTERNAL_PRINTF_BACKTRACE.

And finally, could you write a regression test case and add it to
none/tests?  Something which would have failed before, but now works,
and exercises as much of the semaphore machinery as possible.

Thanks,
	J