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 857501F5AE for ; Thu, 16 Jul 2020 07:03:36 +0000 (UTC) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 1CC33383F84A; Thu, 16 Jul 2020 07:03:35 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 1CC33383F84A DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1594883015; bh=XSSmzPao/2/a5eV6cqkXeLQOaM/zdj1z7vIY+YrFDvM=; h=Date:To:Subject:In-Reply-To:References:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=Gs9hAKbigaEsT1LfJhuiJJ0tjRLOWMZsYepbuGA1NjUmeLuPtidMw/7Liw+p0e5Ho 1nKpXeBY+fJaVJhiKHRCfva0hJKIuu0sVeS3vV2Adl2NpZDTq89Ot2H/+Yz/Fqssvn FRzvHl0MZrs1d2jw04RbHjXVYaR5kkB0OXV+3mAI= Received: from esa3.hgst.iphmx.com (esa3.hgst.iphmx.com [216.71.153.141]) by sourceware.org (Postfix) with ESMTPS id 15339385700A for ; Thu, 16 Jul 2020 07:03:31 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 15339385700A IronPort-SDR: 0zDD2DWCw21qrKIrgETl6FAjt7TlQBl55rw/vjVSI3bacj2qRTSm3qYcvimmEdJ2xo87FFzC8/ BUNLMoP1HUg4Ofz1oVn/EhL126PQMNWEZ+V12KChbWs/d71pWEU9NKCigTCUbbDP2BS1OWrMiH 2OqPEgwTFzaVgkIwhPg1oqm7BEFVM5gWOnSLdP20k+VJrqVXrZD6v++Qppc5kAlwoI4cIxOlLr tUhG2j4y0pe5jOB22thhaeSAzoZLpYlEKrwF0KSB/mSvVE7BOfYxW85ufrADP28JhZgpJjK1eg Ofk= X-IronPort-AV: E=Sophos;i="5.75,358,1589212800"; d="scan'208";a="146893237" Received: from h199-255-45-14.hgst.com (HELO uls-op-cesaep01.wdc.com) ([199.255.45.14]) by ob1.hgst.iphmx.com with ESMTP; 16 Jul 2020 15:03:31 +0800 IronPort-SDR: b5XJbyrTFJE8rTERQlfMFw4QtjZ8Ed380EHPveIUZMNu2Zqvxh/VZExd8wAPJEVBalD/TL4xKr 9wV0biMEb2LF7R2dWdi5G73fvvVySdJrY= Received: from uls-op-cesaip02.wdc.com ([10.248.3.37]) by uls-op-cesaep01.wdc.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 Jul 2020 23:51:54 -0700 IronPort-SDR: jTtO7gpETpCY5Rj/hUAe1GkQ3Erleis6OqUqpkjxstaNHcRBxF5o2XKPUKMnO4Mb9Ii6UgOtxB 5457pKzPtwcg== WDCIronportException: Internal Received: from unknown (HELO redsun52) ([10.149.66.28]) by uls-op-cesaip02.wdc.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 Jul 2020 00:03:30 -0700 Date: Thu, 16 Jul 2020 08:03:25 +0100 (BST) To: Alistair Francis Subject: Re: [PATCH v3 07/19] RISC-V: Add path of library directories for the 32-bit In-Reply-To: <5cf15612abb2f89e7cf7b76b1546b558751ce261.1594568655.git.alistair.francis@wdc.com> Message-ID: References: <5cf15612abb2f89e7cf7b76b1546b558751ce261.1594568655.git.alistair.francis@wdc.com> User-Agent: Alpine 2.21 (LFD 202 2017-01-01) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII 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: "Maciej W. Rozycki via Libc-alpha" Reply-To: "Maciej W. Rozycki" Cc: libc-alpha@sourceware.org Errors-To: libc-alpha-bounces@sourceware.org Sender: "Libc-alpha" On Sun, 12 Jul 2020, Alistair Francis via Libc-alpha wrote: > With RV32 support the list of possible RISC-V system directories > increases to: > - /lib64/lp64d > - /lib64/lp64 > - /lib32/ilp32d > - /lib32/ilp32 > - /lib (only ld.so) > > This patch changes the add_system_di () macro to support the new ilp32d add_system_dir > and ilp32 directories for RV32. While refactoring this code let's split ^ Missing space. > out the confusing if statements into a loop to make it easier to > understand and extend. This change doesn't appear to do what's intended; the list of directories printed without it is: /lib: (from :0) /lib64/lp64d: (from :0) /lib64/lp64: (from :0) /usr/lib: (from :0) /usr/lib64/lp64d: (from :0) /usr/lib64/lp64: (from :0) while with the change applied I only get: /lib64/lp64d: (from :0) > diff --git a/sysdeps/unix/sysv/linux/riscv/dl-cache.h b/sysdeps/unix/sysv/linux/riscv/dl-cache.h > index b3cda4ef9f..7317406036 100644 > --- a/sysdeps/unix/sysv/linux/riscv/dl-cache.h > +++ b/sysdeps/unix/sysv/linux/riscv/dl-cache.h [...] > @@ -45,25 +47,40 @@ > architectures and have that automatically imply /usr/local/lib64/lp64d > etc. so that libraries can be found that come from software that does > use the ABI-specific directories. */ > + > #define add_system_dir(dir) \ > do \ > { \ > + static const char* lib_dirs[] = { \ > + "/lib64/lp64d", \ > + "/lib64/lp64", \ > + "/lib32/ilp32d", \ > + "/lib32/ilp32", \ > + NULL, \ > + }; \ > size_t len = strlen (dir); \ > - char path[len + 9]; \ > + char path[len + 10]; \ > memcpy (path, dir, len + 1); \ > - if (len >= 12 && ! memcmp(path + len - 12, "/lib64/lp64d", 12)) \ > - { \ > - len -= 8; \ > - path[len] = '\0'; \ > - } \ > - if (len >= 11 && ! memcmp(path + len - 11, "/lib64/lp64", 11)) \ > - { \ > - len -= 7; \ > - path[len] = '\0'; \ > + int i = 0; \ > + const char* lib_dir = lib_dirs[0]; \ > + \ > + while (lib_dir != NULL) { \ > + size_t dir_len = strlen (lib_dir); \ > + if (len >= dir_len && ! memcmp(path + len - dir_len, lib_dir, dir_len)) { \ > + len -= dir_len + 4; \ Thinko here, and the reason for the breakage noted above; it should be: len -= dir_len - 4; > + path[len] = '\0'; \ > + break; \ > + } \ > + i++; \ > + lib_dir = lib_dirs[i]; \ Please place the block opening brace on its own on a separate line and indent by two spaces per level, replacing every group of 8 with a tab, and stay within 79 columns (preferably 74). Also the use of a space after the negation operator is deprecated. Please have a look at: for full details. This might be slightly cleaner if rewritten as a `for' loop: const size_t lib_len = sizeof ("/lib") - 1; \ size_t len = strlen (dir); \ char path[len + 10]; \ const char **ptr; \ \ memcpy (path, dir, len + 1); \ \ for (ptr = lib_dirs; *ptr != NULL; ptr++) \ { \ const char *lib_dir = *ptr; \ size_t dir_len = strlen (lib_dir); \ \ if (len >= dir_len \ && !memcmp (path + len - dir_len, lib_dir, dir_len)) \ { \ len -= dir_len - lib_len; \ path[len] = '\0'; \ break; \ } \ } \ add_dir (path); \ And then for the second part I previously requested: if (len >= lib_len \ && !memcmp (path + len - lib_len, "/lib", lib_len)) \ for (ptr = lib_dirs; *ptr != NULL; ptr++) \ { \ const char *lib_dir = *ptr; \ size_t dir_len = strlen (lib_dir); \ \ assert (dir_len >= lib_len); \ memcpy (path + len, lib_dir + lib_len, \ dir_len - lib_len + 1); \ add_dir (path); \ } \ replacing the current handcrafted chain of `memcpy' calls. (We could benefit from the use of the ARRAY_SIZE macro if we had it, but we don't, so let's not complicate things further; this is not a performance-critical piece of code). NB I'd keep any broken formatting as it is in otherwise unchanged lines, as this will make the change more readable and backporting easier. Any clean-up can be done with a follow-up change, preferably across the whole file (i.e. including the breakage I observed in the review of 06/19 and possibly other places). Maciej