Ping: [PATCH] add support for -Wmismatched-dealloc

[Martin Sebor]

Florian, do you have any outstanding concerns or is the most recent
patch good to go?

Thanks
Martin

On 1/4/21 4:18 PM, Martin Sebor wrote:
> On 1/4/21 9:57 AM, Florian Weimer wrote:
>> * Martin Sebor:
>>
>>> On 1/4/21 9:07 AM, Florian Weimer wrote:
>>>> * Martin Sebor via Libc-alpha:
>>>>
>>>>> diff --git a/wcsmbs/wchar.h b/wcsmbs/wchar.h
>>>>> index 9cf8b05a87..4c1c7f1119 100644
>>>>> --- a/wcsmbs/wchar.h
>>>>> +++ b/wcsmbs/wchar.h
>>>>> @@ -151,7 +151,8 @@ extern size_t wcsxfrm_l (wchar_t *__s1, const 
>>>>> wchar_t *__s2,
>>>>>                 size_t __n, locale_t __loc) __THROW;
>>>>>      /* Duplicate S, returning an identical malloc'd string.  */
>>>>> -extern wchar_t *wcsdup (const wchar_t *__s) __THROW 
>>>>> __attribute_malloc__;
>>>>> +extern wchar_t *wcsdup (const wchar_t *__s) __THROW
>>>>> +  __attribute_malloc__ __attr_dealloc_free;
>>>>>    #endif
>>>>>      /* Find the first occurrence of WC in WCS.  */
>>>>> @@ -562,9 +563,18 @@ extern wchar_t *wcpncpy (wchar_t *__restrict 
>>>>> __dest,
>>>>>    /* Wide character I/O functions.  */
>>>>>      #if defined __USE_XOPEN2K8 || __GLIBC_USE (LIB_EXT2)
>>>>> +#  ifdef __REDIRECT
>>>>> +/* Declare the __fclose alias and associate it as a deallocator
>>>>> +   with open_wmemstream below.  */
>>>>> +extern int __REDIRECT (__fclose, (FILE *), fclose);
>>>>> +#    define __attr_dealloc_fclose __attr_dealloc (__fclose, 1)
>>>>> +#  else
>>>>> +#    define __attr_dealloc_fclose /* empty */
>>>>> +#  endif
>>>>>    /* Like OPEN_MEMSTREAM, but the stream is wide oriented and 
>>>>> produces
>>>>>       a wide character string.  */
>>>>> -extern __FILE *open_wmemstream (wchar_t **__bufloc, size_t 
>>>>> *__sizeloc) __THROW;
>>>>> +extern __FILE *open_wmemstream (wchar_t **__bufloc, size_t 
>>>>> *__sizeloc) __THROW
>>>>> +  __attribute_malloc__ __attr_dealloc_fclose;
>>>>>    #endif
>>>>>      #if defined __USE_ISOC95 || defined __USE_UNIX98
>>>> Why is an alias for fclose needed here, but not for free?
>>>
>>> Because fclose is not a built-in so there's no __builtin_fclose
>>> to associate open_wmemstream with.  free is a built-in and so
>>> __attr_dealloc_free just references __builtin_free and doesn't
>>> need an explicit declaration.
>>
>> Ahh, that explains the discrepancy.
>>
>> I'm a bit worried that the __fclose alias causes problems.  Would it be
>> possible to add __builtin_fclose to GCC instead?
> 
> I don't like the alias hack either.  Adding a built-in is possible
> but it's late in GCC stage 3 and I'm doubtful the change would be
> accepted before the Glibc deadline (that's this week, right?)
> 
> The alias isn't necessary in <stdio.h> where fclose is declared
> so I've removed it from there.  It would have been only marginally
> useful in <wchar.h>, and only when fclose isn't declared, but it's
> probably best avoided.  So one possibility is to prepare the header
> for __builtin_fclose if/when it's added, fall back on fclose when
> it's declared (i.e., when <stdio.h> is included), and do nothing
> otherwise (and accept that calling, say free, on a pointer returned
> from open_wmemstteam, will not be diagnosed in those translation
> units).
> 
> Attached is a patch that does that.  If you want to change it
> to something else (e.g, forget about open_wmemstream altogether
> for now or conditionally declare it in <stdio.h> when <wchar.h>
> is included, or any other viable alternative) please let me know.
> 
>> Based on how this patch appears to make both __fclose and fclose
>> acceptable as a deallocator, GCC resolves redirects as part of the
>> matching check.  I wonder if this constrains the usefulness of the
>> attribute in some way.  I can imagine situations where at the source
>> level, different deallocators should be used (say to support debugging
>> builds), but release builds redirect different deallocators to the same
>> implementation.
> 
> The attribute doesn't do anything special with aliases (it was
> just a way to get around the problem with functions not being
> declared in some headers).
> 
> As for different configurations, the attribute is designed for
> standard C/POSIX APIs and others like those.  A user-defined API
> with different deallocators in one configuration than in another
> has to create the associations conditionally, based on those
> configurations.  But there's no way to change these associations
> for GCC built-ins, just like there's no way to change their
> semantics.  They're hardwired into GCC and the only way to
> affect them is to disable the built-ins.
> 
> Martin
> 
>>
>> Thanks,
>> Florian
>>
>