[PATCH 06/25] Add struct scratch_buffer and its internal helper functions

[Florian Weimer]

On 04/03/2015 03:12 AM, Paul Eggert wrote:
> On 04/02/2015 11:30 AM, Florian Weimer wrote:
>> +static inline void
>> +scratch_buffer_init (struct scratch_buffer *buffer)
>> +  __attribute__ ((always_inline));
>> +static inline void
> 
> This should be just "static __always_inline void"; there should be no
> need to have both decl and defn, and __always_inline arranges for both
> 'inline' and the attribute.

Hmm, I was confused by the GCC documentation.  Apparently, the prefixed
attributes aren't deprecated after all.

> But then again, why bother to declare this function to be inline? Isn't
> the compiler smart enough to inline when needed?  I assume that this
> include file is not visible to the user, so is there some reason to
> require the function to be __always_inline, or even __inline?

“static inline” functions don't result in global symbols when
out-of-line, so the inlining isn't need for correctness, I assume.

My hope is that __always_inline triggers early inlining, so that the
callers benefit from the branch hint in those functions.
(Compiler-driven inlining is influenced by branch hints, so early
inlining is beneficial.)

But I applied the attribute to the wrong functions; fixed.

>> +  /* Discard old buffer.  */
>> +  scratch_buffer_free (buffer);
>> +
>> +  /* Check for overflow.  */
>> +  if (__glibc_unlikely (new_length < buffer->length))
>> +    {
>> +      /* Buffer must remain valid to free.  */
>> +      scratch_buffer_init (buffer);
>> +      __set_errno (ENOMEM);
>> +      return false;
>> +    }
>> +
>> +  void *new_ptr = malloc (new_length);
>> +  if (new_ptr == NULL)
>> +    {
>> +      /* Buffer must remain valid to free.  */
>> +      scratch_buffer_init (buffer);
>> +      return false;
>> +    }
> 
> The two ifs can be combined so that there's only copy of the cleanup
> code.

Thanks, I tried to simplify the code a bit based on the suggestion.  I
didn't want to overwrite errno with ENOMEM in the malloc failure case,
though.

> Also, if all that's needed is that the buffer remains valid to
> free, why not let the caller's free (which we need anyway) do the
> cleanup rather than doing the cleanup redundantly ourselves? Instead of
> the above 19 lines, we could have just 6 lines:
> 
>  void *new_ptr = new_length < buffer->length ? NULL : malloc (new_length);
>  if (__glibc_unlikely (! new_ptr))
>    {
>       __set_errno (ENOMEM);
>       return false;
>    }

With the current version, several callers do not need cleanup on the
error path.  That's why I think it's worth moving cleanup-on-error into
the buffer growth functions.

> Sorry, I still don't get why the ABI matters here, since the functions
> are all private and presumably are all linked into glibc already.

My concern was the internal ABI between libc and the NSS modules.  I
have remove those comments because no one else seems to be bothered by that.

	* include/scratch_buffer.h: New file.
	* malloc/scratch_buffer_grow.c: Likewise.
	* malloc/scratch_buffer_grow_preserve.c: Likewise.
	* malloc/scratch_buffer_set_array_size.c: Likewise.
	* malloc/tst-scratch_buffer.c: Likewise.
	* malloc/Makefile (routines): Add scratch_buffer_grow.
	(tests): Add test case.
	* malloc/Versions (GLIBC_PRIVATE): Export
	__libc_scratch_buffer_grow, __libc_scratch_buffer_grow_preserve,
	__libc_scratch_buffer_set_array_size.

-- 
Florian Weimer / Red Hat Product Security
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-struct-scratch_buffer-and-its-internal-helper-fu.patch
Type: text/x-patch
Size: 18369 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20150406/186b4819/attachment.bin>