From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pl1-x62c.google.com (mail-pl1-x62c.google.com [IPv6:2607:f8b0:4864:20::62c]) by sourceware.org (Postfix) with ESMTPS id 43C6F3858C60 for ; Fri, 7 Jul 2023 05:51:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 43C6F3858C60 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-pl1-x62c.google.com with SMTP id d9443c01a7336-1b52875b8d9so113685ad.0 for ; Thu, 06 Jul 2023 22:51:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20221208; t=1688709104; x=1691301104; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=qt+2EJi/PKygUX67MS2s0KyKeBvvx9BUNfa/+ryPA6s=; b=Si5J7036S00K7tf8Pna4XBoD1B7tg5TrXwiydYNzNLJoS21fnhqagFIUwQy5qa5ZhI VNacuMBacGWJzQwdj2Mw7TCiL0zvA46kGOZoD+/BzJMOZ9APUYEV5k8tj9EyMNNP6Fn2 52udCDjTQP+1JbD5r54qneVtPUZB+U9WWTmzaXbp+0MB6ucuyKmSNn3wYn7KuxOy/qj6 1PDVvNI+Ezhpd0GfXphqQDyHG/3SI7TSwWqtqIGg9rl5yVrw5bxsdafah5rTp9l98ZjM rol+XwTdQmh4G+GByMtNqnrXFYEd505FVBbtLvE2ZKqE+GEL9OedbC42TMaAdG6f5s44 nNzg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1688709104; x=1691301104; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=qt+2EJi/PKygUX67MS2s0KyKeBvvx9BUNfa/+ryPA6s=; b=Svyk0PbzVep5/K3HCpEQawU+hE/s2S7gbFP/WWO5HN3jtdMiYR6ONXAD/wuyXbioek 2b3k5xzcwwYOAdjtJTYVT2VSqVUEiVpSzOtUl51cpYB2A3phirn8T/r2eAU5V5/9Gm09 Dq1GwYDOr5uiUF5oU/eoTGXPuIdoCdAaI4uiqxu4k5LaSV6TYO+yLaQZAmqjKMxTE441 JdyatqFqIEAyLMdyxbM/pn7W+wol36j6mOOLwIgbAniCEINucO6J8nRev6bEkEYgzQ5y 6sT0+WWGT5vwDoxlPFMUiqxVcUibnqqmGC0m+DiWRxzTHABmiVa21AAMli0SZPPoURA7 i57g== X-Gm-Message-State: ABy/qLa3+W1+He2PyTd1cveYz72mCee/p3vhK/j1RTpFSbmfE4K1ru+2 +SSOkox8vIbNO+Xh+uOcMijfBWviN7k9ni5chKEtMw== X-Google-Smtp-Source: APBJJlFiS400QoRgemydZKTmpVeev64VikG5NRTXxiu6ow2LhsV28+9BH4MVLM9aUZl3ccHarBBeTA== X-Received: by 2002:a17:903:647:b0:1b3:db56:9ca9 with SMTP id kh7-20020a170903064700b001b3db569ca9mr169467plb.2.1688709104043; Thu, 06 Jul 2023 22:51:44 -0700 (PDT) Received: from google.com ([2620:15c:2d3:205:2d5e:47ed:cba0:11f]) by smtp.gmail.com with ESMTPSA id u2-20020a17090a6a8200b00263d7c5323dsm678573pjj.49.2023.07.06.22.51.43 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 06 Jul 2023 22:51:43 -0700 (PDT) Date: Thu, 6 Jul 2023 22:51:39 -0700 From: Fangrui Song To: Jan Beulich Cc: binutils@sourceware.org Subject: Re: [PATCH v2] PR30592 objcopy: allow --set-section-flags to add or remove SHF_X86_64_LARGE Message-ID: <20230707055139.3ix2gotioz6dr2b3@google.com> References: <20230628231610.220112-1-maskray@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: X-Spam-Status: No, score=-19.9 required=5.0 tests=BAYES_00,DKIMWL_WL_MED,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,ENV_AND_HDR_SPF_MATCH,FSL_HELO_FAKE,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 2023-07-06, Jan Beulich wrote: Thanks for the review! Uploaded PATCH v3 https://sourceware.org/pipermail/binutils/2023-July/128306.html >On 29.06.2023 01:16, Fangrui Song via Binutils wrote: >> --- a/bfd/bfd-in2.h >> +++ b/bfd/bfd-in2.h >> @@ -633,6 +633,9 @@ typedef struct bfd_section >> /* This section contains vliw code. This is for Toshiba MeP only. */ >> #define SEC_MEP_VLIW 0x20000000 >> >> + /* This section has the SHF_X86_64_LARGE flag. This is ELF x86-64 only. */ >> +#define SEC_ELF_LARGE 0x20000000 > >Is this taking the same value as SEC_MEP_VLIW and SEC_TIC54X_CLINK >deliberate? If so, the comment below on binutils/objcopy.c is yet >more relevant than I first thought, as the flag could then >unintentionally become set from (mis)using "large" there. Yes, updated the description in v3. >Also this is a generated file; you want to primarily edit section.c >if I recall correctly. Ack. `make -C $build/bfd headers` updates bfd-in2.h. Changed. >> --- a/bfd/elf.c >> +++ b/bfd/elf.c >> @@ -1034,6 +1034,10 @@ _bfd_elf_make_section_from_shdr (bfd *abfd, >> if ((hdr->sh_flags & SHF_EXCLUDE) != 0) >> flags |= SEC_EXCLUDE; >> >> + if (get_elf_backend_data (abfd)->elf_machine_code == EM_X86_64) >> + if ((hdr->sh_flags & SHF_X86_64_LARGE) != 0) >> + flags |= SEC_ELF_LARGE; > >Such two if()-s want to be joined to one, imo. Also style-wise it >would be nice if this and ... Updated. >> @@ -3351,6 +3355,9 @@ elf_fake_sections (bfd *abfd, asection *asect, void *fsarg) >> } >> if ((asect->flags & (SEC_GROUP | SEC_EXCLUDE)) == SEC_EXCLUDE) >> this_hdr->sh_flags |= SHF_EXCLUDE; >> + if (asect->flags & SEC_ELF_LARGE) >> + if (get_elf_backend_data (abfd)->elf_machine_code == EM_X86_64) >> + this_hdr->sh_flags |= SHF_X86_64_LARGE; > >... this could match (in order of checks as well as in whether or not >!= 0 is used on the result of &. Updated. >> @@ -7940,6 +7947,9 @@ _bfd_elf_init_private_section_data (bfd *ibfd, >> elf_section_flags (osec) = (elf_section_flags (isec) >> & (SHF_MASKOS | SHF_MASKPROC)); >> >> + if (get_elf_backend_data (ibfd)->elf_machine_code == EM_X86_64) >> + elf_section_flags (osec) = (elf_section_flags (isec) & ~SHF_X86_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 add 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... ) >> --- 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] = '\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. >> --- 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. > >Jan 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.