From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from esa2.hgst.iphmx.com (esa2.hgst.iphmx.com [68.232.143.124]) by sourceware.org (Postfix) with ESMTPS id 544C83858D35 for ; Wed, 8 Jul 2020 18:42:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 544C83858D35 IronPort-SDR: vrVKzqaf4nyZ8AokPWVnjaTb76EyRAAvbmtuA3RpeubGmxVzKzWp3y3mUMPR9zhcsMNHo9FKoU Wm/gRHCAhiqmdaPYqwXlnJHCrK/UxegYImzifem6e9IfwFmHT7J43bMWnwkcqCbQ6BjR90q3ua DbrxsQYUf5YoHR1Y3BRf/fLZDy5qjYI/iP7XfPIy7KvR4Kr1e/QqM+EWBouxb/ujA9HebNGz/l R1pqA/54rUgB1ItEuyxO8AwLOMZ2VGnUgmVQ10V8It6+5VlVnBk/I+prjHkyhO6u2HFSMukjAZ moc= X-IronPort-AV: E=Sophos;i="5.75,328,1589212800"; d="scan'208";a="244972724" 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; 09 Jul 2020 02:42:31 +0800 IronPort-SDR: YnuglU7a0JOJ6uGoW1AR2dPQ/g1Q3vmXri4zmlwLbClkIJRYxU9nXhqZZSQCBSqS5DVl0kq3kU 50dnKOCWLuRKvAPJcx5jwAuOOkA/WoIUc= 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; 08 Jul 2020 11:30:59 -0700 IronPort-SDR: 7vYuSdEVGTQJCfliq82WuCNiINuFaNhGl/lQOs3dvIOECaLFkpIiVtJRgSzRkJNnLTCG0Siib1 lNesO9D8anVg== 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; 08 Jul 2020 11:42:22 -0700 Date: Wed, 8 Jul 2020 19:42:16 +0100 (BST) From: "Maciej W. Rozycki" To: Alistair Francis cc: libc-alpha@sourceware.org Subject: Re: [PATCH v2 05/18] RISC-V: Add path of library directories for the 32-bit In-Reply-To: <110083dce6019a1b98bda674d2e19acef4ed1dda.1591201405.git.alistair.francis@wdc.com> Message-ID: References: <110083dce6019a1b98bda674d2e19acef4ed1dda.1591201405.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.7 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, 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: Wed, 08 Jul 2020 18:42:24 -0000 On Wed, 3 Jun 2020, Alistair Francis via Libc-alpha wrote: > From: Zong Li > > For the recommand of 64 bit version, we add the libraries path of 32 bit > in this patch. I have troubles comprehending this sentence; also it's repeated literally below. Please rewrite an remove the duplication. > The status of RV32 binaries under RV64 kernels is that the ISA > optionally supports having different XLEN for user and supervisor modes, > but AFAIK there's no silicon that implements this feature, and the Linux > kernel doesn't support it yet. I'm not sure if this note is relevant here. > For the recommand of 64 bit version, we add the libraries path of 32 bit > in this patch. This includes a fix to avoid an out of bound array check > when building with GCC 8.2. Where exactly is that fix? Shouldn't it be applied as a separate preparatory change? > diff --git a/sysdeps/unix/sysv/linux/riscv/dl-cache.h b/sysdeps/unix/sysv/linux/riscv/dl-cache.h > index c297dfe84f..60fc172edb 100644 > --- a/sysdeps/unix/sysv/linux/riscv/dl-cache.h > +++ b/sysdeps/unix/sysv/linux/riscv/dl-cache.h [...] > @@ -49,9 +51,16 @@ > do \ > { \ > 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)) \ > + if (len >= 13 && ! memcmp(path + len - 13, "/lib32/ilp32d", 13)) \ > + { \ > + len -= 9; \ > + path[len] = '\0'; \ > + } \ > + if (len >= 12 \ > + && (! memcmp(path + len - 12, "/lib32/ilp32", 12) \ > + || ! memcmp(path + len - 12, "/lib64/lp64d", 12))) \ Please correct indentation above and use tabs throughout. I think this code should use `else' clauses. While the truncation of the string will cause subsequent `memcmp' calls to fail, they'll still be executed, `len' permitting, making this ugly. Though frankly with the growing number of entries I would rewrite this sequence of conditionals entirely, as a loop over a (static) array of the strings processed, cutting the insane number of error-prone hardcoded string lengths while at it too. It is only ever used in `ldconfig', and then invoked there at most twice, so it is not that it is critical enough for performance to justify illegibility, and making additional `strlen' calls shouldn't be a big deal. > @@ -64,6 +73,10 @@ > add_dir (path); \ > if (len >= 4 && ! memcmp(path + len - 4, "/lib", 4)) \ > { \ > + memcpy (path + len, "32/ilp32d", 10); \ > + add_dir (path); \ > + memcpy (path + len, "32/ilp32", 9); \ > + add_dir (path); \ Then the same array of strings could be used here (skipping over the "/lib" prefix throughout). Maciej