From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/12585 Path: news.gmane.org!.POSTED!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH] reduce size of struct __libc by using narrower integers Date: Tue, 6 Mar 2018 12:21:55 -0500 Message-ID: <20180306172155.GJ1436@brightrain.aerifal.cx> References: <20180306165502.23633-1-vda.linux@googlemail.com> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: blaine.gmane.org 1520356821 10387 195.159.176.226 (6 Mar 2018 17:20:21 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Tue, 6 Mar 2018 17:20:21 +0000 (UTC) User-Agent: Mutt/1.5.21 (2010-09-15) To: musl@lists.openwall.com Original-X-From: musl-return-12601-gllmg-musl=m.gmane.org@lists.openwall.com Tue Mar 06 18:20:17 2018 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by blaine.gmane.org with smtp (Exim 4.84_2) (envelope-from ) id 1etGFz-0000kf-1i for gllmg-musl@m.gmane.org; Tue, 06 Mar 2018 18:20:07 +0100 Original-Received: (qmail 32481 invoked by uid 550); 6 Mar 2018 17:22:08 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Original-Received: (qmail 32456 invoked from network); 6 Mar 2018 17:22:08 -0000 Content-Disposition: inline In-Reply-To: <20180306165502.23633-1-vda.linux@googlemail.com> Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:12585 Archived-At: On Tue, Mar 06, 2018 at 05:55:02PM +0100, Denys Vlasenko wrote: > can_do_threads, threaded and secure are boolean flags, can use bytes > instead of full word ints. > > tls_align and page_size are surely smaller than 4GB, no need to use > potentially 64-bit size_t. > > Signed-off-by: Denys Vlasenko > CC: musl > --- > ldso/dynlink.c | 3 ++- > src/internal/libc.h | 11 ++++++----- > 2 files changed, 8 insertions(+), 6 deletions(-) > > diff --git a/ldso/dynlink.c b/ldso/dynlink.c > index 9bf6924b..67d36395 100644 > --- a/ldso/dynlink.c > +++ b/ldso/dynlink.c > @@ -124,8 +124,9 @@ static jmp_buf *rtld_fail; > static pthread_rwlock_t lock; > static struct debug debug; > static struct tls_module *tls_tail; > -static size_t tls_cnt, tls_offset, tls_align = MIN_TLS_ALIGN; > +static size_t tls_cnt, tls_offset; > static size_t static_tls_cnt; > +static unsigned tls_align = MIN_TLS_ALIGN; > static pthread_mutex_t init_fini_lock = { ._m_type = PTHREAD_MUTEX_RECURSIVE }; > static struct fdpic_loadmap *app_loadmap; > static struct fdpic_dummy_loadmap app_dummy_loadmap; > diff --git a/src/internal/libc.h b/src/internal/libc.h > index 5e145183..3cd44e90 100644 > --- a/src/internal/libc.h > +++ b/src/internal/libc.h > @@ -18,14 +18,15 @@ struct tls_module { > }; > > struct __libc { > - int can_do_threads; > - int threaded; > - int secure; > + char can_do_threads; > + char threaded; > + char secure; > volatile int threads_minus_1; > size_t *auxv; > struct tls_module *tls_head; > - size_t tls_size, tls_align, tls_cnt; > - size_t page_size; > + size_t tls_size, tls_cnt; > + unsigned tls_align; > + unsigned page_size; > struct __locale_struct global_locale; > }; This might be okay but needs some care, as it introduces some subtle changes wrt types. For instance, PAGE_SIZE is #defined (internally for musl) using libc.page_size when it's not constant for the arch, and unsigned int will promote & compare differently against signed longs than size_t would. However, I think you've actually shed light on a bigger bug here: PAGE_SIZE, when it is defined in the public headers, has the wrong type (int rather than unsigned int or unsigned long). It needs to be fixed to use U or UL suffix. I'll do this along with the PAGESIZE/PAGE_SIZE swap logic (pending namespace issue) right away. The int->char changes are only valid if we only store positive values, since char has arch-specific signedness, but I think it's true that we only store 0 or 1. For tls_align it's true that >4GB would be ridiculous and breaking, but there are no checks to see that a loaded module does not have ridiculous alignment requirements except for failure of the allocation requests. If we change to a type that can't represent all possible inputs, there needs to be a check to reject large alignments anywhere they could arise. I think that means __init_tls.c and dynlink.c's load_library(). Rich