This is the mail archive of the cygwin-developers mailing list for the Cygwin project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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.

>> +__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.

>> 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).

>>   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.

> 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.

>> @@ -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.

  Righto.  Respin will follow in a day or two as a submission to the -patches
list.  Thanks for the fast review.

    cheers,
      DaveK


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]