libio: Add small optimization on fmemopen

  This patch uses C99 variable-lenght array on internal fmemopen cookie
allocation to avoid to use two mallocs for the case if an internal
buffer must be allocated.  No functional changes are done.

Tested on x86_64 and i686.

	* libio/fmemopen.c (fmemopen_cookie_t): Remove mybuffer and add
	variable-length array member.
	(fmemopen_close): Do no free the internal buffer.
	(__fmemopen): Use C99 variable-length array to allocate both the
	fmemopen cookie and internal buffer (for null arguments).
---
 libio/fmemopen.c | 40 ++++++++++++++++------------------------
 2 files changed, 24 insertions(+), 24 deletions(-)
  

On 09/21/2016 04:05 PM, Adhemerval Zanella wrote:
> This patch uses C99 variable-lenght array on internal fmemopen cookie
> allocation to avoid to use two mallocs for the case if an internal
> buffer must be allocated.  No functional changes are done.
>
> Tested on x86_64 and i686.
>
> 	* libio/fmemopen.c (fmemopen_cookie_t): Remove mybuffer and add
> 	variable-length array member.
> 	(fmemopen_close): Do no free the internal buffer.
> 	(__fmemopen): Use C99 variable-length array to allocate both the
> 	fmemopen cookie and internal buffer (for null arguments).

It's a flexible array member, not a variable-length array.  Looks good 
to me.

What about libio/oldfmemopen.c?  Do you want to touch this as well?

Thanks,
Florian
  

On 09/21/2016 07:05 AM, Adhemerval Zanella wrote:
> +      clen += len;

That might cause problems , as the C standard says you can't access a 
flexible array member past the last byte that is properly aligned for 
the containing structure, and GCC relies on this when generating code 
(see GCC bug 66661). One way to work around the problem is to change the 
earlier:

+  size_t clen = sizeof (fmemopen_cookie_t);

to:

+  size_t clen = sizeof (fmemopen_cookie_t) + _Alignof 
(fmemopen_cookie_t) - 1;

This may overallocate a bit, but that's OK here.
  

On 21/09/2016 14:22, Paul Eggert wrote:
> On 09/21/2016 07:05 AM, Adhemerval Zanella wrote:
>> +      clen += len;
> 
> That might cause problems , as the C standard says you can't access a flexible array member past the last byte that is properly aligned for the containing structure, and GCC relies on this when generating code (see GCC bug 66661). One way to work around the problem is to change the earlier:
> 
> +  size_t clen = sizeof (fmemopen_cookie_t);
> 
> to:
> 
> +  size_t clen = sizeof (fmemopen_cookie_t) + _Alignof (fmemopen_cookie_t) - 1;
> 
> This may overallocate a bit, but that's OK here.
> 

Thanks for pointing this out, I think this is the safe approach regarding
Florian comment #10 and #11.  I will change it.
  

On 21/09/2016 11:57, Florian Weimer wrote:
> On 09/21/2016 04:05 PM, Adhemerval Zanella wrote:
>> This patch uses C99 variable-lenght array on internal fmemopen cookie
>> allocation to avoid to use two mallocs for the case if an internal
>> buffer must be allocated.  No functional changes are done.
>>
>> Tested on x86_64 and i686.
>>
>>     * libio/fmemopen.c (fmemopen_cookie_t): Remove mybuffer and add
>>     variable-length array member.
>>     (fmemopen_close): Do no free the internal buffer.
>>     (__fmemopen): Use C99 variable-length array to allocate both the
>>     fmemopen cookie and internal buffer (for null arguments).
> 
> It's a flexible array member, not a variable-length array.  Looks good to me.

Right, I will correct naming in commit message.

> 
> What about libio/oldfmemopen.c?  Do you want to touch this as well?

I won't bother with it, testing would be tricky and I do not see a
compelling reason to optimize compat code.
  

