From patchwork Thu Sep 21 22:53:42 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Paul Eggert X-Patchwork-Id: 23070 Received: (qmail 10268 invoked by alias); 21 Sep 2017 22:53:48 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 10258 invoked by uid 89); 21 Sep 2017 22:53:48 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.4 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RP_MATCHES_RCVD, SPF_PASS autolearn=ham version=3.3.2 spammy=abovementioned, credit X-HELO: zimbra.cs.ucla.edu Subject: Re: [PATCH 2/2] Use C11 _Alignas on scratch_buffer internal buffer To: Adhemerval Zanella , Florian Weimer , Andreas Schwab Cc: libc-alpha@sourceware.org, Gnulib bugs References: <1505745774-29303-1-git-send-email-adhemerval.zanella@linaro.org> <1505745774-29303-2-git-send-email-adhemerval.zanella@linaro.org> <0f162389-e84d-3168-35eb-f168484e87ee@cs.ucla.edu> <22a21507-adc7-3116-813e-0ee873e2a8f9@cs.ucla.edu> <30c017fe-fafe-d787-7d76-865223cef599@linaro.org> <0e2a0dfe-5a44-f365-9544-ee0fc2b0df63@linaro.org> <4f5ad3af-0791-a917-e2dc-719e1d4308ab@redhat.com> <344ae454-b285-e829-ada8-2d3097e9207a@cs.ucla.edu> From: Paul Eggert Message-ID: <53984fd3-83cf-1688-1aa4-5ccb7b72e416@cs.ucla.edu> Date: Thu, 21 Sep 2017 15:53:42 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: Adhemerval Zanella wrote: > Now back to the patch topic, I think the first union version [1] should > the most suitable one and it is what I am intended to push. > > [1]https://sourceware.org/ml/libc-alpha/2017-09/msg00730.html The code for that looks fine for Gnulib. However, the commit message could be improved. It needs a summary line that does not mention _Alignas. "an union" should be "a union". There is no need to cite every use affected by the change; just say "All uses changed". It would be nice to credit Florian for pointing out the problem and Andreas for suggesting a solution. Attached is a proposed patch to Gnulib, which uses the same code as the abovementioned glibc patch, but which has an improved commit message; please use it as a guideline for the glibc commit. One more thing. Gnulib and other GNU projects are switching to using https: URLs to gnu.org and to fsf.org, and this causes the shared glob sources to differ in one line. Glibc should be fixed to use https: instead of http: for citations to GNU and FSF web sites. That should be a separate patch, of course. I'll look into it. From de22924261ba7cb489afdbc8fe9dd6013da6f1cb Mon Sep 17 00:00:00 2001 From: Adhemerval Zanella Date: Thu, 21 Sep 2017 15:40:07 -0700 Subject: [PATCH] scratch_buffer: use union for internal buffer * lib/malloc/scratch_buffer.h (struct scratch_buffer): Use a union instead of a max_align_t array for __space, so that __space is the same size on all platforms. Problem reported by Florian Weimer in: https://sourceware.org/ml/libc-alpha/2017-09/msg00693.html Solution suggested by Andreas Schwab as discussed in: https://sourceware.org/ml/libc-alpha/2017-09/msg00695.html All uses changed. --- ChangeLog | 12 ++++++++++++ lib/malloc/scratch_buffer.h | 6 +++--- lib/malloc/scratch_buffer_grow_preserve.c | 4 ++-- 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/ChangeLog b/ChangeLog index 6a0b496..ecbab28 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,15 @@ +2017-09-21 Adhemerval Zanella + + scratch_buffer: use union for internal buffer + * lib/malloc/scratch_buffer.h (struct scratch_buffer): + Use an union instead of a max_align_t array for __space, + so that __space is the same size on all platforms. + Problem reported by Florian Weimer in: + https://sourceware.org/ml/libc-alpha/2017-09/msg00693.html + Solution suggested by Andreas Schwab as discussed in: + https://sourceware.org/ml/libc-alpha/2017-09/msg00695.html + All uses changed. + 2017-09-16 Paul Eggert manywarnings: port to GCC on 64-bit MS-Windows diff --git a/lib/malloc/scratch_buffer.h b/lib/malloc/scratch_buffer.h index 206b320..7d9020c 100644 --- a/lib/malloc/scratch_buffer.h +++ b/lib/malloc/scratch_buffer.h @@ -66,7 +66,7 @@ struct scratch_buffer { void *data; /* Pointer to the beginning of the scratch area. */ size_t length; /* Allocated space at the data pointer, in bytes. */ - max_align_t __space[(1023 + sizeof (max_align_t)) / sizeof (max_align_t)]; + union { max_align_t __a; char __c[1024]; } __space; }; /* Initializes *BUFFER so that BUFFER->data points to BUFFER->__space @@ -74,7 +74,7 @@ struct scratch_buffer { static inline void scratch_buffer_init (struct scratch_buffer *buffer) { - buffer->data = buffer->__space; + buffer->data = buffer->__space.__c; buffer->length = sizeof (buffer->__space); } @@ -82,7 +82,7 @@ scratch_buffer_init (struct scratch_buffer *buffer) static inline void scratch_buffer_free (struct scratch_buffer *buffer) { - if (buffer->data != buffer->__space) + if (buffer->data != buffer->__space.__c) free (buffer->data); } diff --git a/lib/malloc/scratch_buffer_grow_preserve.c b/lib/malloc/scratch_buffer_grow_preserve.c index cf55cf2..c752bdd 100644 --- a/lib/malloc/scratch_buffer_grow_preserve.c +++ b/lib/malloc/scratch_buffer_grow_preserve.c @@ -30,14 +30,14 @@ __libc_scratch_buffer_grow_preserve (struct scratch_buffer *buffer) size_t new_length = 2 * buffer->length; void *new_ptr; - if (buffer->data == buffer->__space) + if (buffer->data == buffer->__space.__c) { /* Move buffer to the heap. No overflow is possible because buffer->length describes a small buffer on the stack. */ new_ptr = malloc (new_length); if (new_ptr == NULL) return false; - memcpy (new_ptr, buffer->__space, buffer->length); + memcpy (new_ptr, buffer->__space.__c, buffer->length); } else { -- 2.7.4