[PATCH] add support for -Wmismatched-dealloc

[Martin Sebor]

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
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: glibc-attr-malloc.diff
Type: text/x-patch
Size: 16904 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/libc-alpha/attachments/20210104/2992b733/attachment-0001.bin>