On 09/21/2016 08:17 PM, Adhemerval Zanella wrote:
>
>
> On 21/09/2016 14:22, Paul Eggert wrote:
>> On 09/21/2016 07:05 AM, Adhemerval Zanella wrote:
>>> +      clen += len;
>>
>> That might cause problems , as the C standard says you can't access a flexible array member past the last byte that is properly aligned for the containing structure, and GCC relies on this when generating code (see GCC bug 66661). One way to work around the problem is to change the earlier:
>>
>> +  size_t clen = sizeof (fmemopen_cookie_t);
>>
>> to:
>>
>> +  size_t clen = sizeof (fmemopen_cookie_t) + _Alignof (fmemopen_cookie_t) - 1;
>>
>> This may overallocate a bit, but that's OK here.
>>
>
> Thanks for pointing this out, I think this is the safe approach regarding
> Florian comment #10 and #11.  I will change it.

Please don't, it's a defect in the standard (and GCC"s Address Sanitizer 
has a bug as well).

Florian
  

On 21/09/2016 15:23, Florian Weimer wrote:
> On 09/21/2016 08:17 PM, Adhemerval Zanella wrote:
>>
>>
>> On 21/09/2016 14:22, Paul Eggert wrote:
>>> On 09/21/2016 07:05 AM, Adhemerval Zanella wrote:
>>>> +      clen += len;
>>>
>>> That might cause problems , as the C standard says you can't access a flexible array member past the last byte that is properly aligned for the containing structure, and GCC relies on this when generating code (see GCC bug 66661). One way to work around the problem is to change the earlier:
>>>
>>> +  size_t clen = sizeof (fmemopen_cookie_t);
>>>
>>> to:
>>>
>>> +  size_t clen = sizeof (fmemopen_cookie_t) + _Alignof (fmemopen_cookie_t) - 1;
>>>
>>> This may overallocate a bit, but that's OK here.
>>>
>>
>> Thanks for pointing this out, I think this is the safe approach regarding
>> Florian comment #10 and #11.  I will change it.
> 
> Please don't, it's a defect in the standard (and GCC"s Address Sanitizer has a bug as well).

I do not have a strong opinion here, my idea to use this workaround is 
to avoid possible issues with faulty compilers.  However, if the
consensus is to not push this forward, I can use my original patch.
  

On 09/21/2016 11:26 AM, Adhemerval Zanella wrote:
> my idea to use this workaround is
> to avoid possible issues with faulty compilers.

That's my thought as well. Although we can argue that the C standard, 
GCC, valgrind, etc. are all faulty, possibly the C standard etc. won't 
change (after all, there is a performance advantage to GCC's current 
behavior) and our code would continue to be over the edge.

We could document this more clearly by having a macro 
FLEXMEMBER_NEEDS_ALIGNMENT specifying whether flexible array member 
sizes require alignment. The default could be true to be safe for 
current platforms, and the macro could be false on (future?) platforms 
known to not require aligned allocation of flexible array members. Then 
we could say something like this:

   size_t clen = sizeof (fmemopen_cookie_t);
   if (FLEXMEMBER_NEEDS_ALIGNMENT)
     clen += _Alignof (fmemopen_cookie_t) - 1;
  

On 09/21/2016 08:43 PM, Paul Eggert wrote:
> On 09/21/2016 11:26 AM, Adhemerval Zanella wrote:
>> my idea to use this workaround is
>> to avoid possible issues with faulty compilers.
>
> That's my thought as well. Although we can argue that the C standard,
> GCC, valgrind, etc. are all faulty, possibly the C standard etc. won't
> change (after all, there is a performance advantage to GCC's current
> behavior) and our code would continue to be over the edge.

We don't know the nature of the GCC issue, so we cannot work around it 
reliably.  The most likely explanation is that Address Sanitizer does 
not account for a valid GCC optimization.

Florian
  

On 09/21/2016 11:47 AM, Florian Weimer wrote:
> We don't know the nature of the GCC issue, so we cannot work around it 
> reliably.  The most likely explanation is that Address Sanitizer does 
> not account for a valid GCC optimization.

That's not the sense that I get from looking at Bug 66661. GCC is 
assuming it can do an aligned word load of the trailing bytes of a 
flexible array member in a properly-aligned structure. This assumption 
is something we can easily work around reliably, e.g., with the patch I 
suggested. Although the followups to Bug 66661 suggest that there may be 
further problems with overaligned structures, the code here is not using 
_Alignas so these further problems are not an issue here.
  

On 09/21/2016 08:57 PM, Paul Eggert wrote:
> On 09/21/2016 11:47 AM, Florian Weimer wrote:
>> We don't know the nature of the GCC issue, so we cannot work around it
>> reliably.  The most likely explanation is that Address Sanitizer does
>> not account for a valid GCC optimization.
>
> That's not the sense that I get from looking at Bug 66661. GCC is
> assuming it can do an aligned word load of the trailing bytes of a
> flexible array member in a properly-aligned structure.

But that assumption is completely correct!  It's just that Address 
Sanitizer does not account for it (based on what I've seen so far).

>Although the followups to Bug 66661 suggest that there may be
> further problems with overaligned structures, the code here is not using
> _Alignas so these further problems are not an issue here.

_Alignas is used only to demonstrate the issue more clearly.

glibc currently does not compile with Address Sanitizer anyway, so until 
that is fixed (or someone has a better explanation of the root cause of 
GCC PR66661), I would suggest to assume that this issue does not matter 
to glibc at all.

Thanks,
Florian
  

On 21/09/2016 16:27, Florian Weimer wrote:
> On 09/21/2016 08:57 PM, Paul Eggert wrote:
>> On 09/21/2016 11:47 AM, Florian Weimer wrote:
>>> We don't know the nature of the GCC issue, so we cannot work around it
>>> reliably.  The most likely explanation is that Address Sanitizer does
>>> not account for a valid GCC optimization.
>>
>> That's not the sense that I get from looking at Bug 66661. GCC is
>> assuming it can do an aligned word load of the trailing bytes of a
>> flexible array member in a properly-aligned structure.
> 
> But that assumption is completely correct!  It's just that Address Sanitizer does not account for it (based on what I've seen so far).
> 
>> Although the followups to Bug 66661 suggest that there may be
>> further problems with overaligned structures, the code here is not using
>> _Alignas so these further problems are not an issue here.
> 
> _Alignas is used only to demonstrate the issue more clearly.
> 
> glibc currently does not compile with Address Sanitizer anyway, so until that is fixed (or someone has a better explanation of the root cause of GCC PR66661), I would suggest to assume that this issue does not matter to glibc at all.

I check with a different memory profiler (valgrind) and at least with gcc 5.4 
fmemopen with this change shows no wrong memory access.  It also shows that
the issue might be indeed in ASAN.

I think we can push this optimization as if and I will continue track BZ#66661
to check for further development if the patch would require more changes.
  

On 09/21/2016 12:27 PM, Florian Weimer wrote:
> _Alignas is used only to demonstrate the issue more clearly.
>

That's not clear to me. Yes, _Alignas does make the issue pop out more. 
However, the problem with the proposed fmemopen code has to do with 
allocating flexible arrays via malloc (a fairly common practice), 
whereas the problem illustrated by GCC bug 66661 comment 12 has to do 
with static allocation of flexible array members (a rarely used feature).

As I understand it, comment 12 argues that GCC should not be able to 
assume that an object's size is a multiple of its alignment. That's a 
steep uphill climb, I'm afraid, as GCC is riddled with that assumption, 
it would be painful to remove it, and the resulting benefit would be so 
trivial that I'm afraid it would not be worth the effort. If there's a 
bug in the abovementioned rarely used feature, surely it can be fixed 
without having to alter GCC's fundamental assumption that size is a 
multiple of alignment.

> glibc currently does not compile with Address Sanitizer anyway

It's not just Address Sanitizer, right? valgrind has a similar issue.
  

On Wed, Sep 21, 2016 at 11:43:23AM -0700, Paul Eggert wrote:
> On 09/21/2016 11:26 AM, Adhemerval Zanella wrote:
> >my idea to use this workaround is
> >to avoid possible issues with faulty compilers.
> 
> That's my thought as well. Although we can argue that the C
> standard, GCC, valgrind, etc. are all faulty, possibly the C
> standard etc. won't change (after all, there is a performance
> advantage to GCC's current behavior) and our code would continue to
> be over the edge.
> 
> We could document this more clearly by having a macro
> FLEXMEMBER_NEEDS_ALIGNMENT specifying whether flexible array member
> sizes require alignment. The default could be true to be safe for
> current platforms, and the macro could be false on (future?)
> platforms known to not require aligned allocation of flexible array
> members. Then we could say something like this:
> 
>   size_t clen = sizeof (fmemopen_cookie_t);
>   if (FLEXMEMBER_NEEDS_ALIGNMENT)
>     clen += _Alignof (fmemopen_cookie_t) - 1;

Couldn't you just use __attribute__((aligned(16))) or is that also
buggy?
  

On 09/23/2016 07:17 AM, Ondřej Bílka wrote:
>>   size_t clen = sizeof (fmemopen_cookie_t);
>>   if (FLEXMEMBER_NEEDS_ALIGNMENT)
>>     clen += _Alignof (fmemopen_cookie_t) - 1;
>
> Couldn't you just use __attribute__((aligned(16))) or is that also
> buggy?

Buggy in what sense?

malloc isn't guaranteed to provide 16-byte alignment on all glibc 
architectures.

Florian
  

On Fri, Sep 23, 2016 at 07:34:24AM +0200, Florian Weimer wrote:
> On 09/23/2016 07:17 AM, Ondřej Bílka wrote:
> >>  size_t clen = sizeof (fmemopen_cookie_t);
> >>  if (FLEXMEMBER_NEEDS_ALIGNMENT)
> >>    clen += _Alignof (fmemopen_cookie_t) - 1;
> >
> >Couldn't you just use __attribute__((aligned(16))) or is that also
> >buggy?
> 
> Buggy in what sense?
> 
> malloc isn't guaranteed to provide 16-byte alignment on all glibc
> architectures.
> 
Buggy in sense that some gcc version would ignore it. I picked 16 to
have same alignment as malloc does as 16 is max granularity of malloc.
  

On 09/22/2016 10:17 PM, Ondřej Bílka wrote:
> Couldn't you just use __attribute__((aligned(16))) or is that also
> buggy?

I don't see how that would help. It would make the structure more 
aligned than it already is, giving GCC even more license to use "unsafe" 
optimizations.

Or were you thinking of "__attribute__ ((packed))"? That might do it 
(someone would have to investigate). My guess is that it would cause 
other problems, though.
  

* Ondřej Bílka:

> On Fri, Sep 23, 2016 at 07:34:24AM +0200, Florian Weimer wrote:
>> On 09/23/2016 07:17 AM, Ondřej Bílka wrote:
>> >>  size_t clen = sizeof (fmemopen_cookie_t);
>> >>  if (FLEXMEMBER_NEEDS_ALIGNMENT)
>> >>    clen += _Alignof (fmemopen_cookie_t) - 1;
>> >
>> >Couldn't you just use __attribute__((aligned(16))) or is that also
>> >buggy?
>> 
>> Buggy in what sense?
>> 
>> malloc isn't guaranteed to provide 16-byte alignment on all glibc
>> architectures.
>> 
> Buggy in sense that some gcc version would ignore it. I picked 16 to
> have same alignment as malloc does as 16 is max granularity of malloc.

malloc granularity can be as low as 2 (on m68k) without impacting C
conformance, and an interposed malloc implementation might provide
just that.

On most 32-bit platforms, glibc's own malloc provides only 8-byte
alignment.