Message ID | 1474466758-26544-1-git-send-email-adhemerval.zanella@linaro.org |
---|---|
State | New |
Headers | show |
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.
diff --git a/libio/fmemopen.c b/libio/fmemopen.c index 0f65590..d7df1be 100644 --- a/libio/fmemopen.c +++ b/libio/fmemopen.c @@ -35,11 +35,11 @@ typedef struct fmemopen_cookie_struct fmemopen_cookie_t; struct fmemopen_cookie_struct { char *buffer; /* memory buffer. */ - int mybuffer; /* allocated my buffer? */ int append; /* buffer open for append? */ size_t size; /* buffer length in bytes. */ _IO_off64_t pos; /* current position at the buffer. */ size_t maxpos; /* max position in buffer. */ + char data[]; }; @@ -136,11 +136,7 @@ fmemopen_seek (void *cookie, _IO_off64_t *p, int w) static int fmemopen_close (void *cookie) { - fmemopen_cookie_t *c = (fmemopen_cookie_t *) cookie; - - if (c->mybuffer) - free (c->buffer); - free (c); + free (cookie); return 0; } @@ -153,23 +149,24 @@ __fmemopen (void *buf, size_t len, const char *mode) fmemopen_cookie_t *c; FILE *result; - c = (fmemopen_cookie_t *) calloc (sizeof (fmemopen_cookie_t), 1); - if (c == NULL) - return NULL; - - c->mybuffer = (buf == NULL); - - if (c->mybuffer) + size_t clen = sizeof (fmemopen_cookie_t); + if (buf == NULL) { - c->buffer = (char *) malloc (len); - if (c->buffer == NULL) + if (__glibc_unlikely (len >= (SIZE_MAX - clen))) { - free (c); + __set_errno (ENOMEM); return NULL; } - c->buffer[0] = '\0'; + clen += len; } - else + + c = (fmemopen_cookie_t *) calloc (clen, 1); + if (c == NULL) + return NULL; + + c->buffer = c->data; + + if (buf != NULL) { if (__glibc_unlikely ((uintptr_t) len > -(uintptr_t) buf)) { @@ -214,12 +211,7 @@ __fmemopen (void *buf, size_t len, const char *mode) result = _IO_fopencookie (c, mode, iof); if (__glibc_unlikely (result == NULL)) - { - if (c->mybuffer) - free (c->buffer); - - free (c); - } + free (c); return result; }