From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from aye.elm.relay.mailchannels.net (aye.elm.relay.mailchannels.net [23.83.212.6]) by sourceware.org (Postfix) with ESMTPS id 919CA3858D1E for ; Sun, 1 Oct 2023 16:34:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 919CA3858D1E Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=eagercon.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=eagercon.com X-Sender-Id: dreamhost|x-authsender|eager@eagerm.com Received: from relay.mailchannels.net (localhost [127.0.0.1]) by relay.mailchannels.net (Postfix) with ESMTP id 830F5C1633; Sun, 1 Oct 2023 16:34:21 +0000 (UTC) Received: from pdx1-sub0-mail-a288.dreamhost.com (unknown [127.0.0.6]) (Authenticated sender: dreamhost) by relay.mailchannels.net (Postfix) with ESMTPA id 0A992C129A; Sun, 1 Oct 2023 16:34:21 +0000 (UTC) ARC-Seal: i=1; s=arc-2022; d=mailchannels.net; t=1696178061; a=rsa-sha256; cv=none; b=pQxdYLNEPN7rNVEgCFkGAmUxybiFDVpsP0oahFpyuZzYKelyRB6V8rhY7mYcj8psfv6cE5 kuB9BHM4mTFsL//A+fczxuHgp3fx8+KholzsLK6FfNSeqXVRjk9JVJw+ekz7zT0rHKryyJ LFvQsvVOU3PnZkq+WmjSm+b0EnMgSnh0baW5UPOOtK6WBM46keS8N0MCcOvZZZaOyDBpap qjbxtUIxCuTzukYmRmrHBl5kKYjMSGvbneG0xIOBAoz1IeiRUxe40OXmjunu+rEegE8ZVr MelZvdXlsiDGtpGmbmh4Sl5tjAyELHTJhUc9SexX03wEgzgOZ1+JWZRcVprBWw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=mailchannels.net; s=arc-2022; t=1696178061; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=XIj5LohyMJa1Zr21vOzy3b2OUG6fIEcohvEFSNu4qb4=; b=Eak0QFQBcXyCQqLSkcxbldjBHJT08JaibRT6XXsutVY5o9waV9qWfFIrpSgwBnwUfpcjCo JKHEVVqrwHn/JHPKNE+O0XheqYlqvFLXvjuuyHaFsBi6GShTVyjJZh4WfMLlbZbMoz37r4 XDtQghnsRDEKqjdLdSZdPjEYkoavbGJheKukV008JcKdBwSMZppMykRRjVWUXluojxDap0 wTDsXU8x+JsfsRAek+pCdUzVQtOPdNy2v7V9uvWsgKu9f2S9gq6qOiuqZAp2XXxGYb34rm FTdoYz5YaxkUVPTp6+I+PcrDvOzAv8tPXh/4GRzZh4PLJdn1ADbjnhr7KJ+DaQ== ARC-Authentication-Results: i=1; rspamd-7d5dc8fd68-hsrxv; auth=pass smtp.auth=dreamhost smtp.mailfrom=eager@eagercon.com X-Sender-Id: dreamhost|x-authsender|eager@eagerm.com X-MC-Relay: Neutral X-MailChannels-SenderId: dreamhost|x-authsender|eager@eagerm.com X-MailChannels-Auth-Id: dreamhost X-Inform-Reaction: 60b8c7141fee588f_1696178061347_610354096 X-MC-Loop-Signature: 1696178061347:3333063077 X-MC-Ingress-Time: 1696178061346 Received: from pdx1-sub0-mail-a288.dreamhost.com (pop.dreamhost.com [64.90.62.162]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384) by 100.106.112.200 (trex/6.9.1); Sun, 01 Oct 2023 16:34:21 +0000 Received: from [192.168.20.10] (c-73-170-238-207.hsd1.ca.comcast.net [73.170.238.207]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: eager@eagerm.com) by pdx1-sub0-mail-a288.dreamhost.com (Postfix) with ESMTPSA id 4Rz8mr2hGmzDj; Sun, 1 Oct 2023 09:34:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=eagercon.com; s=dreamhost; t=1696178060; bh=XIj5LohyMJa1Zr21vOzy3b2OUG6fIEcohvEFSNu4qb4=; h=Date:Subject:To:Cc:From:Content-Type:Content-Transfer-Encoding; b=i3ukvyPcaxZnVT/+dcVTpfZJ+hZsLlh6RfRPPWbhKJ+RyLs6MWx5N/+P+e7JJK2Ca jE+DpJYblZH4mLNrVDKKWAUvflOLHFbHDltSmUR5/zB6bQFornaUmOrMRV1EDyzhbp d6ol8icl/OmdWOl7OnWN/WL88+2RD59gZGwvVZd3nrfopRxH+UDBLvFUQsq05edAYr MpcDz2SqG5L6/5VBgXRbFk4Y7H5wDF1OeegVQA+kwFB4lKxrYIoaLAeJGi1+RfpiLP jKkRrAdbVMl/RIulWX+ohnu0nvI/kYLK0bR85YLpNGtbDQsn3EDO80CwUjBYyhRami zEqcFvqeGObtA== Message-ID: <7fae40fc-0a1e-e38e-0836-9bd78bf78887@eagercon.com> Date: Sun, 1 Oct 2023 09:34:19 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 Subject: Re: [PATCH v1 1/1] gas: expr: fix support .long 0U and .long 0u Content-Language: en-US To: "Frager, Neal" , Jan Beulich Cc: "Erkiaga Elorza, Ibai" , "Mekala, Nagaraju" , "Hatle, Mark" , "Mutyala, Sadanand" , "Nali, Appa Rao" , "Hunsigida, Vidhumouli" , "luca.ceresoli@bootlin.com" , "binutils@sourceware.org" References: <20230927132130.1604555-1-neal.frager@amd.com> <424aef37-8ee6-5309-8342-502e0f41b73c@suse.com> <1eb5429b-94dc-e394-14f3-9063f19125eb@eagercon.com> From: Michael Eager In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-6.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 10/1/23 09:22, Frager, Neal wrote: >>> Fix support for .long 0U and .long 0u in GCC. >>> >>> This patch has been tested for years of AMD Xilinx Yocto releases as >>> part of the following patch set: >>> >>> https://github.com/Xilinx/meta-xilinx/tree/master/meta-microblaze/rec >>> ipes-devtools/binutils/binutils >>> >>> Signed-off-by: Neal Frager >>> --- >>> gas/expr.c | 9 +++++++++ >>> 1 file changed, 9 insertions(+) >> >> Without a testcase demonstrating what's wrong, I'd almost be inclined >> to say that ... >> >>> --- a/gas/expr.c >>> +++ b/gas/expr.c >>> @@ -824,6 +824,15 @@ operand (expressionS *expressionP, enum expr_mode mode) >>> break; >>> } >>> } >>> + if ((*input_line_pointer == 'U') || (*input_line_pointer == 'u')) >>> + { >>> + input_line_pointer--; >>> + >>> + integer_constant ((NUMBERS_WITH_SUFFIX || flag_m68k_mri) >>> + ? 0 : 10, >>> + expressionP); >>> + break; >>> + } >>> c = *input_line_pointer; >>> switch (c) >>> { >> >> ... this ought to be covered by logic in integer_constant() already. >> But I think I see what the issue is. Nevertheless, go look for >> tc_allow_U_suffix, which wants using here as well. Further I think the >> same issue then exists for L/l suffixes? >> >> Plus I think you want to add the new code to the switch() statement >> rather than immediately ahead of it. >> >> Jan >> > > Hi Michael, Jan, > >> Neal -- > >> Can you provide a test case for this patch? I'm not able to reproduce any error. > >> Please take a look at gas/expr.c:541-543 (PR 19910: Look for, and ignore, a U suffix to the number). Is this a fix for the same issue? >> https://sourceware.org/bugzilla/show_bug.cgi?id=19910 > >> As Jan says, it looks like this code, if needed, should be in the switch >> statement. > > Thank you for your review. > > I would just like to clarify where this patch is coming from. > I am not actually the author of the patches I am submitting. > When we provide a Microblaze toolchain with PetaLinux, Yocto or Vitis, we are actually applying all of the following patches to the binutils portion of the toolchain: > https://github.com/Xilinx/meta-xilinx/tree/master/meta-microblaze/recipes-devtools/binutils/binutils > > Instead of continuing to carry the maintenance of this patch set with all of our releases, I would like to get as many of these patches upstreamed as possible. > > This particular patch is coming from here: > https://github.com/Xilinx/meta-xilinx/blob/master/meta-microblaze/recipes-devtools/binutils/binutils/0009-Patch-Microblaze-fixed-bug-in-GCC-so-that-It-will-su.patch > > It is possible that this patch is not required anymore. I will check internally at AMD Xilinx to see if I can identify a way to reproduce the error that this patch is solving. > > Until I can prove that this patch is still needed, let's skip it. If it is really no longer necessary, I will try to get it removed from our list of patches. > > Thanks for your help! > > Best regards, > Neal Frager > AMD Hi Neal -- In general, a patch needs to be for a confirmed bug, not hypothetical and not for a problem that has already been fixed. It also needs to build with the top-of-tree sources cleanly. If possible, there should be a test case which shows that there is a bug and that the patch fixes is. When you sign off on a patch, that means that you are affirming that you have reviewed the patch and confirmed that this is the case. This reduces the amount of effort that I and others have to expend to review patches and avoids adding unnecessary changes to the source. -- Michael Eager