From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.120]) by sourceware.org (Postfix) with ESMTP id 5197E3870852 for ; Tue, 2 Jun 2020 03:39:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 5197E3870852 Received: from mail-qk1-f199.google.com (mail-qk1-f199.google.com [209.85.222.199]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-142-MLyu_5dhOYCnEvOMgQjC3g-1; Mon, 01 Jun 2020 23:39:34 -0400 X-MC-Unique: MLyu_5dhOYCnEvOMgQjC3g-1 Received: by mail-qk1-f199.google.com with SMTP id v197so6190240qkb.16 for ; Mon, 01 Jun 2020 20:39:34 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:organization :message-id:date:user-agent:mime-version:in-reply-to :content-language:content-transfer-encoding; bh=GRhW1O5aqJac3lm4fWuFDCmCpbERPHYJb+RNWQqz4PY=; b=D2MQ9A/7wE7F0nrxqY6QsmPZRVKjBtpUxQUBUxGgsDo2KMgMnLfQIN55XechL+UdRL 7eOjEHoAuokc5/QQgwldp/TLgoaIuUJh/JXPdKIBRBJ9vvRBVUjzNqPBWX2mBRzc5KBi ovf+2zW0ob3BPn6Qkw0u6uqDI8JxvGAEpN1OGtEbSYucFRdmryZIIca9f7EVbLD++7qw MJSh2Fe86YnhaXC2mykE1lVe6xLeNnuBRBbBsiAJ1ROZORNAEWmbatL48DBnqjk5OYCk rEhUsfm61whPRCz+hEdD+fCU7D1Z+EU406RaJTeLwf7KQvaDQg6f5fQoHJpuyQlkX8C3 PihQ== X-Gm-Message-State: AOAM532/DsJTwXJRGnbDCoqFzEQzCLpChU5C868hTOJ0qca+WS8hDjvU bMpinm1iUrsyhaRzVe2VlsT0WzXpTLGzWUKQCpXYsKNM/hozIZqIZpB7eZElBroHHeMXk5y6AFo 7FPGEziyUbnMDCrr7i+S0 X-Received: by 2002:a37:6cd:: with SMTP id 196mr22563019qkg.393.1591069173359; Mon, 01 Jun 2020 20:39:33 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxO5OdL0P8m3FFbhJqwYdvU+aFmOVzB+wvEOR020FJBg48b/kUUngGdqLkdmGqgkfYXsPVdFg== X-Received: by 2002:a37:6cd:: with SMTP id 196mr22563006qkg.393.1591069173056; Mon, 01 Jun 2020 20:39:33 -0700 (PDT) Received: from [192.168.1.4] (198-84-170-103.cpe.teksavvy.com. [198.84.170.103]) by smtp.gmail.com with ESMTPSA id 10sm1053130qkv.136.2020.06.01.20.39.32 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 01 Jun 2020 20:39:32 -0700 (PDT) Subject: Re: [PATCH 14/19] nptl: Change type of __default_pthread_attr To: Florian Weimer , libc-alpha@sourceware.org References: From: Carlos O'Donell Organization: Red Hat Message-ID: <87c23469-c65b-79a3-e8fa-a559ec74960a@redhat.com> Date: Mon, 1 Jun 2020 23:39:31 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: Content-Language: en-US X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-13.0 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 02 Jun 2020 03:39:37 -0000 On 5/19/20 6:44 AM, Florian Weimer via Libc-alpha wrote: > union pthread_attr_transparent has always the correct size, even if > pthread_attr_t has padding that is not present in struct pthread_attr. > > This should not result in an observable behavioral change. The > existing code appears to have been correct, but it was brittle because > it was not clear which functions were allowed to write to an entire > pthread_attr_t argument (e.g., by copying it). OK for master. Reviewed-by: Carlos O'Donell > --- > nptl/allocatestack.c | 2 +- > nptl/nptl-init.c | 4 ++-- > nptl/pthreadP.h | 2 +- > nptl/pthread_attr_getstacksize.c | 2 +- > nptl/pthread_create.c | 8 ++++---- > nptl/pthread_getattr_default_np.c | 3 +-- > nptl/pthread_setattr_default_np.c | 6 +++--- > nptl/vars.c | 2 +- > 8 files changed, 14 insertions(+), 15 deletions(-) > > diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c > index c94980c21c..d16f3d71f8 100644 > --- a/nptl/allocatestack.c > +++ b/nptl/allocatestack.c > @@ -425,7 +425,7 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp, > else > { > lll_lock (__default_pthread_attr_lock, LLL_PRIVATE); > - size = __default_pthread_attr.stacksize; > + size = __default_pthread_attr.internal.stacksize; OK. > lll_unlock (__default_pthread_attr_lock, LLL_PRIVATE); > } > > diff --git a/nptl/nptl-init.c b/nptl/nptl-init.c > index 96b1444a01..d4cf20e3d1 100644 > --- a/nptl/nptl-init.c > +++ b/nptl/nptl-init.c > @@ -318,8 +318,8 @@ __pthread_initialize_minimal_internal (void) > /* Round the resource limit up to page size. */ > limit.rlim_cur = ALIGN_UP (limit.rlim_cur, pagesz); > lll_lock (__default_pthread_attr_lock, LLL_PRIVATE); > - __default_pthread_attr.stacksize = limit.rlim_cur; > - __default_pthread_attr.guardsize = GLRO (dl_pagesize); > + __default_pthread_attr.internal.stacksize = limit.rlim_cur; > + __default_pthread_attr.internal.guardsize = GLRO (dl_pagesize); OK. > lll_unlock (__default_pthread_attr_lock, LLL_PRIVATE); > > #ifdef SHARED > diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h > index 9c6dd41b7c..acc8e88e4a 100644 > --- a/nptl/pthreadP.h > +++ b/nptl/pthreadP.h > @@ -199,7 +199,7 @@ enum > > > /* Default pthread attributes. */ > -extern struct pthread_attr __default_pthread_attr attribute_hidden; > +extern union pthread_attr_transparent __default_pthread_attr attribute_hidden; OK. > extern int __default_pthread_attr_lock attribute_hidden; > > /* Size and alignment of static TLS block. */ > diff --git a/nptl/pthread_attr_getstacksize.c b/nptl/pthread_attr_getstacksize.c > index 346b375690..9830a635a6 100644 > --- a/nptl/pthread_attr_getstacksize.c > +++ b/nptl/pthread_attr_getstacksize.c > @@ -33,7 +33,7 @@ __pthread_attr_getstacksize (const pthread_attr_t *attr, size_t *stacksize) > if (size == 0) > { > lll_lock (__default_pthread_attr_lock, LLL_PRIVATE); > - size = __default_pthread_attr.stacksize; > + size = __default_pthread_attr.internal.stacksize; OK. > lll_unlock (__default_pthread_attr_lock, LLL_PRIVATE); > } > *stacksize = size; > diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c > index 347d510707..86fbeb5218 100644 > --- a/nptl/pthread_create.c > +++ b/nptl/pthread_create.c > @@ -612,16 +612,16 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr, > STACK_VARIABLES; > > const struct pthread_attr *iattr = (struct pthread_attr *) attr; > - struct pthread_attr default_attr; > + union pthread_attr_transparent default_attr; OK. > bool destroy_default_attr = false; > bool c11 = (attr == ATTR_C11_THREAD); > if (iattr == NULL || c11) > { > - int ret = __pthread_getattr_default_np ((pthread_attr_t *) &default_attr); > + int ret = __pthread_getattr_default_np (&default_attr.external); OK. Uses external type for public API. > if (ret != 0) > return ret; > destroy_default_attr = true; > - iattr = &default_attr; > + iattr = &default_attr.internal; OK. > } > > struct pthread *pd = NULL; > @@ -852,7 +852,7 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr, > > out: > if (destroy_default_attr) > - __pthread_attr_destroy ((pthread_attr_t *) &default_attr); > + __pthread_attr_destroy (&default_attr.external); OK. > > return retval; > } > diff --git a/nptl/pthread_getattr_default_np.c b/nptl/pthread_getattr_default_np.c > index 5c99f980e2..f3ce1c2885 100644 > --- a/nptl/pthread_getattr_default_np.c > +++ b/nptl/pthread_getattr_default_np.c > @@ -22,8 +22,7 @@ int > __pthread_getattr_default_np (pthread_attr_t *out) > { > lll_lock (__default_pthread_attr_lock, LLL_PRIVATE); > - int ret = __pthread_attr_copy (out, > - (pthread_attr_t *) &__default_pthread_attr); > + int ret = __pthread_attr_copy (out, &__default_pthread_attr.external); OK. > lll_unlock (__default_pthread_attr_lock, LLL_PRIVATE); > return ret; > } > diff --git a/nptl/pthread_setattr_default_np.c b/nptl/pthread_setattr_default_np.c > index eb5d24d3bd..c4cfb4e8ef 100644 > --- a/nptl/pthread_setattr_default_np.c > +++ b/nptl/pthread_setattr_default_np.c > @@ -68,15 +68,15 @@ pthread_setattr_default_np (const pthread_attr_t *in) > > /* Preserve the previous stack size (see above). */ > if (temp.internal.stacksize == 0) > - temp.internal.stacksize = __default_pthread_attr.stacksize; > + temp.internal.stacksize = __default_pthread_attr.internal.stacksize; > > /* Destroy the old attribute structure because it will be > overwritten. */ > - __pthread_attr_destroy ((pthread_attr_t *) &__default_pthread_attr); > + __pthread_attr_destroy (&__default_pthread_attr.external); > > /* __default_pthread_attr takes ownership, so do not free > attrs.internal after this point. */ > - __default_pthread_attr = temp.internal; > + __default_pthread_attr = temp; OK. > > lll_unlock (__default_pthread_attr_lock, LLL_PRIVATE); > return ret; > diff --git a/nptl/vars.c b/nptl/vars.c > index b88300d9b4..3696020145 100644 > --- a/nptl/vars.c > +++ b/nptl/vars.c > @@ -22,7 +22,7 @@ > > /* Default thread attributes for the case when the user does not > provide any. */ > -struct pthread_attr __default_pthread_attr attribute_hidden; > +union pthread_attr_transparent __default_pthread_attr attribute_hidden; OK. > > /* Mutex protecting __default_pthread_attr. */ > int __default_pthread_attr_lock = LLL_LOCK_INITIALIZER; > -- Cheers, Carlos.