Message ID | 20140801124904.GG7393@tucnak.redhat.com |
---|---|
State | New |
Headers | show |
On Fri, Aug 1, 2014 at 3:49 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Thu, Jul 31, 2014 at 11:32:12PM +0300, Janne Blomqvist wrote: >> a while ago I committed a patch to trunk adding a function >> xmallocarray to libgfortran, which is a malloc wrapper like xmalloc >> but has two arguments and does an overflow check before multiplying >> them together. > > That seems to be unnecessarily expensive for the common case where both > nmemb and size are small. > > calloc in glibc uses something like following to avoid the division most of > the time, if both nmemb and size are small enough then nmemb * size can't > overflow. At least for 64-bit architectures small is smaller than 4GB > for both numbers. > > 2014-08-01 Jakub Jelinek <jakub@redhat.com> > > * runtime/memory.c (xmallocarray): Avoid division for the common case. > > --- libgfortran/runtime/memory.c.jj 2014-06-18 08:50:33.000000000 +0200 > +++ libgfortran/runtime/memory.c 2014-08-01 14:41:08.385856116 +0200 > @@ -56,7 +56,9 @@ xmallocarray (size_t nmemb, size_t size) > > if (!nmemb || !size) > size = nmemb = 1; > - else if (nmemb > SIZE_MAX / size) > +#define HALF_SIZE_T (((size_t) 1) << (__CHAR_BIT__ * sizeof (size_t) / 2)) > + else if (__builtin_expect ((nmemb | size) >= HALF_SIZE_T, 0) > + && nmemb > SIZE_MAX / size) > { > errno = ENOMEM; > os_error ("Integer overflow in xmallocarray"); Nice, though as os_error() has the _Noreturn specifier the __builtin_expect() is not necessary, right? In libgfortran.h we have in the comment block above the likely/unlikely macros, which are wrappers around __builtin_expect: "...as __builtin_expect overrides the compiler heuristic, do not use in conditions where one of the branches ends with a call to a function with __attribute__((noreturn)): the compiler internal heuristic will mark this branch as much less likely as unlikely() would do." Ok for trunk/4.9/4.8. If you choose to leave the __builtin_expect there, please explain why?
On Sat, Aug 02, 2014 at 12:09:24AM +0300, Janne Blomqvist wrote: > > --- libgfortran/runtime/memory.c.jj 2014-06-18 08:50:33.000000000 +0200 > > +++ libgfortran/runtime/memory.c 2014-08-01 14:41:08.385856116 +0200 > > @@ -56,7 +56,9 @@ xmallocarray (size_t nmemb, size_t size) > > > > if (!nmemb || !size) > > size = nmemb = 1; > > - else if (nmemb > SIZE_MAX / size) > > +#define HALF_SIZE_T (((size_t) 1) << (__CHAR_BIT__ * sizeof (size_t) / 2)) > > + else if (__builtin_expect ((nmemb | size) >= HALF_SIZE_T, 0) > > + && nmemb > SIZE_MAX / size) > > { > > errno = ENOMEM; > > os_error ("Integer overflow in xmallocarray"); > > Nice, though as os_error() has the _Noreturn specifier the > __builtin_expect() is not necessary, right? In libgfortran.h we have The reason for __builtin_expect here was to make already the nmemb > SIZE_MAX / size computation as unlikely, the noreturn predictor will of course DTRT with the {} block. Jakub
Jakub Jelinek wrote: > On Sat, Aug 02, 2014 at 12:09:24AM +0300, Janne Blomqvist wrote: >>> --- libgfortran/runtime/memory.c.jj 2014-06-18 08:50:33.000000000 +0200 >>> +++ libgfortran/runtime/memory.c 2014-08-01 14:41:08.385856116 +0200 >>> @@ -56,7 +56,9 @@ xmallocarray (size_t nmemb, size_t size) >>> >>> if (!nmemb || !size) >>> size = nmemb = 1; >>> - else if (nmemb > SIZE_MAX / size) >>> +#define HALF_SIZE_T (((size_t) 1) << (__CHAR_BIT__ * sizeof (size_t) / 2)) >>> + else if (__builtin_expect ((nmemb | size) >= HALF_SIZE_T, 0) >>> + && nmemb > SIZE_MAX / size) >>> { >>> errno = ENOMEM; >>> os_error ("Integer overflow in xmallocarray"); >> Nice, though as os_error() has the _Noreturn specifier the >> __builtin_expect() is not necessary, right? In libgfortran.h we have > The reason for __builtin_expect here was to make already the > nmemb > SIZE_MAX / size > computation as unlikely, the noreturn predictor will of course DTRT with the > {} block. But there is a difference in probability between __builtin_expect and "noreturn". __builtin_expect had until two years ago a probability of 99%, now it has a probability of only 90% (which is tunable with a -param)* – while "noreturn" has a higher probability. Thus, at least if you had used else if (unlikely(... & ...)) os_error you would have made the basic block with os_error more likely than without the "unlikely" (alias __builtin_expect). However, I don't know what happens with using "unlikely(cond1) && cond2". Tobias * Or internally in the compiler only, by passing a third argument.
--- libgfortran/runtime/memory.c.jj 2014-06-18 08:50:33.000000000 +0200 +++ libgfortran/runtime/memory.c 2014-08-01 14:41:08.385856116 +0200 @@ -56,7 +56,9 @@ xmallocarray (size_t nmemb, size_t size) if (!nmemb || !size) size = nmemb = 1; - else if (nmemb > SIZE_MAX / size) +#define HALF_SIZE_T (((size_t) 1) << (__CHAR_BIT__ * sizeof (size_t) / 2)) + else if (__builtin_expect ((nmemb | size) >= HALF_SIZE_T, 0) + && nmemb > SIZE_MAX / size) { errno = ENOMEM; os_error ("Integer overflow in xmallocarray");