From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 492F03858D1E for ; Tue, 2 May 2023 12:43:52 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 492F03858D1E Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1683031431; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=53bnFbZm74TBnFotfvGDlH/z+GeMttkl97LVPdfZXsA=; b=hdttLAdsUlO+FViOGehix5e5d18QtZAf9mDM6mw19zJJHI91t3JZVlU6nzy4MD1cY3MUBR oBinhaC187xtHMJBkHzMerDN3H/b8Pg1AKeonR6jyA3xlt/0mEb9pnpkM5vMRWbgZF+gZ2 HYRtqbKH0TH7oanBn9O3jBD1c6fUTgU= Received: from mail-yw1-f199.google.com (mail-yw1-f199.google.com [209.85.128.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-203-FzkftYbTPf2qaZbwwg9_fQ-1; Tue, 02 May 2023 08:43:51 -0400 X-MC-Unique: FzkftYbTPf2qaZbwwg9_fQ-1 Received: by mail-yw1-f199.google.com with SMTP id 00721157ae682-559ffd15df9so49074677b3.3 for ; Tue, 02 May 2023 05:43:50 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1683031430; x=1685623430; h=content-transfer-encoding:in-reply-to:organization:from:references :to:content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=53bnFbZm74TBnFotfvGDlH/z+GeMttkl97LVPdfZXsA=; b=kFa6rlY5mp/uKDjAVr52QScd55vAj9AGgJKViuXhxuXi7AdsB56XCmk0b36RL/L4Jm K9Nf9onsgwfbhl5gMvKZ9q4S5mmluTI1n2HmJRG9ZQ+GzogWM+Iu2n1erqBi9TP5IlBk WSXgSg9TPBfPy5ej9Xwg3W1W5QKNSRnL3+T1mBucqfv43ePicoxlEZamehsiUFFiRy5X rwNcxcnZRlNhBJpGXcz+h8h6lTP2biIzQwNFqt2+FaWoKLaCQJ2388d0EYGqvheJqeJs Qv6scFw3Z/PF8vBPQwZr5I5Sb/jYpfqshC/69bf4BHEe1zTSfWCy1ztWtgv1DaUNyIb0 lzhg== X-Gm-Message-State: AC+VfDx7rQa9O5aOsFj+m5xJ66QAyLcIC3hQeZ6xWPlI+vGuWHl2fRsK LFyF/U4KWQzfUEW6fUcPwOzYqRCb8HU3c/yJvuM/DsM+/O+snsu1EDf4lFtCab+fgLG/6b+95QM btdx+xHmSOidHe5tRUOXr X-Received: by 2002:a0d:d6c8:0:b0:55a:da:8d1a with SMTP id y191-20020a0dd6c8000000b0055a00da8d1amr10896411ywd.36.1683031430168; Tue, 02 May 2023 05:43:50 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ5mrauQwRWdZEALNFhlVXeZppzEUKFlJWsu9m4iQc0htbDW5fCTcdxlXal439YsCujCOPlqCg== X-Received: by 2002:a0d:d6c8:0:b0:55a:da:8d1a with SMTP id y191-20020a0dd6c8000000b0055a00da8d1amr10896389ywd.36.1683031429901; Tue, 02 May 2023 05:43:49 -0700 (PDT) Received: from [192.168.0.241] ([198.48.244.52]) by smtp.gmail.com with ESMTPSA id n187-20020a0de4c4000000b005463e45458bsm7984039ywe.123.2023.05.02.05.43.49 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 02 May 2023 05:43:49 -0700 (PDT) Message-ID: <53234b02-ffa6-13e9-0b9c-87325e15dc91@redhat.com> Date: Tue, 2 May 2023 08:43:48 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.10.0 Subject: Re: [patch v4] aligned_alloc: conform to C17 To: DJ Delorie , libc-alpha@sourceware.org References: From: Carlos O'Donell Organization: Red Hat In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-13.2 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_NUMSUBJECT,KAM_SHORT,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 3/22/23 17:38, DJ Delorie via Libc-alpha wrote: > > Changes from V3: > > * Simplified aligned_alloc symbol definition. > Looking forward to a v5. > References: > https://patchwork.sourceware.org/project/glibc/patch/33ec9e0c1e587813b90e8aa771c2c8e6e379dd48.camel@posteo.net/ > https://sourceware.org/bugzilla/show_bug.cgi?id=20137 > https://sourceware.org/pipermail/libc-alpha/2023-February/145858.html > > The memory.texi portion matches Martin's proposed patch. > > man page portion, quoted to avoid CI/CD issues (I can send an official > patch separately after the glibc patch is applied): > >> diff --git a/man3/posix_memalign.3 b/man3/posix_memalign.3 >> index f5d6618b7..a73ff0421 100644 >> --- a/man3/posix_memalign.3 >> +++ b/man3/posix_memalign.3 >> @@ -91,9 +91,8 @@ The function >> is the same as >> .BR memalign (), >> except for the added restriction that >> -.I size >> -should be a multiple of >> -.IR alignment . >> +.I alignment >> +must be a power of two. >> .PP >> The obsolete function >> .BR valloc () > > From 9cc7d558d9c06edb95102151212b319f21acf603 Mon Sep 17 00:00:00 2001 > From: DJ Delorie > Date: Tue, 21 Mar 2023 00:46:43 -0400 > Subject: aligned_alloc: conform to C17 > > This patch adds the strict checking for power-of-two alignments > in aligned_alloc(), and updates the manual accordingly. > > diff --git a/malloc/Makefile b/malloc/Makefile > index dfb51d344c..691714fb52 100644 > --- a/malloc/Makefile > +++ b/malloc/Makefile > @@ -43,10 +43,12 @@ tests := mallocbug tst-malloc tst-valloc tst-calloc tst-obstack \ > tst-tcfree1 tst-tcfree2 tst-tcfree3 \ > tst-safe-linking \ > tst-mallocalign1 \ > + tst-aligned-alloc \ OK. Adds a new test. > > tests-static := \ > tst-interpose-static-nothread \ > - tst-interpose-static-thread > + tst-interpose-static-thread \ > + tst-aligned-alloc-static OK. Adds a new test (built static). > > # Test for the malloc_set_state symbol removed in glibc 2.25. > ifeq ($(have-GLIBC_2.23)$(build-shared),yesyes) > diff --git a/malloc/malloc-debug.c b/malloc/malloc-debug.c > index 3867d15698..da9d2340d3 100644 > --- a/malloc/malloc-debug.c > +++ b/malloc/malloc-debug.c > @@ -299,7 +299,14 @@ __debug_memalign (size_t alignment, size_t bytes) > return _debug_mid_memalign (alignment, bytes, RETURN_ADDRESS (0)); > } > strong_alias (__debug_memalign, memalign) > -strong_alias (__debug_memalign, aligned_alloc) > +static void * > +__debug_aligned_alloc (size_t alignment, size_t bytes) > +{ > + if (!powerof2 (alignment) || alignment == 0) > + return NULL; > + return _debug_mid_memalign (alignment, bytes, RETURN_ADDRESS (0)); > +} > +strong_alias (__debug_aligned_alloc, aligned_alloc) OK. Correctly provide a debug alias for aligned_alloc. > > static void * > __debug_pvalloc (size_t bytes) > diff --git a/malloc/malloc.c b/malloc/malloc.c > index 76c50e3f58..ece5b8a224 100644 > --- a/malloc/malloc.c > +++ b/malloc/malloc.c > @@ -3509,6 +3509,27 @@ __libc_memalign (size_t alignment, size_t bytes) > void *address = RETURN_ADDRESS (0); > return _mid_memalign (alignment, bytes, address); > } > +libc_hidden_def (__libc_memalign) > + > +/* For ISO C11. */ This is not true, it's for ISO C17 (starting with n2310). > +void * > +weak_function > +aligned_alloc (size_t alignment, size_t bytes) > +{ > + if (!__malloc_initialized) > + ptmalloc_init (); > + > + /* Similar to memalign, but ISO C17 requires an error for invalid > + alignments. Valid alignments are non-negative powers of two. */ This isn't quite correct. Suggest: /* Similar to memalign, but starting with ISO C17 the standard requires an error for alignments that are not supported by the implementation. Valid alignments for the current implementation are non-negative powers of two. */ The error is only in the case of alignments that are not supported by the implementation. > + if (!powerof2 (alignment) || alignment == 0) OK. Assumes MALLOC_ALIGNMENT is a power of 2, but that's true generally. > + { > + __set_errno (EINVAL); > + return 0; > + } > + > + void *address = RETURN_ADDRESS (0); > + return _mid_memalign (alignment, bytes, address); OK. We'll likely see back a higher alignment due to MALLOC_ALIGNMENT, but that's still aligned. > +} > > static void * > _mid_memalign (size_t alignment, size_t bytes, void *address) > @@ -3567,9 +3588,6 @@ _mid_memalign (size_t alignment, size_t bytes, void *address) > ar_ptr == arena_for_chunk (mem2chunk (p))); > return tag_new_usable (p); > } > -/* For ISO C11. */ > -weak_alias (__libc_memalign, aligned_alloc) > -libc_hidden_def (__libc_memalign) OK. > > void * > __libc_valloc (size_t bytes) > diff --git a/malloc/tst-aligned-alloc-static.c b/malloc/tst-aligned-alloc-static.c > new file mode 100644 > index 0000000000..d504473094 > --- /dev/null > +++ b/malloc/tst-aligned-alloc-static.c > @@ -0,0 +1 @@ > +#include "tst-aligned-alloc.c" > diff --git a/malloc/tst-aligned-alloc.c b/malloc/tst-aligned-alloc.c > new file mode 100644 > index 0000000000..d6739376d4 > --- /dev/null > +++ b/malloc/tst-aligned-alloc.c > @@ -0,0 +1,56 @@ > +/* Copyright (C) 2023 Free Software Foundation, Inc. Needs a first line explaining what the test does. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + . */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +static int > +do_test (void) > +{ > + void *p1; > + void *p2; > + void *p3; > + > + errno = 0; > + Suggest: /* The implementation supports alignments that are non-negative powers of 2. We test 5 distinct conditions here: - A non-negative power of 2 alignment e.g. 64. - A degenerate zero power of 2 alignment e.g. 1. - A non-power-of-2 alignment e.g. 65. - A zero alignment. - A corner case SIZE_MAX / 2 + 1 alignment. */ > + p1 = aligned_alloc (64, 64); > + > + if (p1 == NULL) > + FAIL_EXIT1 ("aligned_alloc(64, 64) failed"); Please add a degenerate test for alignment == 1 e.g. 2^(0) > + > + p2 = aligned_alloc (65, 64); > + > + if (p2 != NULL) > + FAIL_EXIT1 ("aligned_alloc(65, 64) did not fail"); > + > + p3 = aligned_alloc (0, 64); > + > + if (p3 != NULL) > + FAIL_EXIT1 ("aligned_alloc(0, 64) did not fail"); Please add a SIZE_MAX / 2 + 1 alignment case that has to fail. > + > + free (p1); > + return 0; > +} > + > +#define TEST_FUNCTION do_test () > +#include "../test-skeleton.c" > diff --git a/manual/memory.texi b/manual/memory.texi > index 9d3398a326..8952ff2bfa 100644 > --- a/manual/memory.texi > +++ b/manual/memory.texi > @@ -995,7 +995,7 @@ power of two than that, use @code{aligned_alloc} or @code{posix_memalign}. > @c Alias to memalign. > The @code{aligned_alloc} function allocates a block of @var{size} bytes whose > address is a multiple of @var{alignment}. The @var{alignment} must be a > -power of two and @var{size} must be a multiple of @var{alignment}. > +power of two. OK. > > The @code{aligned_alloc} function returns a null pointer on error and sets > @code{errno} to one of the following values: > -- Cheers, Carlos.