From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-Status: No, score=-4.2 required=3.0 tests=AWL,BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,MAILING_LIST_MULTI, SPF_HELO_PASS,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by dcvr.yhbt.net (Postfix) with ESMTPS id E3F171F66F for ; Wed, 4 Nov 2020 11:57:33 +0000 (UTC) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id E7E133851C31; Wed, 4 Nov 2020 11:57:32 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org E7E133851C31 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1604491053; bh=khQRRI6hQoMr10mcVpRVcdI8MDWiHRFIqw7n8KcZJ10=; h=To:Subject:References:Date:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=dTVsPZacl/+CIrVRqQXqsIbQ1C7fBnc62itvaa5cA+/hxkQrB4pKgaoTY4WAeRLTR irmsGm+IMQH3SFwAhjmEpHnpwCSP3zsI0HeiHkEseUrbVT5dfqyaA61n//Wi0eIl8H VTLOT/lDK4LdizvQMDz4t2M7H/4MfStaMM0h7VSM= Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id 0CEF53857811 for ; Wed, 4 Nov 2020 11:57:30 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 0CEF53857811 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-281-r8OPMBkfP_WhQXky-LBhyg-1; Wed, 04 Nov 2020 06:57:25 -0500 X-MC-Unique: r8OPMBkfP_WhQXky-LBhyg-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id C801D1028D54; Wed, 4 Nov 2020 11:57:24 +0000 (UTC) Received: from oldenburg2.str.redhat.com (ovpn-113-12.ams2.redhat.com [10.36.113.12]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 6D5477E16D; Wed, 4 Nov 2020 11:57:23 +0000 (UTC) To: Adhemerval Zanella via Libc-alpha Subject: Re: [PATCH 27/28] elf: Process glibc-hwcaps subdirectories in ldconfig References: Date: Wed, 04 Nov 2020 12:57:20 +0100 In-Reply-To: (Adhemerval Zanella via Libc-alpha's message of "Tue, 27 Oct 2020 14:28:25 -0300") Message-ID: <87sg9p2v33.fsf@oldenburg2.str.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain 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: , From: Florian Weimer via Libc-alpha Reply-To: Florian Weimer Errors-To: libc-alpha-bounces@sourceware.org Sender: "Libc-alpha" * Adhemerval Zanella via Libc-alpha: >> +/* Helper for sorting struct glibc_hwcaps_subdirectory elements by >> + name. */ >> +static int >> +assign_glibc_hwcaps_indices_compare (const void *l, const void *r) >> +{ >> + const struct glibc_hwcaps_subdirectory *left >> + = *(struct glibc_hwcaps_subdirectory **)l; >> + const struct glibc_hwcaps_subdirectory *right >> + = *(struct glibc_hwcaps_subdirectory **)r; >> + return strcmp (left->name->string, right->name->string); >> +} >> + > > Maybe: > > strcmp (glibc_hwcaps_subdirectory_name (left), > glibc_hwcaps_subdirectory_name (right)); Fixed. >> +/* Compute the section_index fields for all */ >> +static void >> +assign_glibc_hwcaps_indices (void) >> +{ >> + /* Convert the linked list into an array, so that we can use qsort. >> + Only copy the subdirectories which are actually used. */ >> + size_t count = glibc_hwcaps_count (); >> + struct glibc_hwcaps_subdirectory **array >> + = xmalloc (sizeof (*array) * count); >> + { >> + size_t i = 0; >> + for (struct glibc_hwcaps_subdirectory *p = hwcaps; p != NULL; p = p->next) >> + if (p->used) >> + { >> + array[i] = p; >> + ++i; >> + } >> + assert (i == count); > > Do we need this assert? I think it would make sense if hwcaps is modified > concurrently, which does not seem the case. It documents that the loop processed the entire array, consistent with glibc_hwcaps_count. Right now, the function is defined immediately above, but that could change, and I think it is reasonable to capture this dependency. >> @@ -311,8 +442,22 @@ compare (const struct cache_entry *e1, const struct cache_entry *e2) >> return 1; >> else if (e1->flags > e2->flags) >> return -1; >> + /* Keep the glibc-hwcaps extension entries before the regular >> + entries, and sort them by their names. search_cache in >> + dl-cache.c stops searching once the first non-extension entry >> + is found, so the extension entries need to come first. */ >> + else if (e1->hwcaps != NULL && e2->hwcaps == NULL) >> + return -1; >> + else if (e1->hwcaps == NULL && e2->hwcaps != NULL) >> + return 1; >> + else if (e1->hwcaps != NULL && e2->hwcaps != NULL) >> + { >> + res = strcmp (e1->hwcaps->name->string, e2->hwcaps->name->string); > > Maybe: > > res = strcmp (glibc_hwcaps_subdirectory_name (e1->hwcaps), > glibc_hwcaps_subdirectory_name (e2->hwcaps)); Fixed. >> -/* Write the cache extensions to FD. The extension directory is >> - assumed to be located at CACHE_EXTENSION_OFFSET. */ >> +/* Write the cache extensions to FD. The string table is shifted by >> + STRING_TABLE_OFFSET. The extension directory is assumed to be >> + located at CACHE_EXTENSION_OFFSET. assign_glibc_hwcaps_indices >> + must have been called. */ >> static void >> -write_extensions (int fd, uint32_t cache_extension_offset) >> +write_extensions (int fd, uint32_t str_offset, >> + uint32_t cache_extension_offset) >> { >> assert ((cache_extension_offset % 4) == 0); >> >> + /* The length and contents of the glibc-hwcaps section. */ >> + uint32_t hwcaps_count = glibc_hwcaps_count (); >> + uint32_t hwcaps_offset = cache_extension_offset + cache_extension_size; >> + uint32_t hwcaps_size = hwcaps_count * sizeof (uint32_t); >> + uint32_t *hwcaps_array = xmalloc (hwcaps_size); >> + for (struct glibc_hwcaps_subdirectory *p = hwcaps; p != NULL; p = p->next) >> + if (p->used) >> + hwcaps_array[p->section_index] = str_offset + p->name->offset; >> + >> + /* This is the offset of the generator string. */ >> + uint32_t generator_offset = hwcaps_offset; >> + if (hwcaps_count == 0) >> + /* There is no section for the hwcaps subdirectories. */ >> + generator_offset -= sizeof (struct cache_extension_section); >> + else >> + /* The string table indices for the hwcaps subdirectories shift >> + the generator string backwards. */ >> + generator_offset += hwcaps_count * sizeof (uint32_t); > > Maybe > > generator_offset += hwcaps_size; Fixed. >> struct cache_extension *ext = xmalloc (cache_extension_size); >> ext->magic = cache_extension_magic; >> - ext->count = cache_extension_count; >> >> - for (int i = 0; i < cache_extension_count; ++i) >> - { >> - ext->sections[i].tag = i; >> - ext->sections[i].flags = 0; >> - } > > Ok, although maybe you could refactor the 'elf: Add extension > mechanism to ld.so.cache' to avoid add such code. This is still quite ad-hoc. I expect that the code will change again when we have additional extensions and a common pattern emerges. >> + /* Extension index current being filled. */ >> + size_t xid = 0; >> >> const char *generator >> = "ldconfig " PKGVERSION RELEASE " release version " VERSION; >> - ext->sections[cache_extension_tag_generator].offset >> - = cache_extension_offset + cache_extension_size; >> - ext->sections[cache_extension_tag_generator].size = strlen (generator); >> + ext->sections[xid].tag = cache_extension_tag_generator; >> + ext->sections[xid].flags = 0; >> + ext->sections[xid].offset = generator_offset; >> + ext->sections[xid].size = strlen (generator); >> + >> + if (hwcaps_count > 0) >> + { >> + ++xid; >> + ext->sections[xid].tag = cache_extension_tag_glibc_hwcaps; >> + ext->sections[xid].flags = 0; >> + ext->sections[xid].offset = hwcaps_offset; >> + ext->sections[xid].size = hwcaps_size; >> + } >> + >> + ++xid; >> + ext->count = xid; >> + assert (xid <= cache_extension_count); > > Would it make more sense to reference the index directly using the > enumeration instead or add an assert to check if the index is within > the expected size? In the future, the index does not necessarily equal the tag value. We don't write the glibc-hwcaps extension if no such subdirectories exist. >> - if (write (fd, ext, cache_extension_size) != cache_extension_size >> + size_t ext_size = (offsetof (struct cache_extension, sections) >> + + xid * sizeof (struct cache_extension_section)); > > So here we could just use cache_extension_count instead of 'xid' (with > the advantage that we certify at compile-time that only know > cache_extension_count will be written on the file). I think we shouldn't write extensions that aren't used. It will help to make sure that the loader code is tolerant of extensions. >> @@ -838,7 +932,10 @@ search_dir (const struct dir_entry *entry) >> else >> is_dir = S_ISDIR (lstat_buf.st_mode); >> >> - if (is_dir && is_hwcap_platform (direntry->d_name)) >> + /* No descending into subdirectories if this directory is a >> + glibc-hwcaps subdirectory (which are not recursive). */ >> + if (entry->hwcaps == NULL >> + && is_dir && is_hwcap_platform (direntry->d_name)) >> { >> if (!is_link >> && direntry->d_type != DT_UNKNOWN > > is_dir is an 'int', maybe make it a boolean? Fixed. >> diff --git a/sysdeps/generic/dl-cache.h b/sysdeps/generic/dl-cache.h >> index fec209509d..66b0312ac1 100644 >> --- a/sysdeps/generic/dl-cache.h >> +++ b/sysdeps/generic/dl-cache.h >> @@ -81,7 +81,6 @@ struct cache_file >> #define CACHE_VERSION "1.1" >> #define CACHEMAGIC_VERSION_NEW CACHEMAGIC_NEW CACHE_VERSION >> >> - > > Spurious line removal. Fixed. Thanks, Florian -- Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn, Commercial register: Amtsgericht Muenchen, HRB 153243, Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill