From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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) From: "Maciej W. Rozycki" To: Alistair Francis cc: libc-alpha@sourceware.org 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-Spam-Status: No, score=-10.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, SPF_HELO_PASS, 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: Thu, 16 Jul 2020 07:03:34 -0000 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