From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pj1-x1029.google.com (mail-pj1-x1029.google.com [IPv6:2607:f8b0:4864:20::1029]) by sourceware.org (Postfix) with ESMTPS id 70C713858D39 for ; Wed, 15 Mar 2023 16:05:16 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 70C713858D39 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=dabbelt.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=dabbelt.com Received: by mail-pj1-x1029.google.com with SMTP id d13so9313902pjh.0 for ; Wed, 15 Mar 2023 09:05:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=dabbelt-com.20210112.gappssmtp.com; s=20210112; t=1678896315; h=content-transfer-encoding:mime-version:message-id:to:from:cc :in-reply-to:subject:date:from:to:cc:subject:date:message-id :reply-to; bh=rlzUvu1sZ/STOcaX8B7i3nxeIJQ4hfIXSwBZG6wnes8=; b=KiFqk+06IybDnwXFCH+sM9FpCd0+25ABNWSfIRLj7N5Xm1XnjpX4T40qrcAMC8Zr9s nt7aWxXZin8FfrRZvtImhoarZ4BszUHNj0rx58Dp/MQcTA6QTZKjiFS1+51VCYNyO2rE OoiZMPsnFWL3Le+b2mBc9yaKgnmkD1q4P7HMpPkcDjjD4DG/yMsL5HnZojWVW5fqjG4e Mgm7DIjhfsY+0vmGXL2fQAMZR+x1Th9si/Z2IV8dhSbxzvf+1wnvq+O9MZgoGhxk22Gf qyH2jQNWxsPIIORfC+GUQWBF4i1ClCbqINXcRJ1P2g0WgHuLW972s/IzHHejACxtsnQ4 8WNg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678896315; h=content-transfer-encoding:mime-version:message-id:to:from:cc :in-reply-to:subject:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=rlzUvu1sZ/STOcaX8B7i3nxeIJQ4hfIXSwBZG6wnes8=; b=CU5qcXy4UStfDCQvlHsigBwSKFilaPiaQYkUJmmIUnDS2h6lATfZFPPw0y1M/JGl2n Kb2LuymLDJTkDWj0+PumIGitSxC2caYe0wuj6b0CjGevVBeuS14PrDvdMyyUMGFjP1uY avSyG9cKnOryz3gXrIpTgR3nWm/sJ2JjbnsV6vqvC5lIRKEicJTbPdWqjwBltQdgF+ha 2+R3WfPvOnG2ACmCz0hywMNatDnwO3cf6/ENkErM1IoiPjgrTQ1F99N2J9eZwMlDKO5P KklMMDESsYFVk5fq0X+ndx59NhY1+LdAiTfaDuB6a5BqdYJxzKgDqeAsFmfhF4JttLiy o8aA== X-Gm-Message-State: AO0yUKUSKcphIuX40xC+1xqssHpeP7ZZ1IBGagROuCQ9Ls4zi4OVNl4k WMXpR0jssIVsmx6uUptXvdntqTNdAFz4BB7VYPA= X-Google-Smtp-Source: AK7set8GtzppBMjcZGLF/NfV6lg46UHOw6ulDMnlyLlTkSbJTf5VIZOYE3SA4NYoKyrmbjOryi95ng== X-Received: by 2002:a05:6a20:2a26:b0:cc:aedf:9a1e with SMTP id e38-20020a056a202a2600b000ccaedf9a1emr157275pzh.61.1678896314920; Wed, 15 Mar 2023 09:05:14 -0700 (PDT) Received: from localhost ([50.221.140.188]) by smtp.gmail.com with ESMTPSA id e6-20020aa78c46000000b005d62cd8a3c9sm3745595pfd.71.2023.03.15.09.05.14 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 15 Mar 2023 09:05:14 -0700 (PDT) Date: Wed, 15 Mar 2023 09:05:14 -0700 (PDT) X-Google-Original-Date: Wed, 15 Mar 2023 09:04:22 PDT (-0700) Subject: Re: [PATCH RFC] RISC-V: alter the special character used in FAKE_LABEL_NAME In-Reply-To: <150b4184-62af-3f5c-c07b-24b0c2ae788f@suse.com> CC: binutils@sourceware.org, Andrew Waterman , Jim Wilson , nelson@rivosinc.com From: Palmer Dabbelt To: jbeulich@suse.com Message-ID: Mime-Version: 1.0 (MHng) Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-4.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,KAM_STOCKGEN,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=no 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 Fri, 10 Mar 2023 01:36:31 PST (-0800), jbeulich@suse.com wrote: > The use of a space char there collides with anything using temp_ilp(), > i.e. at present at least with the special % operator available in > alternate macro mode. Further uses of the function may appear at any > time. This likely is a result of write.h's comment not properly > spelling out all the constraints placed on the selection of the special > character used. Then again RISC-V anyway violates a constraint which has > been properly spelled out there: Such labels _do_ appear in assembler > output. > --- > RFC: Of course this breaks interoperability between older gas / new > objdump and vice versa. But I don't see a way to resolve the issue > at hand without introducing such a discontinuity. To limit "damage" > a little, riscv_symbol_is_valid() could of course be tought to also > ignore old style fake label names. (Personally I view this tying of > functionality between assembler and disassembler as problematic > anyway.) Thoughts? This wouldn't be the first time we've made a change around here in RISC-V land, see 2469b3c5844 ("Riscv ld-elf/stab failure and fake label cleanup."). Unless I'm missing something we maintained compatibility with the old binaries there, I'd prefer if we can do that here too. That said... > I question the use of FAKE_LABEL_NAME in make_internal_label(). The > comment in tc-riscv.h isn't correct anyway because of (at least) this > use - the symbols generated there are never used for Dwarf. And them all > being the same makes it rather hard to associate relocations resolved to > symbol names (e.g. "objdump -dr" output) with the actual instance that's > referenced. Their naming should imo rather follow the model of > {fb,dollar}_label_name(). > > Considering the special casing of FAKE_LABEL_CHAR in read_symbol_name() > and get_symbol_name() I wonder in how far LOCAL_LABEL_CHAR and/or > DOLLAR_LABEL_CHAR don't also need recognizing there (and then also be > marked in lex[] just like done here). I can't point out a specific case > though where there could be a problem. > > The checking for *_LABEL_CHAR in S_IS_LOCAL() looks to collide with uses > of these characters in quoted symbol names. ... if this is all broken anyway then I guess compatibility doesn't matter that much? I've definately seen some oddness around here, but I think it generally works. > --- a/gas/app.c > +++ b/gas/app.c > @@ -200,6 +200,11 @@ do_scrub_begin (int m68k_mri ATTRIBUTE_U > lex['-'] = LEX_IS_SYMBOL_COMPONENT; > #endif > > + /* If not otherwise used (which it shouldn't be - see write.h), mark the > + fake label character as a possible part of symbol names. */ > + if (!lex[(unsigned char) FAKE_LABEL_CHAR]) > + lex[(unsigned char) FAKE_LABEL_CHAR] = LEX_IS_SYMBOL_COMPONENT; > + > #ifdef H_TICK_HEX > if (enable_h_tick_hex) > { > --- a/gas/config/tc-riscv.h > +++ b/gas/config/tc-riscv.h > @@ -38,8 +38,7 @@ struct expressionS; > #define LOCAL_LABELS_FB 1 > > /* Symbols named FAKE_LABEL_NAME are emitted when generating DWARF, so make > - sure FAKE_LABEL_NAME is printable. It still must be distinct from any > - real label name. So, append a space, which other labels can't contain. */ > + sure FAKE_LABEL_NAME is printable. See write.h for constraints. */ > #define FAKE_LABEL_NAME RISCV_FAKE_LABEL_NAME > /* Changing the special character in FAKE_LABEL_NAME requires changing > FAKE_LABEL_CHAR too. */ > --- a/gas/testsuite/gas/all/altmacro.d > +++ b/gas/testsuite/gas/all/altmacro.d > @@ -8,4 +8,4 @@ > > Contents of section .data: > 0000 01020912 61626331 32332121 3c3e2721 .* > - 0010 3c3e27.* > + 0010 3c3e27a2 11.* > --- a/gas/testsuite/gas/all/altmacro.s > +++ b/gas/testsuite/gas/all/altmacro.s > @@ -33,3 +33,9 @@ m4 "!!<>'" > .altmacro > > m3 "!!<>'" > + > + .macro m2b v1, v2 > + m1 %v2-v1 17 > + .endm > + > + m2b 81 243 > --- a/gas/write.h > +++ b/gas/write.h > @@ -30,7 +30,8 @@ > /* This is a special character that is used to indicate a fake label. > It must be present in FAKE_LABEL_NAME, although it does not have to > be the first character. It must not be a character that would be > - found in a valid symbol name. > + found in a valid symbol name, nor one that starts an operator, nor > + one that's a separator of some kind. > > Also be aware that the function _bfd_elf_is_local_label_name in > bfd/elf.c has an implicit assumption that FAKE_LABEL_CHAR is '\001'. > --- a/include/opcode/riscv.h > +++ b/include/opcode/riscv.h > @@ -323,9 +323,10 @@ static inline unsigned int riscv_insn_le > > /* These fake label defines are use by both the assembler, and > libopcodes. The assembler uses this when it needs to generate a fake > - label, and libopcodes uses it to hide the fake labels in its output. */ > -#define RISCV_FAKE_LABEL_NAME ".L0 " > -#define RISCV_FAKE_LABEL_CHAR ' ' > + label, and libopcodes uses it to hide the fake labels in its output. > + See gas/write.h for constraints. */ > +#define RISCV_FAKE_LABEL_NAME ".L0?" > +#define RISCV_FAKE_LABEL_CHAR '?' > > /* Replace bits MASK << SHIFT of STRUCT with the equivalent bits in > VALUE << SHIFT. VALUE is evaluated exactly once. */