Re: [RFC] Cygwin libstdc++ plan (operator new/delete replacement)

[Christopher Faylor <cgf-use-the-mailinglist-please at cygwin dot com>]

On Sun, Jun 28, 2009 at 06:47:51PM +0100, Dave Korn wrote:
>Christopher Faylor wrote:
>> On Sun, Jun 28, 2009 at 02:47:24PM +0100, Dave Korn wrote:
>>> to the linker spec.  If we agree this is a reasonable approach to take, I
>>> could rush out a gcc4-4.3.2-3 update to add this very quickly.
>>>
>>>  So, would everyone be happy to do it this way?
>> 
>> It looks ok with a couple of caveats/questions below.
>> 
>> Would it make sense to standardize on this approach for the malloc
>> functions too and remove the malloc_wrapper file?
>
>Well...  "not broke, don't fix"?  That would be my preferred option
>anyway.

My goal here is to see if we really need to have two different ways of
doing the same thing in the cygwin DLL.  Since malloc_wrapper.cc
attempts to do the same thing as what you're doing, it seemed like we
should just standardize on one approach if possible.  If it isn't
possible then nevermind.

>>> +__wrap__ZdaPv NOSIGFE
>>> +__wrap__ZdaPvRKSt9nothrow_t NOSIGFE
>>> +__wrap__ZdlPv NOSIGFE
>>> +__wrap__ZdlPvRKSt9nothrow_t NOSIGFE
>>> +__wrap__Znaj NOSIGFE
>>> +__wrap__ZnajRKSt9nothrow_t NOSIGFE
>>> +__wrap__Znwj NOSIGFE
>>> +__wrap__ZnwjRKSt9nothrow_t NOSIGFE
>> 
>> Could we add comments whereever these mangled functions are referenced
>> showing what they really refer to when they are demangled?
>
>Oh, can we do comments in the .din file?  I noticed we already have
>"dll_crt0__FP11per_process" in there and just did what it did; the only
>other places these names are used are on prototypes, which serve as
>well as comments for that purpose.

Yes, sorry.  I knew that the .def file took comments but there was a bug
in the gendef script which got confused by both comments and trailing
spaces (!).  That should be fixed now and I've added a needed comment
for that entry.

So, if you could add comments in your eventual patch it would be
appreciated.

>>> Index: winsup/cygwin/globals.cc
>
>> See below.
>
>>> -  DWORD unused[7];
>>> +  DWORD unused[6];
>>> +  struct per_process_cxx_malloc *cxx_malloc;
>> 
>> This should go before the unused rather than after.
>
>  Rightyho, will do.  (Side note: are you sure it's for the best though?  I
>figured by allocating from the end, we wouldn't change the offset of the
>unused[] array within the struct.  But I guess I can't think of a situation
>where that would cause anything to break that wasn't already broke).

I thought there was precedent for putting it first but the unused field
has oscillated for years.  How about if we just leave it alone and
repurpose the forkee field?  That really should have been pulled into
unused a while ago so we can kill two birds by just eliminating it with
your patch.

(And this time I actually did check to make sure that this would work.
Eliminating the forkee field does not cause any obvious problems
building the cygwin DLL)

>>>   if (u != NULL)
>>>     uwasnull = 0;	/* Caller allocated space for per_process structure */
>>> -  else if ((newu = cygwin_internal (CW_USER_DATA)) == (DWORD) -1)
>>> +  else if (~0u == (DWORD) newu)
>> 
>> I really detest the 0 == variable usage.
>
>  That's not quite what the above says.
>
>>  I know why people use it (yes,
>> I really do) but I'd rather not see it in Cygwin source.
>
>  Do you mean the ordering of constant-before-variable in the == comparison,
>or are you referring to comparing == 0 as opposed to using the logical not
>operator (having overlooked the "~" operator)?  Either way, I'll word it
>however you like.

I don't like constant before variable.  As I said, I know why people do
this and I'm carefully trying not to start a religious debate.

>> But, the good news is that you don't really need it since we're getting
>> rid of old Cygwin cruft anyway so this should be gone for 1.7.
>
>  Ah, of course, we can assume for certain that cygwin_internal (CW_USER_DATA)
>won't fail on any 1.7 version!  I'll remove the check-for-fail altogether when
>I respin it.

Thanks.  We probably should have done this some time ago.

>>> @@ -84,6 +109,27 @@ _cygwin_crt0_common (MainFunc f, per_pro
>>>   u->realloc = &realloc;
>>>   u->calloc = &calloc;
>>>
>>> +  /* Likewise for the C++ memory operators - if any.  */
>>> +  if (newu && newu->cxx_malloc)
>>> +    {
>>> +      /* Inherit what we don't override.  */
>>> +#define CONDITIONALLY_OVERRIDE(MEMBER) \
>>> +      if (!__cygwin_cxx_malloc.MEMBER) \
>>> +	__cygwin_cxx_malloc.MEMBER = newu->cxx_malloc->MEMBER;
>>> +      CONDITIONALLY_OVERRIDE(oper_new);
>>> +      CONDITIONALLY_OVERRIDE(oper_new__);
>>> +      CONDITIONALLY_OVERRIDE(oper_delete);
>>> +      CONDITIONALLY_OVERRIDE(oper_delete__);
>>> +      CONDITIONALLY_OVERRIDE(oper_new_nt);
>>> +      CONDITIONALLY_OVERRIDE(oper_new___nt);
>>> +      CONDITIONALLY_OVERRIDE(oper_delete_nt);
>>> +      CONDITIONALLY_OVERRIDE(oper_delete___nt);
>>> +    }
>> 
>> I don't suppose there's any way that the above could be in some sort
>> of array so that the array could just be extended and new functions
>> handled automatically?
>
>  I think it's important to keep the correctly-typed pointers-to-functions,
>but I guess I could union them with an array of void* or something like that
>so this part can be a for-loop, sure.

This was only an idle thought.  If you think it's better to do it this way
that's fine.

cgf