From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt1-x836.google.com (mail-qt1-x836.google.com [IPv6:2607:f8b0:4864:20::836]) by sourceware.org (Postfix) with ESMTPS id 56C883858C00 for ; Sat, 8 Jul 2023 05:59:48 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 56C883858C00 Authentication-Results: sourceware.org; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=google.com Received: by mail-qt1-x836.google.com with SMTP id d75a77b69052e-401f4408955so67671cf.1 for ; Fri, 07 Jul 2023 22:59:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20221208; t=1688795987; x=1691387987; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=q44XBgh26q49wkt8y6kA16hqVmhX53b49oo5cpNYFMI=; b=1P6llDXo673Lug0Gp+mbTQGc77vjrzZ5AGPlZt61S32YkSiN2kgjlmPFxcXjNUTAUa VUKY6GwYSnjWTi6ktViRtFkic4XIaEpPxY/eLyhyJElNGKgj5Q/eJsF0DRalX9RkOOtF DkbR3T9XbPuboKg50RQPtF2h9dbFbSdWMdYu1ATBbAFjO6H28QWNX0YjSkXoXLD4Qu1Y 3X/tBQ3l4y/I/jCCkcEWyD4Jmvmon2KozGMBeaTfQ2NGmOgaM9rqQ9vx9PojK2/zlBuU 30HvwXI7afPQI7N0ZPouJrZ3CAE+4/bIuBpm9ed3iM9ivW1c0XTlcudNgqupDg1dGlMG Yhkg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1688795987; x=1691387987; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=q44XBgh26q49wkt8y6kA16hqVmhX53b49oo5cpNYFMI=; b=U/y5XKM7pRCOe5aQxkolCGkHg9IYTBN5/0iYooaiPRunMeBeXGWb+3sAtUUpvt2jFc WqSyad0gZewNhIe0DDewcQv1pbQUWNDAATx2XVMzQOWUTy70tGIPuwGlZRxpK/FbAa+6 3VYuFcVpWQ32jFzN+xuqBKm7zDWEY1+KH9wIs3D8+Hd4ewIE6c1vzhF0cx1qHFiwgBPT tFEDfUo7+PY28tHIFUG/a/v0XDdyF/i7KpQHnr1doIk+cfMhhodi/SWer3wxwnf9+2/6 bjNRSeFG1wJ/j61HA2hBHglq4mpwLtfT8srW/1QQjRDF6KxdkniPn3xluTMGhErXXakX Hd7Q== X-Gm-Message-State: ABy/qLZB3CGAYx6Lfu0OCy95AFFZ8doz1wg2WCboZ8Z5sqeydsAtMzVx t3uwgMW2FkGZNvWf83kLaf2c3RQwJOsPCeL/u8tYvA== X-Google-Smtp-Source: APBJJlFzp6bvUKnOJ8v3yJ2Q6wYfb6CwrqwqihGNcQ8DamzeFCG0KSZapVJpJD7qUXsuoO6ikUxMpp27HgmG7mR29BA= X-Received: by 2002:a05:622a:188e:b0:3f7:ff4a:eae5 with SMTP id v14-20020a05622a188e00b003f7ff4aeae5mr110100qtc.12.1688795987529; Fri, 07 Jul 2023 22:59:47 -0700 (PDT) MIME-Version: 1.0 References: <20230628231610.220112-1-maskray@google.com> <20230707055139.3ix2gotioz6dr2b3@google.com> <82f0bb78-9a64-c614-4211-c9b05ca4719d@suse.com> In-Reply-To: <82f0bb78-9a64-c614-4211-c9b05ca4719d@suse.com> From: Fangrui Song Date: Fri, 7 Jul 2023 22:59:36 -0700 Message-ID: Subject: Re: [PATCH v2] PR30592 objcopy: allow --set-section-flags to add or remove SHF_X86_64_LARGE To: Jan Beulich Cc: binutils@sourceware.org, Alan Modra Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-17.0 required=5.0 tests=BAYES_00,BODY_8BITS,DKIMWL_WL_MED,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,ENV_AND_HDR_SPF_MATCH,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE,USER_IN_DEF_DKIM_WL,USER_IN_DEF_SPF_WL 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 Fri, Jul 7, 2023 at 12:27=E2=80=AFAM Jan Beulich wro= te: > > On 07.07.2023 07:51, Fangrui Song wrote: > > On 2023-07-06, Jan Beulich wrote: > > > > Thanks for the review! > > Uploaded PATCH v3 https://sourceware.org/pipermail/binutils/2023-July/1= 28306.html > > It is perhaps worth considering to wait with sending a new version until > the discussion on the earlier one has settled. OK. I was used to other review systems where providing quick update can allow other reviewers to skip the to-be-improved parts in the old patch... I guess in this case I could have waited a bit. > >>> @@ -7940,6 +7947,9 @@ _bfd_elf_init_private_section_data (bfd *ibfd, > >>> elf_section_flags (osec) =3D (elf_section_flags (isec) > >>> & (SHF_MASKOS | SHF_MASKPROC)); > >>> > >>> + if (get_elf_backend_data (ibfd)->elf_machine_code =3D=3D EM_X86_64= ) > >>> + elf_section_flags (osec) =3D (elf_section_flags (isec) & ~SHF_X8= 6_64_LARGE); > >> > >> What is this about? You're overwriting what the previous statement has > >> written. > > > > This behavior is tested by large-sections-2.d: objcopy > > --set-section-flags ... without "large" should drop SHF_X86_64_LARGE. > > If SEC_ELF_LARGE is set ("large" is included), elf_fake_section will ad= d back > > SHF_X86_64_LARGE. > > > > > > ( > > Note that we don't clear SHF_EXCLUDE (unfortunately part of > > SHF_MASKPROC), so --set-section-flags without "exclude" doesn't drop > > SHF_EXCLUDE. > > rg 'SHF.*0x80000000' include/ has many occurrences. I have no idea how > > to treat them yet... > > ) > > Which is part of my earlier comment: You should not undo what the > immediately preceding line does. It also doesn't feel right to do the > masking-off of SHF_X86_64_LARGE right here, as the function has (aiui) > purposes beyond its use in the course of handling --set-section-flags. Thanks for Alan's hints. Addressed in v4. The SHF_X86_64_LARGE bits are all moved to bfd/elf64-x86-64.c in PATCH v4: https://sourceware.org/pipermail/binutils/2023-July/128332.html > >>> --- a/binutils/objcopy.c > >>> +++ b/binutils/objcopy.c > >>> @@ -797,6 +797,7 @@ parse_flags (const char *s) > >>> PARSE_FLAG ("contents", SEC_HAS_CONTENTS); > >>> PARSE_FLAG ("merge", SEC_MERGE); > >>> PARSE_FLAG ("strings", SEC_STRINGS); > >>> + PARSE_FLAG ("large", SEC_ELF_LARGE); > >>> #undef PARSE_FLAG > >>> else > >>> { > >>> @@ -807,7 +808,7 @@ parse_flags (const char *s) > >>> copy[len] =3D '\0'; > >>> non_fatal (_("unrecognized section flag `%s'"), copy); > >>> fatal (_("supported flags: %s"), > >>> - "alloc, load, noload, readonly, debug, code, data, rom, = exclude, share, contents, merge, strings"); > >>> + "alloc, load, noload, readonly, debug, code, data, rom, = exclude, share, contents, merge, strings, large"); > >>> } > >> > >> So what about someone specifying "large" for a target other the x86-64= /ELF? > >> Aiui there'll be no indication whatsoever that the flag specification = didn't > >> take any effect. > > > > Changed this to look like > > > > "..." > > "share, contents, merge, strings, (ELF x86-64 specific) large" > > > > Ideally we should report an error for other targets, but I don't find a > > convenient way to detect ELF x86-64... I think at the option parsing > > time the target isn't known yet. > > The error can well be reported later, but potentially (and silently) > affecting unrelated flags on other targets because of a wrong use of > a command line option is a no-go imo. OK, I managed to find check_new_section_flags. I have played with different -O values and decided to implement the error check under the condition `bfd_get_flavour (abfd) =3D=3D bfd_target_elf_flavour`. objcopy --set-section-flags .data=3Dalloc,large -O binary a.o # fine objcopy --set-section-flags .data=3Dalloc,large -O elf32-i386 a.o # error + /* Report a fatal error if 'large' is used with a non-x86-64 ELF target. + Suppress the error for non-ELF targets to allow -O binary and formats= that + use the bit value SEC_ELF_LARGE for other purposes. */ + if ((flags & SEC_ELF_LARGE) !=3D 0 + && bfd_get_flavour (abfd) =3D=3D bfd_target_elf_flavour + && get_elf_backend_data (abfd)->elf_machine_code !=3D EM_X86_64) + { + fatal (_ ("%s[%s]: 'large' flag is ELF x86-64 specific"), + bfd_get_filename (abfd), secname); + flags &=3D ~SEC_ELF_LARGE; + } > >>> --- a/include/elf/common.h > >>> +++ b/include/elf/common.h > >>> @@ -588,6 +588,8 @@ > >>> > >>> #define SHF_GNU_MBIND 0x01000000 /* Mbind section. */ > >>> > >>> +#define SHF_X86_64_LARGE 0x10000000 > >> > >> elf/x86-64.h already has such a #define, and that's imo the only place > >> where it should live. > > > > This is a bit unfortunate, but bfd/elf.c only includes elf/common.h. > > I think bfd/elf.c likely cannot include target-specific headers. > > elf/common.h already has machine-specific NT_*_* and GNU_PROPERTY_X86_*= , so I > > hope that defining SHF_X86_64_LARGE isn't too bad. > > I'm afraid I have to defer to Nick or Alan here; personally I wouldn't > approve a change adding such a duplicate definition. To me it suggests > that use of such constants in bfd/elf.c isn't intended, and hence things > need arranging differently (which may mean a larger overall change is > needed, to properly abstract the handling of arch-specific flags). > > Jan Removed all x86-64-specific extra code from bfd/elf.c in PATCH v4. Playing with objcopy.c:parse_flags and BFD hooks bfd_elf64_bfd_copy_private_section_data/elf_backend_section_flags/elf_backe= nd_fake_sections makes me feel that the UI of --set-section-flags isn't convenient. Filed https://sourceware.org/bugzilla/show_bug.cgi?id=3D30623 ("objcopy --set-section-flags: support toggling a flag"). --=20 =E5=AE=8B=E6=96=B9=E7=9D=BF