Date: Sun, 21 Jan 2018 12:59:12 -0500 From: Pedro Giffuni <pfg@FreeBSD.org> To: Mark Millard <marklmi26-fbsd@yahoo.com> Cc: FreeBSD Toolchain <freebsd-toolchain@freebsd.org>, FreeBSD Hackers <freebsd-hackers@freebsd.org> Subject: Re: Attribute alloc__size use and clang 5.0.1 vs. gcc7 (e.g.): __builtin_object_size(p,1) and __builtin_object_size(p,3) disagreements result Message-ID: <67a7b385-b554-1a5e-ef30-fb29a0c6cf08@FreeBSD.org> In-Reply-To: <9DE674C6-EAA3-4E8A-906F-446E74D82FC4@yahoo.com> References: <1AA0993D-81E4-4DC0-BBD9-CC42B26ADB1C@yahoo.com> <F227842D-6BE2-4680-82E7-07906AF61CD7@yahoo.com> <8c4c5985-885a-abf7-93df-0698c645d36e@FreeBSD.org> <9DE674C6-EAA3-4E8A-906F-446E74D82FC4@yahoo.com>
next in thread | previous in thread | raw e-mail | index | archive | help
This is a multi-part message in MIME format.
--------------EFD43819BA4DE658890B95F7
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Hi;
On 01/21/18 11:56, Mark Millard wrote:
> [May be an __alloc_size2(n,s) should be added and used?]
>
> On 2018-Jan-20, at 5:05 PM, Pedro Giffuni <pfg@FreeBSD.org> wrote:
>
>> Very interesting , thanks for running such tests ...
>>
>>
>> On 01/20/18 18:59, Mark Millard wrote:
>>> [Noting a typo in the program source, and
>>> so in the output text: the 2nd occurance of: "my_calloc_alt0
>>> should have been: "my_calloc_alt1
>>> . Hand edited corrections below for clarity.]
>>>
>>> On 2018-Jan-20, at 3:27 PM, Mark Millard <marklmi26-fbsd@yahoo.com> wrote:
>>>
>>>> [Bugzilla 225197 indirectly lead to this.
>>>> Avoiding continuing there.]
>>>>
>>>> I decided to compare some alternate uses of
>>>> __attribute__((alloc_size(. . .))) compiled
>>>> and run under clang 5.0.1 and gcc7. I did not
>>>> get what I expected based on prior discussion
>>>> material.
>>>>
>>>> This is an FYI since I do not know how important
>>>> the distinctions that I found are.
>>>>
>>>> Here is the quick program:
>>>>
>>>> # more alloc_size_attr_test.c
>>>> #include <stdlib.h>
>>>> #include <stdio.h>
>>>>
>>>> __attribute__((alloc_size(1,2)))
>>>> void* my_calloc_alt0(size_t n, size_t s)
>>>> {
>>>> void* p = calloc(n,s);
>>>> printf("calloc __builtin_object_size 0,1,2,3: %ld, %ld, %ld, %ld\n"
>>>> ,(long) __builtin_object_size(p, 0)
>>>> ,(long) __builtin_object_size(p, 1)
>>>> ,(long) __builtin_object_size(p, 2)
>>>> ,(long) __builtin_object_size(p, 3)
>>>> );
>>>> return p;
>>>> }
>>>>
>>>> __attribute__((alloc_size(1))) __attribute__((alloc_size(2)))
>>>> void* my_calloc_alt1(size_t n, size_t s)
>>>> {
>>>> void* p = calloc(n,s);
>>>> printf("calloc __builtin_object_size 0,1,2,3: %ld, %ld, %ld, %ld\n"
>>>> ,(long) __builtin_object_size(p, 0)
>>>> ,(long) __builtin_object_size(p, 1)
>>>> ,(long) __builtin_object_size(p, 2)
>>>> ,(long) __builtin_object_size(p, 3)
>>>> );
>>>> return p;
>>>> }
>>>>
>>>> int main()
>>>> {
>>>> void* p = my_calloc_alt0(2,7);
>>>> printf("my_calloc_alt0 __builtin_object_size 0,1,2,3: %ld, %ld, %ld, %ld\n"
>>>> ,(long) __builtin_object_size(p, 0)
>>>> ,(long) __builtin_object_size(p, 1)
>>>> ,(long) __builtin_object_size(p, 2)
>>>> ,(long) __builtin_object_size(p, 3)
>>>> );
>>>> void* q = my_calloc_alt1(2,7);
>>>> printf("my_calloc_alt0 __builtin_object_size 0,1,2,3: %ld, %ld, %ld, %ld\n"
>>> The above line should have been:
>>>
>>> printf("my_calloc_alt1 __builtin_object_size 0,1,2,3: %ld, %ld, %ld, %ld\n"
>>>
>>>> ,(long) __builtin_object_size(q, 0)
>>>> ,(long) __builtin_object_size(q, 1)
>>>> ,(long) __builtin_object_size(q, 2)
>>>> ,(long) __builtin_object_size(q, 3)
>>>> );
>>>> }
>>>>
>>>> # uname -apKU
>>>> FreeBSD FBSDFSSD 12.0-CURRENT FreeBSD 12.0-CURRENT r327485M amd64 amd64 1200054 1200054
>>>>
>>>> The system-clang 5.0.1 result was:
>>>>
>>>> # clang -O2 alloc_size_attr_test.c
>>> The later outputs are edited for clarity:
>>>
>>>> # ./a.out
>>>> calloc __builtin_object_size 0,1,2,3: 14, 14, 14, 0
>>>> my_calloc_alt0 __builtin_object_size 0,1,2,3: 14, 14, 14, 0
>>>> calloc __builtin_object_size 0,1,2,3: 14, 14, 14, 0
>>> my_calloc_alt1 __builtin_object_size 0,1,2,3: 14, 14, 14, 0
>>>> The lang/gcc7 result was:
>>>>
>>>> # gcc7 -O2 alloc_size_attr_test.c
>>>>
>>>> # ./a.out
>>>> calloc __builtin_object_size 0,1,2,3: -1, -1, 0, 0
>>>> my_calloc_alt0 __builtin_object_size 0,1,2,3: 14, 14, 14, 14
>>>> calloc __builtin_object_size 0,1,2,3: -1, -1, 0, 0
>>> my_calloc_alt1 __builtin_object_size 0,1,2,3: 14, 7, 14, 14
>>>> I'll ignore that gcc does not provide actual sizes
>>>> via __builtin_object_size for calloc use.
>>>>
>>>> Pairing the other lines for easy comparison, with
>>>> some notes mixed in:
>>>>
>>>> __attribute__((alloc_size(1,2))) style:
>>>> my_calloc_alt0 __builtin_object_size 0,1,2,3: 14, 14, 14, 0 (system clang)
>>>> my_calloc_alt0 __builtin_object_size 0,1,2,3: 14, 14, 14, 14 (gcc7)
>>>>
>>>> __attribute__((alloc_size(1))) __attribute__((alloc_size(2))) style:
>>> my_calloc_alt1 __builtin_object_size 0,1,2,3: 14, 14, 14, 0 (system clang)
>>> my_calloc_alt1 __builtin_object_size 0,1,2,3: 14, 7, 14, 14 (gcc7)
>> So on GCC7 it appears
>> __attribute__((alloc_size(1,2))) != __attribute__((alloc_size(1))) __attribute__((alloc_size(2)))
>>
>> This is not good as it was the base for r280801 .. related to the old discussion about deprecating old compilers that don't accept VA_ARGS.
>>
>> I am unsure if its a regression but it appears that for clang it is the same thing though.
> May be there should be a __alloc_size2(n,s) that translates to
> __attribute__((alloc_size(n,s))) when it is not a no-op. This
> avoids the VA_ARGS issue and gcc's documentation makes no mention
> of a more than 2 argument variant of
> __attribute__((alloc_size(. . .))) .
>
> Looking up glib it has:
>
> G_GNUC_ALLOC_SIZE(x)
> G_GNUC_ALLOC_SIZE2(x,y)
>
> but none for any other count of arguments.
Yes, that would work fine for alloc_array. It would work for the nonnull
attribute but that is deprecated on FreeBSD :).
Concept patch attached.
Pedro.
>>>> Thus. . .
>>>>
>>>> For __attribute__((alloc_size(1))) __attribute__((alloc_size(2))):
>>>> __builtin_object_size(p,1) is not equivalent (clang vs. gcc7)
>>>>
>>>> For both of the alloc_size usage styles:
>>>> __builtin_object_size(p,3) is not equivalent (clang vs. gcc7)
>>>>
>>>> This means that the two style of alloc_size use are not
>>>> equivalent across some major compilers/toolchains.
>> This is actually not a surprise: GCC and clang implementation of __alloc_size__ has differences due to limitations on the LLVM IR (or the fact there is one).
>>
>> The alloc_size attribute is basically only used for the so-called FORTIFY_SOURCE feature that depends on GCC with some support from the C-library: last time I looked clang didn't support the compile-time checks very well. The attributes are mostly unused in FreeBSD at this time but, GCC7 -Walloc-size-larger-than=size depends on them (I have never tested that though).
>>
>> FWIW, we had an unfinished GSoC that attempted to implement FORTIFY_SOURCE but we got stuck on the lack of clang support and other issues. Lately Google has been spending some effort on it but it is more limited and doesn't match the GCC behavior.
>>
>>
>>
>>
>>>> But I do not know if either of the differences is a problem or
>>>> not.
>>>>
>>>>
>>>> Note: without a sufficient -O<?> all the figures can be
>>>> the mix of -1's and 0's.
> ===
> Mark Millard
> marklmi at yahoo.com
> ( markmi at dsl-only.net is
> going away in 2018-Feb, late)
>
--------------EFD43819BA4DE658890B95F7
Content-Type: text/x-patch;
name="alloc_size2.diff"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename="alloc_size2.diff"
Index: include/stdlib.h
===================================================================
--- include/stdlib.h (revision 328218)
+++ include/stdlib.h (working copy)
@@ -92,7 +92,7 @@
void *bsearch(const void *, const void *, size_t,
size_t, int (*)(const void * _Nonnull, const void *));
void *calloc(size_t, size_t) __malloc_like __result_use_check
- __alloc_size(1) __alloc_size(2);
+ __alloc_size2(1, 2);
div_t div(int, int) __pure2;
_Noreturn void exit(int);
void free(void *);
@@ -302,8 +302,8 @@
int (*)(void *, const void *, const void *));
int radixsort(const unsigned char **, int, const unsigned char *,
unsigned);
-void *reallocarray(void *, size_t, size_t) __result_use_check __alloc_size(2)
- __alloc_size(3);
+void *reallocarray(void *, size_t, size_t) __result_use_check
+ __alloc_size2(2, 3);
void *reallocf(void *, size_t) __result_use_check __alloc_size(2);
int rpmatch(const char *);
void setprogname(const char *);
Index: sys/sys/cdefs.h
===================================================================
--- sys/sys/cdefs.h (revision 328218)
+++ sys/sys/cdefs.h (working copy)
@@ -230,8 +230,10 @@
#endif
#if __GNUC_PREREQ__(4, 3) || __has_attribute(__alloc_size__)
#define __alloc_size(x) __attribute__((__alloc_size__(x)))
+#define __alloc_size(n, x) __attribute__((__alloc_size__(n, x)))
#else
#define __alloc_size(x)
+#define __alloc_size2(n, x)
#endif
#if __GNUC_PREREQ__(4, 9) || __has_attribute(__alloc_align__)
#define __alloc_align(x) __attribute__((__alloc_align__(x)))
Index: sys/sys/malloc.h
===================================================================
--- sys/sys/malloc.h (revision 328218)
+++ sys/sys/malloc.h (working copy)
@@ -188,7 +188,7 @@
__malloc_like __result_use_check __alloc_size(1);
void *mallocarray(size_t nmemb, size_t size, struct malloc_type *type,
int flags) __malloc_like __result_use_check
- __alloc_size(1) __alloc_size(2);
+ __alloc_size2(1, 2);
void malloc_init(void *);
int malloc_last_fail(void);
void malloc_type_allocated(struct malloc_type *type, unsigned long size);
--------------EFD43819BA4DE658890B95F7--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?67a7b385-b554-1a5e-ef30-fb29a0c6cf08>